Merge lp:~mc-return/compiz/compiz.merge-fix1182794-screenshot-not-compatible-with-unityshell.0 into lp:compiz/0.9.10

[MC Return]

Screenshot plugin:
We have to redraw and damage the painted region the whole time,
if unityshell is enabled, not only when it's size changed.
Otherwise the constant whole-screen damaging of Unity makes the
screenshot selection box disappear.

Note:
Screenshot is less efficient now, so this should probably be
reverted, once unityshell has learned to correctly damage just
repainted regions.

(LP: #1182794)

[Sam Spilsbury (smspillaz)]

review: Approve

[MC Return (mc-return)]

Sam, I am setting this back to WIP as it seems to need the additional damage call - otherwise it can get into a flickering condition...

[Sam Spilsbury (smspillaz)]

Hmm, I'm not really a fan of that approach as it causes spurious redraws due to the repaint-rescheduling happening every time a draw function is called (eg, all the time). Overdraw will certainly be the cause of flickering in some cases though.

The other option is to grab the current damage region and clip drawing to that so there is no overdraw. For example:

bool scissoringEnabled = glIsEnabled (GL_SCISSOR_TEST);
GLint lastScissorBox[4];

glGetIntegerv (GL_SCISSOR_BOX, lastScissorBox);

glEnable (GL_SCISSOR_TEST);

CompRect::vector rects = region.rects ();

foreach (const CompRect &rect, rects)
{
    glScissor (rect.x1 (),
               screen->height () - rect.y2 (),
               rect.width (),
               rect.height ());

    streamingBuffer->render ();
}

/* Reset scissor state */
if (!scissoringEnabled)
    glDisable (GL_SCISSOR_TEST);

/* Reset scissor box to old value */
glScissor (lastScissorBox[0],
           lastScissorBox[1],
           lastScissorBox[2],
           lastScissorBox[3]);

This reminds me that GLVertexBuffer could probably use a clip () or renderClipped () method so that we don't have to repeat this everywhere.

[MC Return (mc-return)]

I am not really a fan of re-fixing screenshot again as I really have the strong feeling Unity is to blame for the disappearing-screenshot-selection-box-bug...

Although the damage code could still be improved, the "non-redrawing and non-re-damaging when not moving the mouse" version seems to be very efficient and working perfectly in all cases, even with video underneath the selection box, except when Unity decides to redraw the whole screen/all screens without reason (or because one pixel in the panel changed) :(
This also leads to other nasty side-effects, like flickering annotate boxes and ellipses and really should be fixed there, not worked around in all other plugins IMHO...

Sam, don't you have some damaging fixes for nux/unity ready somewhere or am I hallucinating ?

It would really be nice if Unity could learn some efficiency in screen damaging as currently it not only creates bugs, but also degrades Compiz' performance massively... :(

[Sam Spilsbury (smspillaz)]

I do - but they don't completely fix the problem. Its only able to
make the panels and redraw (to the extent that they are just blitted
back to the backbuffer) once some damage intersects them, but it
doesn't do any of the vertex clipping stuff that compiz does.

To be honest though the problem is with the screenshot plugin and more
generally plugins that draw stuff on top of the screen directly.
There's no way to clip to damage regions with those, which is what
causes overdraw related flickering.

On Tue, May 28, 2013 at 12:37 AM, MC Return <email address hidden> wrote:
> I am not really a fan of re-fixing screenshot again as I really have the strong feeling Unity is to blame for the disappearing-screenshot-selection-box-bug...
>
> Although the damage code could still be improved, the "non-redrawing and non-re-damaging when not moving the mouse" version seems to be very efficient and working perfectly in all cases, even with video underneath the selection box, except when Unity decides to redraw the whole screen/all screens without reason (or because one pixel in the panel changed) :(
> This also leads to other nasty side-effects, like flickering annotate boxes and ellipses and really should be fixed there, not worked around in all other plugins IMHO...
>
> Sam, don't you have some damaging fixes for nux/unity ready somewhere or am I hallucinating ?
>
> It would really be nice if Unity could learn some efficiency in screen damaging as currently it not only creates bugs, but also degrades Compiz' performance massively... :(
> --
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix1182794-screenshot-not-compatible-with-unityshell.0/+merge/165112
> You are reviewing the proposed merge of lp:~mc-return/compiz/compiz.merge-fix1182794-screenshot-not-compatible-with-unityshell.0 into lp:compiz.

--
Sam Spilsbury

[MC Return (mc-return)]

I am waiting with the final fix for this until the damage handling of Unity has been improved or until shortly before the release of 0.9.10, although I really would like to finish the work on screenshot and maybe just add features to this plugin (like the possibility of taking screenshots of individual windows)...

Unmerged revisions

3723. By MC Return on 2013-05-22

Reduced the .diff to make review easier

3722. By MC Return on 2013-05-22

Screenshot plugin:
We have to redraw and damage the painted region the whole time,
if unityshell is enabled, not only when it's size changed
Otherwise the constant whole-screen damaging of Unity makes the
screenshot selection box disappear

Note:
Screenshot is less efficient now, so this should probably be
reverted, once unityshell has learned to correctly damage just
repainted regions

(LP: #1182794)

=== modified file 'plugins/screenshot/src/screenshot.cpp'
--- plugins/screenshot/src/screenshot.cpp	2013-05-09 13:43:07 +0000
+++ plugins/screenshot/src/screenshot.cpp	2013-05-22 13:30:54 +0000
@@ -456,8 +456,7 @@
     if (status && mGrab)
     {
 	/* We just want to draw the screenshot selection box if
-	 * we are grabbed, the size has changed and the CCSM
-	 * option to draw it is enabled. */
+	 * we are grabbed and the CCSM option to draw it is enabled. */
 
 	CompRect selectionRect (std::min (mX1, mX2),
 				std::min (mY1, mY2),
@@ -465,7 +464,6 @@
 				std::abs (mY2 - mY1));
 
 	if (mGrabIndex &&
-	    selectionSizeChanged &&
 	    optionGetDrawSelectionIndicator ())
 	{
 	    GLMatrix        transform (matrix);
@@ -481,10 +479,6 @@
 					    optionGetSelectionOutlineColor (),
 					    streamingBuffer,
 					    transform);
-
-	    /* we finished painting the selection box,
-	     * reset selectionSizeChanged now */
-	    selectionSizeChanged = false;
 	}
 	else if (!mGrabIndex)
 	{
@@ -508,9 +502,6 @@
     if (mGrabIndex &&
 	(mX2 != xRoot || mY2 != yRoot))
     {
-	/* the size has changed now */
-	selectionSizeChanged = true;
-
 	int x1 = MIN (mX1, mX2) - 1;
 	int y1 = MIN (mY1, mY2) - 1;
 	int x2 = MAX (mX1, mX2) + 1;
@@ -558,8 +549,7 @@
     cScreen (CompositeScreen::get (screen)),
     gScreen (GLScreen::get (screen)),
     mGrabIndex (0),
-    mGrab (false),
-    selectionSizeChanged (false)
+    mGrab (false)
 {
     optionSetInitiateButtonInitiate (boost::bind (&ShotScreen::initiate, this,
 						  _1, _2, _3));

=== modified file 'plugins/screenshot/src/screenshot.h'
--- plugins/screenshot/src/screenshot.h	2013-04-24 11:38:36 +0000
+++ plugins/screenshot/src/screenshot.h	2013-05-22 13:30:54 +0000
@@ -66,7 +66,6 @@
 
 	CompScreen::GrabHandle mGrabIndex;
 	bool                   mGrab;
-	bool                   selectionSizeChanged;
 
 	int  mX1, mY1, mX2, mY2;
 };