Closed Bug 1071769 Opened 5 years ago Closed 4 years ago

Use DrawTargetTiled on B2G

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

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

People

(Reporter: nical, Assigned: nical)

References

Details

Attachments

(5 files)

On Mac we observed that DrawTargetTiled yields better performance than the single paint buffer method (which we use on B2G and Fennec). The same is probably true for B2G and Fennec since they use software content rendering. Let's find out.
There are three test failures.

These two can be fuzzed:
layout/reftests/forms/input/range/stepDown-unthemed.html
layout/reftests/forms/input/range/stepDown.html

And this one looks very bad:
layout/reftests/position-dynamic-changes/horizontal/leftA-widthN-rightA.html?padding_abspos

It looks like your push didn't contain the patch from bug 1063046. Can you fire off another one from a more recent mozilla-central revision? Hopefully bug 1063046 will have fixed the latter test failure.
With a bit of fuzzing this try push shows up green on b2g https://tbpl.mozilla.org/?tree=Try&rev=b36bd826a511
(still need a bit of work on android, though)
Exciting stuff. \o/
Assignee: nobody → nical.bugzilla
Attachment #8502488 - Flags: review?(roc)
Attachment #8503040 - Flags: review?(bas)
Attachment #8503040 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/8ac5fed37830
https://hg.mozilla.org/mozilla-central/rev/c61c91ea40fe
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1081349
Backed out for causing bug 1081349 and a crap-ton of dupes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c77f152f280
https://hg.mozilla.org/releases/mozilla-aurora/rev/c79ad6f6a4bb
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla35 → ---
for a tile to be a placeholder, it would have to have both its front and back buffer to be null, but we'd only discard one of the two, making a placeholder look like it wasn't one.
Attachment #8538489 - Flags: review?(bas)
Comment on attachment 8538489 [details] [diff] [review]
Fix DrawTargetTiled

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

::: modules/libpref/init/all.js
@@ +3902,5 @@
>  pref("layers.draw-tile-borders", false);
>  pref("layers.draw-bigimage-borders", false);
>  pref("layers.frame-counter", false);
>  pref("layers.enable-tiles", false);
> +pref("layers.tiled-drawtarget.enabled", true);

I expected you didn't mean to have this hunk in this patch.
Attachment #8538489 - Flags: review?(bas) → review+
Attachment #8538490 - Flags: review?(bas) → review+
(In reply to Bas Schouten (:bas.schouten) from comment #15)
> Comment on attachment 8538489 [details] [diff] [review]
> Fix DrawTargetTiled
> 
> Review of attachment 8538489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libpref/init/all.js
> @@ +3902,5 @@
> >  pref("layers.draw-tile-borders", false);
> >  pref("layers.draw-bigimage-borders", false);
> >  pref("layers.frame-counter", false);
> >  pref("layers.enable-tiles", false);
> > +pref("layers.tiled-drawtarget.enabled", true);
> 
> I expected you didn't mean to have this hunk in this patch.

oops, indeed i didn't.
Unfortunately there are some new regressions in TiledDrawTarget on B2G (with and without my patches), as shown in this try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5b31edbc8ce3
A few days ago R11 and R14 were the only two issues (see https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=99955634391f ).
This was a mistake from Bug 1108164. There is no more code paths where we are supposed to pass aCanRerasterizeValidRegion=true now that per-tile drawing is gone, so let's remove it. This should fix the issue (waiting for try results)
Attachment #8540164 - Flags: review?(bas)
Attachment #8540164 - Flags: review?(bas) → review+
Backed out the enabling chunk in https://hg.mozilla.org/integration/mozilla-inbound/rev/3a86099219cb - still getting reftest-14 and -15 failures in position-dynamic-changes/, around 10% of the runs.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Use DrawTagetTiled on B2G → Use DrawTargetTiled on B2G
Depends on: 1093662
Comment on attachment 8540164 [details] [diff] [review]
We can never rerasterize the valid region

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: black artifact described in bug 1079996
[Describe test coverage new/current, TBPL]: has been baking on nightly for a week or so.
[Risks and why]: low, the affected code path is plain wrong without the patch.
[String/UUID change made/needed]:
Attachment #8540164 - Flags: approval-mozilla-aurora?
Attachment #8540164 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8540164 [details] [diff] [review]
We can never rerasterize the valid region

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

Nevermind, the bug that caused the regression which this fixes hasn't made it to beta.
Attachment #8540164 - Flags: approval-mozilla-aurora+
Depends on: 1122621
Depends on: 1118876
Depends on: 1127289
Depends on: 1129467
Are all the remaining pieces of this bug B2G only?  This comment https://bugzilla.mozilla.org/show_bug.cgi?id=1079996#c15 is suggesting that we're expecting some goodness to come to other platforms, so should we change the summary?
(In reply to Milan Sreckovic [:milan] from comment #25)
> Are all the remaining pieces of this bug B2G only?  This comment
> https://bugzilla.mozilla.org/show_bug.cgi?id=1079996#c15 is suggesting that
> we're expecting some goodness to come to other platforms, so should we
> change the summary?

This tracks the work that's needed to get DrawTargetTiled on b2g specifically, but along the way I have found some issues that, once fixed, can make things better for other platforms (Mac is the only one shipping DrawTargetTiled for now, but It may get enabled on android eventually too, and I think that the plan is to get windows xp tiling directly through DrawTargetTiled).

Bug 1071772 tracks DrawTaretTiled on android.
Right - I was interested in us mentioning this helping a Mac bug 1064236 and bug 1079996.
(In reply to Nicolas Silva [:nical] from comment #26)
> This tracks the work that's needed to get DrawTargetTiled on b2g
> specifically, but along the way I have found some issues that, once fixed,
> can make things better for other platforms (Mac is the only one shipping
> DrawTargetTiled for now, but It may get enabled on android eventually too,
> and I think that the plan is to get windows xp tiling directly through
> DrawTargetTiled).

This description makes it sound to me like we do not need to take a fix in 37. As such, I'm not tracking this bug (was suggested in bug 1079996 that referenced this one).
https://hg.mozilla.org/mozilla-central/rev/90b07ab2ae5b
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla37 → mozilla42
You need to log in before you can comment on or make changes to this bug.