951554 - Only sync what we will not paint anyway when painting with double buffers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=f42271f231d3

Comment 1

[Nick Cameron [:nrc]]

Between this and bug 951556 we make some pretty good gains on the talos numbers for drawing heavy tests - svg opacity/canvasmark (still not quite as good as without OMTC, but getting there) - and some small losses on other tests. I think they are small enough that they might be noise or due to a couple of weeks more landings on m-c, but the fact that _all_ the tests I looked at slowed down a little is a bit sad-making. Still I think the gains make it worth while.

Comment 2

[Nick Cameron [:nrc]]

Attachment: Part 1: Don't sync buffers if we are going to throw away our buffers

Comment 3

[Nick Cameron [:nrc]]

Attachment: Part 2: Don't sync anywhere we will paint over

Things are bit messy in these two patches, but I tidy up in the next two.

Comment 4

[Nick Cameron [:nrc]]

Attachment: Part 3: Refactor stuff out of ClientThebesLayer and into ContentClient

Comment 5

[Nick Cameron [:nrc]]

Attachment: Part 4: Only do the BeginPaint computation once

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8349259 [details] [diff] [review]
Part 1: Don't sync buffers if we are going to throw away our buffers

Review of attachment 8349259 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/RotatedBuffer.h
@@ +251,5 @@
> +   * Returns true if we must sync our front buffer to the back buffer. We don't
> +   * need to do this if BeginPaint is going to throw away our buffers without
> +   * copying them to the new ones.
> +   */
> +  bool BeginPaintRequiresSyncBuffers(ThebesLayer* aLayer,

RotatedBuffer doesn't really know anything about front/back buffers and syncing. Can we just call this 'WillReallocateBufferForPaint' or something similar?

Comment 8

[Nick Cameron [:nrc]]

Before Christmas and via IRC, Matt and I discussed this in some detail. Matt suggested that instead of splitting up BeginPaint I could simply call SyncFront... after BeginPaint instead of before it, then pass the region to draw into SyncFront... This sounded like a more elegant approach, so I had a go and it almost worked. A little fixing up to remember when we took the 'self copy' path and to sync early if we are going to recreate our buffers (which should be optimised to only do this if we copy to our buffers and just not sync in the other case) and it almost works.

BUT, not really. If BeginPaint changes the buffer rect or rotation, then we will not sync the right part of the buffer. We could preserve the old values of mBufferRect/Rotation and use these for the sync, but that would be a bit weird because we would have to pass our preserved values up to RotatedBuffer where it should use the values it expects. Furthermore, it means we are updating the rotated buffer as if it were rotated to a different position. That feels really, really fragile.

Another option is to sync early in these cases. In fact we may as well do it in all cases, as soon as we know the area we are going to draw into. Which brings us back to the first solution which is effectively to Sync in the middle of BeginPaint, except we are calling out of BeginPaint, rather than breaking BeginPaint into pieces and orchestrating from PaintThebes. I'm not sure which approach is better, the existing approach has the advantage that it refactors BeginPaint a bit (and it has got pretty gross). The new approach changes less code. (We have to split syncfront..., but that is a much smaller and simpler method).

Comment 9

[Nick Cameron [:nrc]]

Talos: https://tbpl.mozilla.org/?tree=Try&rev=65bdecbd84a3

Comment 10

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=9171430da82e

Comment 11

[Nick Cameron [:nrc]]

(In reply to Nick Cameron [:nrc] from comment #8)
> Before Christmas and via IRC, Matt and I discussed this in some detail. Matt
> suggested that instead of splitting up BeginPaint I could simply call
> SyncFront... after BeginPaint instead of before it, then pass the region to
> draw into SyncFront... This sounded like a more elegant approach, so I had a
> go and it almost worked. A little fixing up to remember when we took the
> 'self copy' path and to sync early if we are going to recreate our buffers
> (which should be optimised to only do this if we copy to our buffers and
> just not sync in the other case) and it almost works.
> 
> BUT, not really. If BeginPaint changes the buffer rect or rotation, then we
> will not sync the right part of the buffer. We could preserve the old values
> of mBufferRect/Rotation and use these for the sync, but that would be a bit
> weird because we would have to pass our preserved values up to RotatedBuffer
> where it should use the values it expects. Furthermore, it means we are
> updating the rotated buffer as if it were rotated to a different position.
> That feels really, really fragile.
> 
> Another option is to sync early in these cases. In fact we may as well do it
> in all cases, as soon as we know the area we are going to draw into. Which
> brings us back to the first solution which is effectively to Sync in the
> middle of BeginPaint, except we are calling out of BeginPaint, rather than
> breaking BeginPaint into pieces and orchestrating from PaintThebes. I'm not
> sure which approach is better, the existing approach has the advantage that
> it refactors BeginPaint a bit (and it has got pretty gross). The new
> approach changes less code. (We have to split syncfront..., but that is a
> much smaller and simpler method).

I chose the new approach and the second version - i.e., we now call SyncFront... from BeginPaint, except that I call it FinalizeFrame.

Comment 12

[Nick Cameron [:nrc]]

Attachment: Part 1: Add SurfaceMode::SURFACE_NONE

Comment 13

[Nick Cameron [:nrc]]

Attachment: Part 2: Replace SyncFrontBufferToBackBuffer with PrepareFrame and FinalizeFrame

Comment 14

[Nick Cameron [:nrc]]

Attachment: Part 3: Changes to RotatedBuffer

Comment 15

[Nick Cameron [:nrc]]

Attachment: Part 4: Changes to ContentClient

Comment 16

[Nick Cameron [:nrc]]

Attachment: Part 5: Change the high level painting functions

Comment 17

[Nicolas Silva [:nical]]

Comment on attachment 8358213 [details] [diff] [review]
Part 3: Changes to RotatedBuffer

Review of attachment 8358213 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/RotatedBuffer.h
@@ +18,5 @@
>  #include "nsPoint.h"                    // for nsIntPoint
>  #include "nsRect.h"                     // for nsIntRect
>  #include "nsRegion.h"                   // for nsIntRegion
>  #include "nsTraceRefcnt.h"              // for MOZ_COUNT_CTOR, etc
> +#include "Layers.h"

Do we really need to include Layers.h here? it's a pretty bad header to include in another header.
If it's just for PaintState's ctor, i'd rather move the ctor implementation to the .cpp

Comment 18

[Nicolas Silva [:nical]]

Comment on attachment 8358216 [details] [diff] [review]
Part 5: Change the high level painting functions

Review of attachment 8358216 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/ClientThebesLayer.cpp
@@ +55,2 @@
>      mValidRegion.Sub(mValidRegion, state.mRegionToInvalidate);
> +    

nit: trailing space

Comment 19

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #17)
> Comment on attachment 8358213 [details] [diff] [review]
> Part 3: Changes to RotatedBuffer
> 
> Review of attachment 8358213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/RotatedBuffer.h
> @@ +18,5 @@
> >  #include "nsPoint.h"                    // for nsIntPoint
> >  #include "nsRect.h"                     // for nsIntRect
> >  #include "nsRegion.h"                   // for nsIntRegion
> >  #include "nsTraceRefcnt.h"              // for MOZ_COUNT_CTOR, etc
> > +#include "Layers.h"
> 
> Do we really need to include Layers.h here? it's a pretty bad header to
> include in another header.
> If it's just for PaintState's ctor, i'd rather move the ctor implementation
> to the .cpp

It's for Layer::SurfaceMode. C++ doesn't let you forward declare nested items, and we need to reference PaintState outside of the cpp, so I don't see any option other than including the header (well, we could move Layer::SurfaceMode out of Layer, but I don't really want to do that here - I'll file a follow up bug).

Comment 20

[Nick Cameron [:nrc]]

Filed bug 958984

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54060f623fb7
https://hg.mozilla.org/mozilla-central/rev/5f4439be303b
https://hg.mozilla.org/mozilla-central/rev/c6bbb2c41e78
https://hg.mozilla.org/mozilla-central/rev/1ff7a0feb484
https://hg.mozilla.org/mozilla-central/rev/1b3735a6c466

Comment 22

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #19)
> It's for Layer::SurfaceMode. C++ doesn't let you forward declare nested
> items, and we need to reference PaintState outside of the cpp, so I don't
> see any option other than including the header (well, we could move
> Layer::SurfaceMode out of Layer, but I don't really want to do that here -
> I'll file a follow up bug).

Ah, yeah, fair enough, and a bit sad. We should avoid nesting declarations in classes like that. Maybe even promote this to an official style guide rule.