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)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox38 --- fixed
firefox42 --- fixed

People

(Reporter: jrmuizel, Assigned: nical)

References

Details

Attachments

(1 file, 3 obsolete files)

We probably should.

This is the VisitEdges stuff in TiledContentClient
Assignee: nobody → nical.bugzilla
Blocks: 1071769
Attached patch Edge padding v1 (obsolete) — Splinter Review
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.
Attached patch Edge padding v1 (obsolete) — Splinter Review
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)
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-
(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.
Attached patch Edge padding v1 (updated) (obsolete) — Splinter Review
Removed the "if (false && [...]" mistake and the AndWidth call. Tested on the flame.
Attachment #8556471 - Attachment is obsolete: true
Attachment #8558502 - Flags: review?(jmuizelaar)
Attachment #8558502 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/4bf1ae49cc11
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
See Also: → 1130681
Backed out for Android reftest failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c6bec02690
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This ranks pretty high in the "regressions that makes no sense"-scale
See Also: → 1133484
Attached patch rebased patchSplinter Review
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)
Did you investigate using Woodrow's pad out the painted area approach?
Flags: needinfo?(nical.bugzilla)
(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)
(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?
Attachment #8639215 - Flags: review?(jmuizelaar) → review+
(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 ago9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla38 → mozilla42
You need to log in before you can comment on or make changes to this bug.