Merge lp:~vanvugt/compiz/fix-1028809 into lp:compiz/0.9.9

[Daniel van Vugt]

Move opacity/brightness/saturation setting from glDraw into glDrawTexture
where it won't care about the plugin load order relative to decor any more.
This ensures the obs setting will always apply to the whole window including
decorations (LP: #1028809)

[Daniel van Vugt (vanvugt)]

I think the primary cause of this regression can be seen in r3320 (GLES merge). The glDraw parameters got const'ified properly, so obs could no longer modify the global window paint attrib and could only pass its changes down the call chain. So it became more sensitive to the plugin load order.

[Sam Spilsbury (smspillaz)]

It might be better to wrap glDrawTexture here and apply the opacity, brightness and saturation in the GLWindowPaintAttrib.

That will be guaranteed to run on every texture that is drawn for some window, unless the plugin short circuits glDrawTexture, which they usually never do.

I'm just noting the comment here:

42 -/* Note: Normally plugins should wrap into glPaintWindow to modify opacity,
43 - brightness and saturation. As some plugins bypass glPaintWindow when
44 - they draw windows and our custom values always need to be applied,
45 - we wrap into glDrawWindow here */

[Daniel van Vugt (vanvugt)]

I think it's more correct (and efficient) to only set the o/b/s of a window once per plugin per frame. That means in glPaint whereas glDrawTexture would do it many times per window per frame.

[Sam Spilsbury (smspillaz)]

I agree, but the problem is that as the comment above said, plugins
can just unintentionally bypass the o/b/s settings.

Unless that's not an issue?

On Wed, Nov 21, 2012 at 4:55 PM, Daniel van Vugt
<email address hidden> wrote:
> I think it's more correct (and efficient) to only set the o/b/s of a window once per plugin per frame. That means in glPaint whereas glDrawTexture would do it many times per window per frame.
> --
> https://code.launchpad.net/~vanvugt/compiz/fix-1028809/+merge/135329
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~vanvugt/compiz/fix-1028809 into lp:compiz.

--
Sam Spilsbury

[Daniel van Vugt (vanvugt)]

That comment is old and I'm not yet aware of any use case where it applies any more. Certainly I would not move the o/b/s setting into DrawTexture unless I really-really had to. It gets called too many times per frame.

Maybe as part of this review we should all test for plugins where o/b/s used to work and doesn't any more. If no one can find a combination that's broken then it's probably not an issue.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3478
http://jenkins.qa.ubuntu.com/job/compiz-ci/256/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/256/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/256//rebuild/?

review: Approve (continuous-integration)

[Sam Spilsbury (smspillaz)]

The only ones would be switcher, staticswitcher, shift, ring.

Those call glDraw directly, so the o/b/s settings won't be applied to
those previews.

On Wed, Nov 21, 2012 at 5:32 PM, Daniel van Vugt
<email address hidden> wrote:
> That comment is old and I'm not yet aware of any use case where it applies any more. Certainly I would not move the o/b/s setting into DrawTexture unless I really-really had to. It gets called too many times per frame.
>
> Maybe as part of this review we should all test for plugins where o/b/s used to work and doesn't any more. If no one can find a combination that's broken then it's probably not an issue.
> --
> https://code.launchpad.net/~vanvugt/compiz/fix-1028809/+merge/135329
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~vanvugt/compiz/fix-1028809 into lp:compiz.

--
Sam Spilsbury

[Daniel van Vugt (vanvugt)]

Confirmed regressions in switcher and staticswitcher.

shift and ring still work fine.

review: Needs Fixing

[lp:~vanvugt/compiz/fix-1028809]

3479. By Daniel van Vugt on 2012-11-22

Revert previous revision. Needs a better approach.

3480. By Daniel van Vugt on 2012-11-22

Same fix as before but now in glDrawTexture so it works with switcher plugins
etc that never glPaint.

[Daniel van Vugt (vanvugt)]

review: Abstain

[Sam Spilsbury (smspillaz)]

+1 although there's a weird whitespace change here

29 -/* Note: Normally plugins should wrap into glPaintWindow to modify opacity,
30 - brightness and saturation. As some plugins bypass glPaintWindow when
31 - they draw windows and our custom values always need to be applied,
32 - we wrap into glDrawWindow here */
33 +/* Note: Normally plugins should wrap into glPaint to modify opacity,
34 + brightness and saturation. As some plugins bypass glPaint when
35 + they draw windows and our custom values always need to be applied,
36 + we wrap into glDrawTexture here */

Also is this still necessary?

8 - <plugin>decor</plugin>
9 <plugin>blur</plugin>

[Daniel van Vugt (vanvugt)]

The whitespace change is intentional. I switched tabs to spaces to guarantee the text lines up with "/* Note: " regardless of the tab size. Code should always look correct and properly aligned regardless of the tab size you're using. But I admit that's not true for most of compiz.

Yes the XML change is technically correct. However I think only CCSM ever pays attention to it.

[Sam Spilsbury (smspillaz)]

Ok then :)

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3480
http://jenkins.qa.ubuntu.com/job/compiz-ci/257/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/257/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/257//rebuild/?

review: Approve (continuous-integration)

=== modified file 'plugins/obs/obs.xml.in'
--- plugins/obs/obs.xml.in	2011-09-09 09:54:02 +0000
+++ plugins/obs/obs.xml.in	2012-11-22 06:49:18 +0000
@@ -9,7 +9,6 @@
 		<plugin>opengl</plugin>
 	    </requirement>
 	    <relation type="after">
-		<plugin>decor</plugin>
 		<plugin>blur</plugin>
 	    </relation>
 	</deps>

=== modified file 'plugins/obs/src/obs.cpp'
--- plugins/obs/src/obs.cpp	2012-09-07 23:29:42 +0000
+++ plugins/obs/src/obs.cpp	2012-11-22 06:49:18 +0000
@@ -115,7 +115,7 @@
 	    break;
 	}
 
-    gWindow->glDrawSetEnabled (this, hasCustom);
+    gWindow->glDrawTextureSetEnabled (this, hasCustom);
     cWindow->addDamage ();
 }
 
@@ -149,16 +149,16 @@
     return gWindow->glPaint (attrib, transform, region, mask);
 }
 
-/* Note: Normally plugins should wrap into glPaintWindow to modify opacity,
-	 brightness and saturation. As some plugins bypass glPaintWindow when
-	 they draw windows and our custom values always need to be applied,
-	 we wrap into glDrawWindow here */
+/* Note: Normally plugins should wrap into glPaint to modify opacity,
+         brightness and saturation. As some plugins bypass glPaint when
+         they draw windows and our custom values always need to be applied,
+         we wrap into glDrawTexture here */
 
-bool
-ObsWindow::glDraw (const GLMatrix            &transform,
-		   const GLWindowPaintAttrib &attrib,
-		   const CompRegion          &region,
-		   unsigned int        mask)
+void
+ObsWindow::glDrawTexture (GLTexture                 *texture,
+                          const GLMatrix            &transform,
+                          const GLWindowPaintAttrib &attrib,
+                          unsigned int              mask)
 {
     GLWindowPaintAttrib wAttrib (attrib);
     int factor;
@@ -178,7 +178,7 @@
     if (factor != 100)
 	wAttrib.saturation = factor * wAttrib.saturation / 100;
 
-    return gWindow->glDraw (transform, wAttrib, region, mask);
+    return gWindow->glDrawTexture (texture, transform, wAttrib, mask);
 }
 
 void

=== modified file 'plugins/obs/src/obs.h'
--- plugins/obs/src/obs.h	2012-09-07 23:29:42 +0000
+++ plugins/obs/src/obs.h	2012-11-22 06:49:18 +0000
@@ -64,8 +64,10 @@
 
 	bool glPaint (const GLWindowPaintAttrib &, const GLMatrix &,
 		      const CompRegion &, unsigned int);
-	bool glDraw (const GLMatrix &, const GLWindowPaintAttrib &,
-		     const CompRegion &, unsigned int);
+	void glDrawTexture (GLTexture                 *texture,
+	                    const GLMatrix            &transform,
+	                    const GLWindowPaintAttrib &attrib,
+	                    unsigned int              mask);
 
 	void changePaintModifier (unsigned int, int);
 	void updatePaintModifier (unsigned int);