989858 - Convert BasicLayers to Moz2D

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add Moz2D helpers to BasicLayersImpl

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Convert BasicCanvasLayer

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Convert BasicImageLayer

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Convert BasicColorLayer

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Remove partially implemented moz2d paint function

I don't know why this was added, partially implemented and never used. It didn't have a mask layer transform parameter, so it couldn't be correct anyway.

My patches replace this entirely, so it can go away.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Rename DeprecatedPaint and stop passing a gfxContext in

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8399227 [details] [diff] [review]
Part 1: Add Moz2D helpers to BasicLayersImpl

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

::: gfx/layers/basic/BasicImplData.h
@@ +121,5 @@
>     */
>    virtual bool GetAsSurface(gfxASurface** aSurface,
>                              SurfaceDescriptor* aDescriptor)
>    { return false; }
> +  virtual bool GetAsSurface(RefPtr<gfx::SourceSurface>& aSurface) { return false; }

Why not just return a TemporaryRef<gfx::SourceSurface> directly, returning null on failure?

::: gfx/layers/basic/BasicLayersImpl.cpp
@@ +84,5 @@
> +
> +bool
> +AutoMoz2DMaskData::IsConstructed()
> +{
> +  return !!mSurface;

Put all these methods inline in the class.

Comment 8

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71db5845ea4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/25306d89bded
https://hg.mozilla.org/integration/mozilla-inbound/rev/433b95e605bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/696938af99e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad933a27ebb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f94c69dfca91

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71db5845ea4d
https://hg.mozilla.org/mozilla-central/rev/25306d89bded
https://hg.mozilla.org/mozilla-central/rev/433b95e605bf
https://hg.mozilla.org/mozilla-central/rev/696938af99e5
https://hg.mozilla.org/mozilla-central/rev/ad933a27ebb5
https://hg.mozilla.org/mozilla-central/rev/f94c69dfca91

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

it appears that the canvasmark benchmark on linux (I suspect linux64, winxp, and win7 also) has regressed about 2.3% as a side effect of this patch set:
http://graphs.mozilla.org/graph.html#tests=[[297,131,33],[297,131,35],[297,131,37],[297,131,25]]&sel=1395770878537,1396375678537&displayrange=7&datatype=running

Here is more about canvasmark:
https://wiki.mozilla.org/Buildbot/Talos/Tests#CanvasMark

datazilla doesn't seem to have data for the last day, so i am not sure which subtest is the problem.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

here is a reference to some retriggers I did:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Ubuntu%20HW%2012.04%20mozilla-inbound%20talos%20chromez&fromchange=b4edb59d3db5&tochange=ecda58bb90a7

you can see prior to this push we were in the 6500 range, and after this push in the 6350 range.

Comment 12

[Firefox Commit Bot]

The following changesets are now in Firefox Nightly:

> 71db5845ea4d Bug 989858 - Part 1: Add Moz2D helpers to BasicLayersImpl. r=roc
> 25306d89bded Bug 989858 - Part 2: Convert BasicCanvasLayer. r=roc
> 433b95e605bf Bug 989858 - Part 3: Convert BasicImageLayer. r=roc
> 696938af99e5 Bug 989858 - Part 4: Convert BasicColorLayer. r=roc
> ad933a27ebb5 Bug 989858 - Part 5: Remove partially implemented moz2d paint function. r=roc
> f94c69dfca91 Bug 989858 - Part 6: Rename DeprecatedPaint and stop passing a gfxContext in. r=roc

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

as per comment 10

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

I wanted to make sure we were looking into the talos regression, we will be uplifting this code in about 2 weeks.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

this is on aurora- we still see the regression on linux32

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Joel, are you still considering an uplift of this to 31 or the problem is still unfixed? thanks

Comment 17

[Avi Halachmi (:avih)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> Joel, are you still considering an uplift of this to 31 or the problem is
> still unfixed? thanks

I haven't been following this, but if this 2.3% regression is the only thing standing between us and uplifting, then it shouldn't stop us from uplifting IMO.

Moz2D is new and important enough and some regressions are expected, especially when it brings other improvements elsewhere which are not mentioned here because we only track regressions and not improvements.

On top of this, we're not blocking on perf regressions unless they're really bad (and this isn't), and the threshold over which we file regression bugs was decided to be too low, and therefore starting next week we'll only be filing regression bugs for regressions over ~4% rather than over 2%.

To summarize, this regression shouldn't stop us from doing anything we think we should.

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

this regressions still exists.  It is disappointing when we spend resources for every changeset that detect a regression and there is no real comment about why we should accept the regression or what work is being done to accept it.  Now that this is going on 10 weeks and a couple uplifts, we really are starting to run out of time.

On Aurora and Inbound the regression exists and for the most part no other canvasmark regressions have been seen.  This is a <3% regression (on multiple platforms), so there isn't much we can do.

Comment 19

[Avi Halachmi (:avih)]

(In reply to Joel Maher (:jmaher) from comment #18)
> ... It is disappointing when we spend resources
> for every changeset that detect a regression and there is no real comment
> about why we should accept the regression or what work is being done to
> accept it.

IMO it shouldn't be disappointing. The effort to reliably detect regressions is a useful one also without devs justifying or commenting in general on every 2% (or 20%) regression.

It allows us to track values over time, it gives feedback to devs and managers on where priorities might need to be, and, in case management or a group or a dev feels that it should be addressed, provide feedback while making changes to the code.

I'm not saying that commenting on a regression completely lacks value, but when the regression is small, the ground shifts much all over (as with OMTC or Moz2D, etc), taking the time and focus to be able to provide a meaningful explanation or justification for a 2% regression will just cost more than it gives back.

Besides, I believe that if anyone thought it's worth a comment or that they have anything useful to say here, you'd have seen a comment by now, as we've seen already on many other cases.

Comment 20

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #17)
> ... we're not blocking on perf regressions unless they're really bad.

That's actually a lie. We don't block on perf regressions, period.

We can help in understanding the numbers better, or help in evaluating the importance of a specific regression, but the decision if it's a blocker or not is affected by so many factors (product, marketing, deadlines, etc, including performance/regressions in other areas of the product or tradeoff) that unless it's a disastrous one to a degree that it will block itself by whomever owns the code, we can't claim that some X% regression is worth blocking just because it exists or is above some Y threshold.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

This bug will probably requires a refactoring which would not be accepted in 31. Untracking it.

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

a larger issue on linxu32/linux64 and a minor issue on winxp:
http://graphs.mozilla.org/graph.html#tests=[[297,53,33],[297,53,35],[297,53,37]]&sel=1397234854073,1405010854073&displayrange=90&datatype=running

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

this is almost 3 months old, a lot has changed, can we close this bug?  is there work we should be tracking?

Comment 24

[Nicolas Silva [:nical]]

yes \o/

Comment 25

[Jonathan Watt [:jwatt]]

Part 6 replaced a call to gfxContext::UserToDevicePixelSnapped() with a call to the new Moz2D helper of the same name. These aren't equivalent though since the Moz2D version doesn't (isn't able to) check for the presence of the FLAG_DISABLE_SNAPPING flag. Should we just stop using FLAG_DISABLE_SNAPPING (it's only used for printing, and it's comments suggest that's only to skip seam avoidance work that isn't necessary for printing), or do we need to replace the gfxContext FLAG_DISABLE_SNAPPING flag with a UserData entry on the DrawTarget (more expensive to look-up than a flag with good cache locality). If you don't know the answer, Matt, do you know who would?

Comment 26

[Matt Woodrow (:mattwoodrow)]

(In reply to Jonathan Watt [:jwatt] (away until Tuesday 14th) from comment #25)
> Part 6 replaced a call to gfxContext::UserToDevicePixelSnapped() with a call
> to the new Moz2D helper of the same name. These aren't equivalent though
> since the Moz2D version doesn't (isn't able to) check for the presence of
> the FLAG_DISABLE_SNAPPING flag. Should we just stop using
> FLAG_DISABLE_SNAPPING (it's only used for printing, and it's comments
> suggest that's only to skip seam avoidance work that isn't necessary for
> printing), or do we need to replace the gfxContext FLAG_DISABLE_SNAPPING
> flag with a UserData entry on the DrawTarget (more expensive to look-up than
> a flag with good cache locality). If you don't know the answer, Matt, do you
> know who would?

I think we probably still do need it, though the lack of complaints would suggest otherwise. I think roc would be the best person to answer this though.

We don't check the flags *that* often, making it user data should be ok. Otherwise we can just add a flag to DrawTarget (SetSubpixelAAEnabled is basically that).

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> I think we probably still do need it, though the lack of complaints would
> suggest otherwise. I think roc would be the best person to answer this
> though.
> 
> We don't check the flags *that* often, making it user data should be ok.
> Otherwise we can just add a flag to DrawTarget (SetSubpixelAAEnabled is
> basically that).

I think we should keep on not snapping for printing. It artificially reduces the accuracy of the printed output.

Comment 28

[Jonathan Watt [:jwatt]]

I've put patches up in bug 1082530.