Merge lp:~mc-return/compiz/compiz.merge-fix1182794-screenshot-not-compatible-with-unityshell 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.

Minor indentation fixes.

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

(LP: #1182794)

[lp:~mc-return/compiz/compiz.merge-fix1182794-screenshot-not-compatible-with-unityshell]

3723. By MC Return on 2013-05-22

Removed line committed by accident

3724. By MC Return on 2013-05-22

Added newline

3725. By MC Return on 2013-05-22

Fixed indentation

[Sam Spilsbury (smspillaz)]

"Otherwise the constant whole-screen damaging of Unity makes the
screenshot selection box disappear."

Can you explain more why you think that's happening? I don't see why an expanded damage region would cause some elements not to paint.

review: Needs Information

[lp:~mc-return/compiz/compiz.merge-fix1182794-screenshot-not-compatible-with-unityshell]

3726. By MC Return on 2013-05-22

Fixed comment

[MC Return (mc-return)]

> "Otherwise the constant whole-screen damaging of Unity makes the
> screenshot selection box disappear."
>
> Can you explain more why you think that's happening? I don't see why an
> expanded damage region would cause some elements not to paint.

No, not really - as I've not investigated the exact cause, but just analyzed
the symptomps.
I have indicator-multiload running here, which paints on top of the Unity panel
each 30 millisecs, which makes unityshell damage the whole screen each 30ms
(very efficient by the way ;) - see bug ##1182803).
When I turn off indicator-multiload, unityshell will damage the whole screen
each second, because the time display redraw (seconds) in the panel will cause
the whole screen damage each second.
Once unityshell damages the whole screen the selection box disappears, if the mouse
is not moved, but the mousebutton is not released.
Without unityshell the selection box never disappears (I had unityshell disabled,
when I tested screenshot, so I didn't notice before...).

[MC Return (mc-return)]

*bug #1182803

[MC Return (mc-return)]

Note:
I tested this behaviour on 2 different machines, with different gfx-hardware (intel/amd) -> both act exactly the same way...

[Sam Spilsbury (smspillaz)]

That sounds more like a case where the screenshot paint function is
disabled and not called when the screen next gets repainted.

On Wed, May 22, 2013 at 7:44 PM, MC Return <email address hidden> wrote:
>> "Otherwise the constant whole-screen damaging of Unity makes the
>> screenshot selection box disappear."
>>
>> Can you explain more why you think that's happening? I don't see why an
>> expanded damage region would cause some elements not to paint.
>
> No, not really - as I've not investigated the exact cause, but just analyzed
> the symptomps.
> I have indicator-multiload running here, which paints on top of the Unity panel
> each 30 millisecs, which makes unityshell damage the whole screen each 30ms
> (very efficient by the way ;) - see bug ##1182803).
> When I turn off indicator-multiload, unityshell will damage the whole screen
> each second, because the time display redraw (seconds) in the panel will cause
> the whole screen damage each second.
> Once unityshell damages the whole screen the selection box disappears, if the mouse
> is not moved, but the mousebutton is not released.
> Without unityshell the selection box never disappears (I had unityshell disabled,
> when I tested screenshot, so I didn't notice before...).
> --
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix1182794-screenshot-not-compatible-with-unityshell/+merge/165066
> You are reviewing the proposed merge of lp:~mc-return/compiz/compiz.merge-fix1182794-screenshot-not-compatible-with-unityshell into lp:compiz.

--
Sam Spilsbury

[MC Return (mc-return)]

> That sounds more like a case where the screenshot paint function is
> disabled and not called when the screen next gets repainted.
>
Maybe. If you say so -> probably. I have not checked the unity code yet.

Nevertheless, this MP here should be merged anyway, as the difference
in performance is just marginal:

1. Just the painted region (screenhot selection rectangle) gets repainted and redamaged, not the whole screen.
2. The additional damaging of this region is only happening for the time the user is holding the left-mousebutton, which in most cases will be under a half second more per screenshot.

It is the same way we handle the damaging of the zoom selection box by the way, but there it is implemented even slightly better (less damage calls)...
That could be improved here as well...

[Sam Spilsbury (smspillaz)]

I think you've got the right idea now, on a quick look it would appear that the selectionSizeChanged stuff is probably what's causing all the problems (it doesn't make sense to say "we've painted this now, don't paint it again until it changes" because it ignores intermediate frames).

Just two notes:

1.

61 + cScreen->damageRegion (CompRegion (selectionRect));

This is unnecessary. You only need to add a new region to mark damaged if the selection rectangle has changed size (as we do in handleMotionEvent). For all other paint passes caused by other damage events, the redraw will effectively be clipped to the currently processed damage. That means that if the painted selection rectangle intersects the draw region then it will be repainted just fine. Then again, the other bits are effectively overdraw, which is bad for other reasons - but its better than causing spurious repaints to be triggered from within a paint function.

2.

The unrelated code cleanups made this difficult to review properly. Try and split them out if you can, or localize them to the area of the code that's being changed.

[lp:~mc-return/compiz/compiz.merge-fix1182794-screenshot-not-compatible-with-unityshell]

3727. By MC Return on 2013-05-22

Removed redundant damage call

[MC Return (mc-return)]

> I think you've got the right idea now, on a quick look it would appear that
> the selectionSizeChanged stuff is probably what's causing all the problems (it
> doesn't make sense to say "we've painted this now, don't paint it again until
> it changes" because it ignores intermediate frames).
>
> Just two notes:
>
> 1.
>
> 61 + cScreen->damageRegion (CompRegion (selectionRect));
>
> This is unnecessary. You only need to add a new region to mark damaged if the
> selection rectangle has changed size (as we do in handleMotionEvent). For all
> other paint passes caused by other damage events, the redraw will effectively
> be clipped to the currently processed damage. That means that if the painted
> selection rectangle intersects the draw region then it will be repainted just
> fine. Then again, the other bits are effectively overdraw, which is bad for
> other reasons - but its better than causing spurious repaints to be triggered
> from within a paint function.
>

Oh - you are right.
I think we could even optimize it some more, but I'll leave it for now... ;)

Removed the redundant damage call.

> 2.
>
> The unrelated code cleanups made this difficult to review properly. Try and
> split them out if you can, or localize them to the area of the code that's
> being changed.

I will split out the fix from this MP.

[MC Return (mc-return)]

2. https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix1182794-screenshot-not-compatible-with-unityshell.0/+merge/165112

Unmerged revisions

3727. By MC Return on 2013-05-22

Removed redundant damage call

3726. By MC Return on 2013-05-22

Fixed comment

3725. By MC Return on 2013-05-22

Fixed indentation

3724. By MC Return on 2013-05-22

Added newline

3723. By MC Return on 2013-05-22

Removed line committed by accident

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

Indentation fixes

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:00:49 +0000
@@ -226,8 +226,8 @@
 					 GLVertexBuffer *streamingBuffer,
 					 const GLMatrix &transform)
     {
-	GLfloat         vertexData[12];
-	GLushort        colorData[4];
+	GLfloat  vertexData[12];
+	GLushort colorData[4];
 
 	int x1 = rect.x1 ();
 	int y1 = rect.y1 ();
@@ -430,14 +430,11 @@
 				  rect.height (), path);
 	}
 	else
-	{
 	    compLogMessage ("screenshot", CompLogLevelWarn, "glReadPixels failed");
-	}
 
 	if (!success)
-	    success =
-		launchApplicationAndTakeScreenshot (alternativeApplication,
-						    directory);
+	    success = launchApplicationAndTakeScreenshot (alternativeApplication,
+							  directory);
 
 	return success;
 
@@ -456,8 +453,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,10 +461,9 @@
 				std::abs (mY2 - mY1));
 
 	if (mGrabIndex &&
-	    selectionSizeChanged &&
 	    optionGetDrawSelectionIndicator ())
 	{
-	    GLMatrix        transform (matrix);
+	    GLMatrix       transform (matrix);
 	    GLVertexBuffer *streamingBuffer (GLVertexBuffer::streamingBuffer ());
 	    transform.toScreenSpace (output, -DEFAULT_Z_CAMERA);
 
@@ -481,10 +476,6 @@
 					    optionGetSelectionOutlineColor (),
 					    streamingBuffer,
 					    transform);
-
-	    /* we finished painting the selection box,
-	     * reset selectionSizeChanged now */
-	    selectionSizeChanged = false;
 	}
 	else if (!mGrabIndex)
 	{
@@ -508,9 +499,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 +546,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:00:49 +0000
@@ -46,29 +46,35 @@
 	bool initiate (CompAction            *action,
 		       CompAction::State     state,
 		       CompOption::Vector    &options);
+
 	bool terminate (CompAction            *action,
 			CompAction::State     state,
 			CompOption::Vector    &options);
+
 	void handleMotionEvent (int xRoot,
 				int yRoot);
 
 	void handleEvent (XEvent *event);
+
 	bool glPaintOutput (const GLScreenPaintAttrib &attrib,
 			    const GLMatrix            &matrix,
 			    const CompRegion          &region,
 			    CompOutput                *output,
 			    unsigned int               mask);
+
 	void paint (CompOutput::ptrList &outputs,
 		    unsigned int        mask);
 
-	CompositeScreen *cScreen;
-	GLScreen        *gScreen;
+	CompositeScreen        *cScreen;
+	GLScreen               *gScreen;
 
 	CompScreen::GrabHandle mGrabIndex;
 	bool                   mGrab;
-	bool                   selectionSizeChanged;
 
-	int  mX1, mY1, mX2, mY2;
+	int                    mX1;
+	int                    mY1;
+	int                    mX2;
+	int                    mY2;
 };
 
 class ShotPluginVTable :