Closed Bug 1150549 Opened 9 years ago Closed 9 years ago

Simplify TiledContentHost

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: nical, Assigned: nical)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 7 obsolete files)

ThebesLayerBufferComposite is way more complicated than it needs to be.
For one, it is based TiledLayerBuffer's templated Update function but doesn't need most of what Update does. It is also treated as a temporary object recreated each update, which makes a few things tricky, like deferring the release of the gfxSharedReadLock to the composition of frame N+1 (right now we keep a half-valid "mOldTiledLayerBuffer" around for that), and going through the fast paths with gralloc buffers.
It's not surprising since we had very a tight deadline when we wrote it but now is a good time to clean it up (actually, we should not have waited that long).
With this patch, TiledLayerBufferComposite is kept across frames which makes it much easier to track the gfxSharedReadLocks and avoid unbinding gralloc buffers for no reason (which we currently do).
This patch also implements all of the update logic in TiledLayerBufferComposite::UseTiles instead of going through TiledLayerBuffer::Update which is way more complicated and does a lot mor ethan what we care about. Aside from making things simpler on the host side, it'll make it easier to simplify the client side.

I still need to implement partial updates on tiles, which I intend to do by passing the bound of each tile's update region rather than reconstructing the update region on the host side. It should make things faster because 1 rather small glTexSubImage2D is faster than several very small glTexSubImage2D, and simpler.
Added partial uploads, works like a charm on Linux.
This version of the patch is green on try, except for the (already existing) ics emulator r14 intermittent. On b2g it should improve performance on the compositor side since it removes a lot of the slow egl binding that we are doing (we used to re-bind every tile after each transaction, and now we only do that for the one s that changed).
Attachment #8587454 - Attachment is obsolete: true
Attachment #8587508 - Attachment is obsolete: true
Attachment #8592178 - Attachment is obsolete: true
Attachment #8592816 - Flags: review?(jmuizelaar)
(In reply to Nicolas Silva [:nical] from comment #4)
> Created attachment 8592816 [details] [diff] [review]
> Simplify and improve the compositor-side tiling code

Do you mind if I ignore this review until next week after people leave Toronto?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> (In reply to Nicolas Silva [:nical] from comment #4)
> > Created attachment 8592816 [details] [diff] [review]
> > Simplify and improve the compositor-side tiling code
> 
> Do you mind if I ignore this review until next week after people leave
> Toronto?

No problem.
Comment on attachment 8592816 [details] [diff] [review]
Simplify and improve the compositor-side tiling code

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

One comment so far:
- It might be better to have a static PlaceHolderTile that gets
used as needed instead of moving the bounds checking out of GetTile
Can you rebase this patch against trunk or use moz-review?
Flags: needinfo?(nical.bugzilla)
Rebased patch, also incorporates your suggestion about GetTile.
Attachment #8592816 - Attachment is obsolete: true
Attachment #8592816 - Flags: review?(jmuizelaar)
Flags: needinfo?(nical.bugzilla)
Attachment #8600897 - Flags: review?(jmuizelaar)
There was a rebasing mistake on the previous version. Also, this one is rebased on top of m-c directly without requiring another patch.
Attachment #8600897 - Attachment is obsolete: true
Attachment #8600935 - Attachment is obsolete: true
Attachment #8600897 - Flags: review?(jmuizelaar)
Attachment #8600949 - Flags: review?(jmuizelaar)
I put a version of this on reviewboard with the hope that it would make the diff better. It does not seem to have:
https://reviewboard.mozilla.org/r/8175/diff/
Comment on attachment 8600949 [details] [diff] [review]
Simplify and improve the compositor-side tiling code

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

::: gfx/layers/TiledLayerBuffer.h
@@ +201,2 @@
>    int             mRetainedWidth;  // in tiles
>    int             mRetainedHeight; // in tiles

It seems like these ints are basically an IntRect. Storing them as such will make that more obvious that they are all related and use the same units.

I'm a bit worried that mFirstTileXY makes it harder for us to have tiles start at layer origin instead of document origin. Do you have any thoughts on this?

::: gfx/layers/composite/TiledContentHost.cpp
@@ +338,5 @@
> +void
> +TiledLayerBufferComposite::Clear()
> +{
> +  for (size_t i = 0; i < mRetainedTiles.Length(); ++i) {
> +    TileHost& tile = mRetainedTiles[i];

for (TileHost &tile : mRetainedTiles) {
Comment on attachment 8600949 [details] [diff] [review]
Simplify and improve the compositor-side tiling code

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

::: gfx/layers/ipc/LayersMessages.ipdlh
@@ +314,5 @@
>  
>  struct TexturedTileDescriptor {
>    PTexture texture;
>    MaybeTexture textureOnWhite;
> +  nsIntRegion updateRegion;

I don't think we want this. I think we're probably not going to get any advantage from tracking updates in tiles on a per region basis. At most, a single rect should be enough.

I've been talking with mstange about improving the region simplification that we do in layout to basically work as a rect per tile instead of a complicated region.

Basically, I'd drop this from the patch for now and we can investigate adding it back as justified by numbers.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> ::: gfx/layers/ipc/LayersMessages.ipdlh
> @@ +314,5 @@
> >  
> >  struct TexturedTileDescriptor {
> >    PTexture texture;
> >    MaybeTexture textureOnWhite;
> > +  nsIntRegion updateRegion;
> 
> I don't think we want this. I think we're probably not going to get any
> advantage from tracking updates in tiles on a per region basis. At most, a
> single rect should be enough.

I have gone back and forth with this. I originally only shipped a rectangle, but Bas pointed out that we'd be using very large tiles with accelerated moz2d backends like D2D, and that it would be better to use regions there. I personally prefer to avoid using regions.

> 
> I've been talking with mstange about improving the region simplification
> that we do in layout to basically work as a rect per tile instead of a
> complicated region.
> 
> Basically, I'd drop this from the patch for now and we can investigate
> adding it back as justified by numbers.

Sounds good.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #15)
> 
> It seems like these ints are basically an IntRect. Storing them as such will
> make that more obvious that they are all related and use the same units.
> 
> I'm a bit worried that mFirstTileXY makes it harder for us to have tiles
> start at layer origin instead of document origin. Do you have any thoughts
> on this?
> 

I don't have thoughts on this yet, this code comes directly from TiledLayerBuffer.h. I tried to avoid changing the client-side logic in this patch, but I want to clean the client side code up eventually too so we can look into this when I get there.
Review comments addressed except the mFirstTileX/Y and mretainedWidth/Height stuff which I'll sort out when I get around cleaning the client side code up.
Attachment #8600949 - Attachment is obsolete: true
Attachment #8600949 - Flags: review?(jmuizelaar)
Attachment #8608679 - Flags: review?(nical.bugzilla)
Attachment #8608679 - Flags: review?(nical.bugzilla) → review?(jmuizelaar)
Attachment #8608679 - Flags: review?(jmuizelaar) → review+
Same for B2G Desktop. Other platforms appear to have been unaffected.
https://hg.mozilla.org/mozilla-central/rev/67efcb1efc43
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
I received a nice notification email that this patch made a 18.2% improvement in tp5_scroll on Mac. I was also told that it made a big difference on unagi. B2G is the place where I expect this to make the biggest difference.
Depends on: 1169339
This seems to cause the regression of flickering. bug 1169339
(In reply to Nicolas Silva [:nical] from comment #30)
> ...B2G is the place where I expect this to make the biggest difference.

(In reply to Sotaro Ikeda [:sotaro] from comment #31)
> This seems to cause the regression of flickering. bug 1169339

Wrong kind of a difference :)  Let's back this out if we can't fix it within a day.
FYI bug 1168015 landed on top this and needed a rebase, so backing this out will require either backing that out as well or doing more rebase work. Unless the regression breaks smoketests it might be better to give Nical a bit more time to try to fix it in place.
I am sure backing this out can wait for a few days.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: