922942 - Remove a bunch of callers that create Thebes contexts when we have azure content

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

We need to find and fix all of these so that we can guarantee all gfxContexts are backed by Azure before deleting too much code.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Remove some no longer needed ShadowLayers code from BasicLayers

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Dont create a dummy context for BasicLayers

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Create Azure contexts for the reference surface

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't create a Thebes context for blurring

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Update BufferTextureClients using Azure

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Throw an assertion if we try create a Thebes gfxContext

I don't think I've fixed enough of the callers to land this yet, but this is the goal :)

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Avoid thebes in ClippedImage

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Avoid thebes in imgTools

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Copy to gfxImageSurface using azure

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Avoid crashes with PointInFill

The browser seems to start up and do basic things now without hitting any Thebes context.

Will push to try to see how many things it breaks.

Comment 11

[Matt Woodrow (:mattwoodrow)]

https://tbpl.mozilla.org/?tree=Try&rev=421f2b44a0eb

Comment 12

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

Comment on attachment 812962 [details] [diff] [review]
Create Azure contexts for the reference surface

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

::: gfx/thebes/gfxPlatform.cpp
@@ +542,5 @@
> +gfxPlatform::ScreenReferenceContext()
> +{
> +  nsRefPtr<gfxContext> ctx;
> +  if (mScreenReferenceDrawTarget) {
> +    ctx = new gfxContext(mScreenReferenceDrawTarget);

Hmm. After this change, all ScreenReferenceContexts share a transform and clip, which they didn't used to.

Can we assert here that the refcount for mScreenReferenceDrawTarget is 1?

Comment 13

[Matt Woodrow (:mattwoodrow)]

> Hmm. After this change, all ScreenReferenceContexts share a transform and
> clip, which they didn't used to.
> 
> Can we assert here that the refcount for mScreenReferenceDrawTarget is 1?

Ooh, good catch.

We can definitely assert that fairly easily, RefCounted has a 'hasOneRef()' function.

Seems like a fairly general problem with the thebes wrapper, I guess it's unlikely we'd hit it in other places though.

Comment 14

[Bas Schouten (:bas.schouten)]

Comment on attachment 812963 [details] [diff] [review]
Don't create a Thebes context for blurring

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

I think we should let filters land next week before we land this patch, then we can make this use hardware acceleration on Windows while we're at it. If we're in a real hurry we can land this first.

::: gfx/thebes/gfxBlur.cpp
@@ +109,4 @@
>  
>      mozilla::gfx::Rect* dirtyrect = mBlur->GetDirtyRect();
>  
> +    if (aDestinationCtx->IsCairo()) {

I can't wait for this to go away ...

::: gfx/thebes/gfxBlur.h
@@ +104,5 @@
>       * The temporary alpha surface.
>       */
>      nsRefPtr<gfxImageSurface> mImageSurface;
>  
> +    nsAutoArrayPtr<unsigned char> mData;

Can we use STL instead of ns*, I believe that's the desired thing to do these days :)

Comment 15

[Nicolas Silva [:nical]]

Comment on attachment 812964 [details] [diff] [review]
Update BufferTextureClients using Azure

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

::: gfx/layers/ImageDataSerializer.h
@@ +40,3 @@
>  
>  protected:
> +  uint32_t GetStride() const;

This doesn't need to be a protected method

Comment 16

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 812970 [details] [diff] [review]
Avoid thebes in ClippedImage

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

Looks good!

I'm glad you brought this to my attention. I was preparing to land bug 764299 in the near future which included code based on this snippet from ClippedImage. I've updated the patch in bug 764299 accordingly.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 812971 [details] [diff] [review]
Avoid thebes in imgTools

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

::: image/src/imgTools.cpp
@@ +29,2 @@
>  using namespace mozilla::image;
> +using namespace mozilla::gfx;

Not a big fan of "using namespace"; it'd be better if you didn't add more. I'd prefer that you use individual using directives for specific identifiers, e.g. "using mozilla::RefPtr". Looks like you probably only need 3.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 812972 [details] [diff] [review]
Copy to gfxImageSurface using azure

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

::: gfx/thebes/gfxASurface.cpp
@@ +53,5 @@
>  #include "nsString.h"
>  #include "nsIClipboardHelper.h"
>  
>  using namespace mozilla;
> +using namespace mozilla::gfx;

Again, I suggest avoiding "using namespace".

Comment 19

[Matt Woodrow (:mattwoodrow)]

Attachment: Use Moz2D for android snapshots

Comment 20

[Matt Woodrow (:mattwoodrow)]

Attachment: Use Moz2D for canvas snapshots

Comment 21

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

(In reply to Seth Fowler [:seth] from comment #17)
> Comment on attachment 812971 [details] [diff] [review]
> Avoid thebes in imgTools
> 
> Review of attachment 812971 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/imgTools.cpp
> @@ +29,2 @@
> >  using namespace mozilla::image;
> > +using namespace mozilla::gfx;
> 
> Not a big fan of "using namespace"; it'd be better if you didn't add more.
> I'd prefer that you use individual using directives for specific
> identifiers, e.g. "using mozilla::RefPtr". Looks like you probably only need
> 3.

Nooooo! For years I've been telling people the exact opposite!

Comment 22

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

Here's a dev-platform thread from about a year ago where we decided "using namespace" was A-OK:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/L809txbobEU

Comment 23

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdcaeede6626
https://hg.mozilla.org/integration/mozilla-inbound/rev/51dda2544c24
https://hg.mozilla.org/integration/mozilla-inbound/rev/5133ba07ea89
https://hg.mozilla.org/integration/mozilla-inbound/rev/a92cf0787b53

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cdcaeede6626
https://hg.mozilla.org/mozilla-central/rev/51dda2544c24
https://hg.mozilla.org/mozilla-central/rev/5133ba07ea89
https://hg.mozilla.org/mozilla-central/rev/a92cf0787b53

Comment 25

[Seth Fowler [:seth] [:s2h]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Here's a dev-platform thread from about a year ago where we decided "using
> namespace" was A-OK:
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/L809txbobEU

I find this conclusion questionable, but it's not really an important matter.

Comment 26

[Matt Woodrow (:mattwoodrow)]

Attachment: Avoid thebes in imgTools v2

Moz2D only supports EXTEND_PAD for drawing, whereas thebes supported (and defaulted to) EXTEND_NONE.

This difference results in very slightly different results when scaling images.

This patch changes the reference image to one that was generated using EXTEND_PAD, and change the Thebes path to explicitly use EXTEND_PAD. I've checked that we pass all imgTools tests using both paths.

Comment 27

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 817524 [details] [diff] [review]
Avoid thebes in imgTools v2

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

Per our discussion on IRC, looks good.

Comment 28

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32bb598c8a82
https://hg.mozilla.org/integration/mozilla-inbound/rev/95427fe5f9b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e5db20b655
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1881913d9a6

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out c0e5db20b655 in https://hg.mozilla.org/integration/mozilla-inbound/rev/da01cc64591f for causing the xpcshell bustage in https://tbpl.mozilla.org/php/getParsedLog.php?id=29164178&tree=Mozilla-Inbound

Comment 30

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53c610770178

Had to update a couple more of the reference images.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32bb598c8a82
https://hg.mozilla.org/mozilla-central/rev/95427fe5f9b9
https://hg.mozilla.org/mozilla-central/rev/d1881913d9a6
https://hg.mozilla.org/mozilla-central/rev/53c610770178

Comment 32

[Matt Woodrow (:mattwoodrow)]

Attachment: Use the screen reference draw target in SVG

Comment 33

[Matt Woodrow (:mattwoodrow)]

Attachment: Avoid crashes with PointInFill v2

Updated to avoid rects with inf values for cg and d2d.

Comment 34

[Matt Woodrow (:mattwoodrow)]

Attachment: Change SVG test to not use overlapping strokes

As discussed at the summit, this behaviour isn't supported correctly for most of our backends. Cairo gets the hit testing part right, but usually draws wrong, most other backends get it wrong for both.

I don't think this behaviour is really relevant to the actual test, so changing to avoid it.

Comment 35

[Matt Woodrow (:mattwoodrow)]

Attachment: Add fuzzy comparisons to a couple of tests since D2D needs it

Comment 36

[Jonathan Watt [:jwatt]]

Comment on attachment 826201 [details] [diff] [review]
Add fuzzy comparisons to a couple of tests since D2D needs it

The fact that D2D comes up with fractional results for pixel aligned circles and rects that have non-fractional x/y/w/h/cx/cy/r values is disappointing. There's no stroke involved here, so I'm not sure how that can happen. If we can't do better somehow we're going to get more than a few complaints about getBBox returning off results for sure.

One option to mitigate this issue might could be to implement a version of GetBBoxContribution that doesn't rely on PathBuilder to calculate bounds for simple shapes like <rect> and <circle>. (It'd be more efficient anyway.) That won't help <polyline>, <polygon> or <path> where the positioning is pixel aligned and with non-fractional values, but it would get the cases where content authors are most likely to notice.)

Comment 37

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

Comment on attachment 826201 [details] [diff] [review]
Add fuzzy comparisons to a couple of tests since D2D needs it

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

I agree with what Jonathan said, though.

Comment 38

[Jonathan Watt [:jwatt]]

Comment on attachment 826200 [details] [diff] [review]
Change SVG test to not use overlapping strokes

This is less important than the bbox issue by a long shot, so I guess this is fine too. The same mitigation idea described in comment 36 would also work for this.

Comment 39

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/effddcf29fa7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a3b01d520ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/993223b5bbe7
https://hg.mozilla.org/integration/mozilla-inbound/rev/a60b5eb6b8ba

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/effddcf29fa7
https://hg.mozilla.org/mozilla-central/rev/3a3b01d520ac
https://hg.mozilla.org/mozilla-central/rev/993223b5bbe7
https://hg.mozilla.org/mozilla-central/rev/a60b5eb6b8ba

Comment 41

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

dev.tree-management (https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/8caTImH3_9o) is showing a svg regressions on most platforms for svg-asap and svg-opacity.  A whole set of related patches landed at the same time, this bug seems to reference SVG changes.

Win 7 svg-asap:
https://datazilla.mozilla.org/?start=1382979351&stop=1383584151&product=Firefox&repository=Mozilla-Inbound-Non-PGO&os=win&os_version=6.1.7601&test=tsvgx&tr_id=3438927&graph=win%206.2.9200&x86=true&x86_64=false&error_bars=false&project=talos

win 7 svg opacity:
https://datazilla.mozilla.org/?start=1382979351&stop=1383584151&product=Firefox&repository=Mozilla-Inbound-Non-PGO&os=win&os_version=6.1.7601&test=tsvgr_opacity&tr_id=3438927&graph=win%206.2.9200&x86=true&x86_64=false&error_bars=false&project=talos

other platforms have a similar problem (winxp, linux)

I would like to find a fix for this since we are 10-25% regressed from previously.

Comment 42

[Matt Woodrow (:mattwoodrow)]

(In reply to Joel Maher (:jmaher) from comment #41)
> dev.tree-management
> (https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/
> 8caTImH3_9o) is showing a svg regressions on most platforms for svg-asap and
> svg-opacity.  A whole set of related patches landed at the same time, this
> bug seems to reference SVG changes.
> 
> Win 7 svg-asap:
> https://datazilla.mozilla.org/
> ?start=1382979351&stop=1383584151&product=Firefox&repository=Mozilla-Inbound-
> Non-PGO&os=win&os_version=6.1.7601&test=tsvgx&tr_id=3438927&graph=win%206.2.
> 9200&x86=true&x86_64=false&error_bars=false&project=talos
> 
> win 7 svg opacity:
> https://datazilla.mozilla.org/
> ?start=1382979351&stop=1383584151&product=Firefox&repository=Mozilla-Inbound-
> Non-PGO&os=win&os_version=6.1.
> 7601&test=tsvgr_opacity&tr_id=3438927&graph=win%206.2.
> 9200&x86=true&x86_64=false&error_bars=false&project=talos
> 
> other platforms have a similar problem (winxp, linux)
> 
> I would like to find a fix for this since we are 10-25% regressed from
> previously.

This is really unexpected, I'll take a look at this today.

Comment 43

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

here is the main wiki for running talos locally:
https://wiki.mozilla.org/Buildbot/Talos/Running

if you want to use try server, that will get you testing on the same hardware and infrastructure, but not as fast as running locally.  

ask for help if you need any, I would be happy to pitch in.

Comment 44

[Matt Woodrow (:mattwoodrow)]

That was fun, talos required mozinfo==0.4, and mozinstall requires mozinfo>=0.7. Fighting ensued.

Anyway, this is because we're spending a lot more time doing Path coordinate space conversions trying to match the thebes API with moz2d.

In particular we're spending about 12% of the last sub-test in tsvgx within TransformedCopyToBuilder.

This should all go away once we convert this code to use moz2d paths directly, which I believe jwatt has patches for.

Jonathan: Are those patches close to being landable?

Comment 45

[Jonathan Watt [:jwatt]]

I have an in-progress patch in bug 934183. I'd really like some Moz2D API decisions to be made by Moz2D people in bug 934182 and bug 934110 (particularly the former) to know how I'm proceeding there though. With that, I should definitely be able to land that work well before the uplift.

Comment 46

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

I have filed bug 935008 to track this.  Thanks for taking a few minutes to look into this and track down the regression.

Comment 47

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Return infinite, rather than empty bounds for infinite path

Comment 48

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 830332 [details] [diff] [review]
Return infinite, rather than empty bounds for infinite path

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

Does this pass all tests? I changed these to return empty because it was required behaviour for our test suite iirc.

Comment 49

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 830332 [details] [diff] [review]
Return infinite, rather than empty bounds for infinite path

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

Does this pass all tests? I changed these to return empty because it was required behaviour for our test suite iirc.

Comment 50

[Bas Schouten (:bas.schouten)]

Comment on attachment 812963 [details] [diff] [review]
Don't create a Thebes context for blurring

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

See previous comment.

Comment 51

[Matt Woodrow (:mattwoodrow)]

I think we've done enough for this bug, closing and leaving bug 933019 as the tracking bug for this work.