Closed
Bug 1118876
Opened 9 years ago
Closed 9 years ago
I don't think we do edge padding with tileddrawtarget
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: jrmuizel, Assigned: nical)
References
Details
Attachments
(1 file, 3 obsolete files)
11.45 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
We probably should. This is the VisitEdges stuff in TiledContentClient
Updated•9 years ago
|
Assignee: nobody → nical.bugzilla
Assignee | ||
Comment 1•9 years ago
|
||
This version of the patch just moves the previous implementation of edge padding into a place where DrawTargetTiled can also use it (painting with DrawTargetTiled happens after ValidateTile so things need to be moved to PostValidate and operate on the front buffer rather than the back buffer since we already flipped in ValidateTile (but haven't sent the new tile buffer to the compositor yet). But I think that we can do something simpler by using the tile's invalid region. But this is blocked on the invalid region not being tracked correctly. I'll try to make this work and see which version we keep.
Assignee | ||
Comment 2•9 years ago
|
||
Previous version had an issue, this one is tested and works both with and without DrawTargetTiled.
Attachment #8555940 -
Attachment is obsolete: true
Attachment #8556471 -
Flags: review?(jmuizelaar)
Reporter | ||
Comment 3•9 years ago
|
||
Comment on attachment 8556471 [details] [diff] [review] Edge padding v1 Review of attachment 8556471 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/TiledContentClient.cpp @@ +712,5 @@ > nsIntRegion& aAddPaintedRegion, > RefPtr<TextureClient>* aBackBufferOnWhite) > { > // Try to re-use the front-buffer if possible > + if (false && mFrontBuffer && Is this intentional? @@ +1069,5 @@ > + nsIntRect unscaledTile = nsIntRect(tile.mOrigin.x, tile.mOrigin.y, > + GetTileSize().width, GetTileSize().height); > + nsIntRegion tileValidRegion = GetValidRegion(); > + nsIntRegion tilePaintRegion = aPaintRegion; > + tilePaintRegion.AndWith(unscaledTile); What's this AndWith for? @@ +1074,5 @@ > + tileValidRegion.OrWith(tilePaintRegion); > + > + // We only need to pad out if the tile has area that's not valid > + if (!tileValidRegion.Contains(unscaledTile)) { > + tileValidRegion = tileValidRegion.Intersect(unscaledTile); Seems likes the AndWith above would accomplish this already.
Attachment #8556471 -
Flags: review?(jmuizelaar) → review-
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #3) > Comment on attachment 8556471 [details] [diff] [review] > Edge padding v1 > > Review of attachment 8556471 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/client/TiledContentClient.cpp > @@ +712,5 @@ > > nsIntRegion& aAddPaintedRegion, > > RefPtr<TextureClient>* aBackBufferOnWhite) > > { > > // Try to re-use the front-buffer if possible > > + if (false && mFrontBuffer && > > Is this intentional? Oops! > > @@ +1069,5 @@ > > + nsIntRect unscaledTile = nsIntRect(tile.mOrigin.x, tile.mOrigin.y, > > + GetTileSize().width, GetTileSize().height); > > + nsIntRegion tileValidRegion = GetValidRegion(); > > + nsIntRegion tilePaintRegion = aPaintRegion; > > + tilePaintRegion.AndWith(unscaledTile); > > What's this AndWith for? This patch redoes exactly what we are currently doing except that it moves it in a place where the DrawTargetTiled path can use it. This corresponds to: https://dxr.mozilla.org/mozilla-central/source/gfx/layers/TiledLayerBuffer.h#490 There may be some calculation that we are doing but don't need to, I didn't attempt to simplify this the logic.
Assignee | ||
Comment 5•9 years ago
|
||
Removed the "if (false && [...]" mistake and the AndWidth call. Tested on the flame.
Attachment #8556471 -
Attachment is obsolete: true
Attachment #8558502 -
Flags: review?(jmuizelaar)
Reporter | ||
Updated•9 years ago
|
Attachment #8558502 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf1ae49cc11
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4bf1ae49cc11
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Reporter | ||
Comment 8•9 years ago
|
||
Backed out for Android reftest failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c6bec02690
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•9 years ago
|
||
This ranks pretty high in the "regressions that makes no sense"-scale
Assignee | ||
Comment 10•9 years ago
|
||
The idea hasn't changed but the original patch was so bitrotted that it probably warrants a new review.
Attachment #8558502 -
Attachment is obsolete: true
Attachment #8639215 -
Flags: review?(jmuizelaar)
Reporter | ||
Comment 11•9 years ago
|
||
Did you investigate using Woodrow's pad out the painted area approach?
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #11) > Did you investigate using Woodrow's pad out the painted area approach? I just made a try push with Matt's patch enabled on mobile platforms and it failed some reftests. I didn't look further because that's around the time I found what was wrong with the other approach. With this patch I can enable DrawTargetTiled on b2g today.
Flags: needinfo?(nical.bugzilla)
Reporter | ||
Comment 13•9 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #12) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #11) > > Did you investigate using Woodrow's pad out the painted area approach? > > I just made a try push with Matt's patch enabled on mobile platforms and it > failed some reftests. I didn't look further because that's around the time I > found what was wrong with the other approach. With this patch I can enable > DrawTargetTiled on b2g today. Remind me, do we have numbers showing the benefit?
Reporter | ||
Updated•9 years ago
|
Attachment #8639215 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #13) > > Remind me, do we have numbers showing the benefit? On android and mac the perf improvement was noticeable, didn't measure on b2g. More generally, getting rid of the single-paint-buffer code is a good reason to switch even if the performance turns out to not change much.
https://hg.mozilla.org/mozilla-central/rev/55df7e417c29
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: mozilla38 → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•