764125 - [Azure] Get Azure/Cairo canvas passing tests

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fixes for 2d.toDataURL.*

The attached patch should fix the 2d.toDataURL.* by implementing DrawTargetCairo::CopySurface()

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Implemented DrawTargetCairo::CopySurface to fix 2d.toDataURL.*

Two similarly named patches. Let's try for the right one this time.

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Implemented DrawTargetCairo::CopySurface to fix 2d.toDataURL.*

Updated patch.

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fixed surface edges on Linux to make 2d.drawImage.5arg pass

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Possible logic error affecting Cairo win32

Comment 6

[Nick Cameron [:nrc]]

Attachment: patch 1: Add DrawTarget arg to GetScaledFontForFont

Comment 7

[Nick Cameron [:nrc]]

Attachment: patch 2: small fix

Comment 8

[Nick Cameron [:nrc]]

Attachment: patch 1: Add DrawTarget arg to GetScaledFontForFont

Comment 9

[Nick Cameron [:nrc]]

Attachment: patch 3: pass around the size of a gfxASurface when creating a DrawTarget

Comment 10

[Nick Cameron [:nrc]]

Attachment: patch 4: store a reference to the backing surface in DrawTargetCairo

Comment 11

[Nick Cameron [:nrc]]

Attachment: patch 5: drawing changes to DrawTargetCairo

Comment 12

[Nick Cameron [:nrc]]

Attachment: patch 6: Fix a bug where a path with a transform is re-used

Comment 13

[Nick Cameron [:nrc]]

Attachment: patch 7: changes to nsCanvasRenderingContext2DAzure

Comment 14

[Bas Schouten (:bas.schouten)]

Comment on attachment 636215 [details] [diff] [review]
patch 3: pass around the size of a gfxASurface when creating a DrawTarget

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

How will this work for Cairo surfaces that don't have a finite size? (i.e. PDF and such)

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: patch 1.5: implement DrawTargetCairo::CopySurface

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: patch 8: Fix canvas 2d.drawImage.5arg and 2d.drawImage.9arg surface scaling tests

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: patch 9: fixed crash test fail on gtk caused by null

Comment 18

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: patch 10: Fixed build break for qt

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 637006 [details] [diff] [review]
patch 8: Fix canvas 2d.drawImage.5arg and 2d.drawImage.9arg surface scaling tests

> Bug 764125; Fix canvas 2d.drawImage.5arg and 2d.drawImage.9arg surface scaling tests. r=karl

The comment should describe how the patch changes behavior.
e.g. "switch from EXTEND_NONE to EXTEND_PAD for drawing surfaces to avoid sampling beyond the surface"

Comment 20

[Bas Schouten (:bas.schouten)]

Comment on attachment 637004 [details] [diff] [review]
patch 1.5: implement DrawTargetCairo::CopySurface

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +473,5 @@
>    AutoPrepareForDrawing prep(this, mContext);
> +
> +  cairo_matrix_t src_mat;
> +  cairo_matrix_init_scale(&src_mat, 1, 1);
> +  cairo_matrix_translate(&src_mat, aSource.X(), aSource.Y());

nit: It's easier to use the members directly rather than the getters (i.e. aSource.x, aSource.y)

@@ +480,5 @@
> +  if (aSurface->GetType() == SURFACE_CAIRO) {
> +    surf = static_cast<SourceSurfaceCairo*>(aSurface)->GetSurface();
> +  }
> +
> +  cairo_pattern_t* pat = cairo_pattern_create_for_surface(surf);

What will this do with a NULL surface? Should we have an else { gfxWarning() above?

@@ +482,5 @@
> +  }
> +
> +  cairo_pattern_t* pat = cairo_pattern_create_for_surface(surf);
> +  cairo_pattern_set_matrix(pat, &src_mat);
> +  cairo_pattern_set_filter(pat, CAIRO_FILTER_BILINEAR);

We can use point filtering as there's never any resizing going on here. I also believe cairo has a set_source_surface that can directly set a source surface with an offset. That should make this function a lot simpler.

What you'd do is totally ignore matrices or anything (except set the identity matrix on the context), i.e. as far as I can see something like this would work (peudo-code):

cairo_save(mContext);

cairo_identity_matrix(mContext);

cairo_set_source_surface(mContext, surf, aDest - aSource);
cairo_set_operator(mContext, SOURCE);

cairo_reset_clip(mContext);
cairo_new_path(mContext);
cairo_rectangle(aDest.x, aDest.y, aSource.width, aSource.height);
cairo_clip(mContext);

cairo_paint(mContext);

cairo_restore(mContext);

@@ +498,5 @@
> +  cairo_clip(mContext);
> +  cairo_set_source(mContext, pat);
> +
> +  cairo_set_operator(mContext, CAIRO_OPERATOR_SOURCE);
> +  cairo_paint_with_alpha(mContext, 1.0F);

We should just be able to use cairo_paint();

Comment 21

[Bas Schouten (:bas.schouten)]

Comment on attachment 637007 [details] [diff] [review]
patch 9: fixed crash test fail on gtk caused by null

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

::: gfx/thebes/gfxPlatform.cpp
@@ +633,5 @@
>    if (backend == BACKEND_CAIRO) {
>      nsRefPtr<gfxASurface> surf = CreateOffscreenSurface(ThebesIntSize(aSize),
>                                                          ContentForFormat(aFormat));
> +    if (!surf)
> +      return NULL;

nit: { } also for single line ifs.

Comment 22

[Bas Schouten (:bas.schouten)]

Comment on attachment 636216 [details] [diff] [review]
patch 4: store a reference to the backing surface in DrawTargetCairo

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

::: gfx/2d/SourceSurfaceCairo.cpp
@@ +105,5 @@
>      cairo_destroy(ctx);
>  
>      // Swap in this new surface.
>      cairo_surface_destroy(mSurface);
> +    cairo_pattern_destroy(pat);

This is actually fixing a leak as far as I can see. It should technically be in a separate patch.

Comment 23

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: patch 1.5: implement DrawTargetCairo::CopySurface [v2]

I've made the suggested changes in comment 20 and now the patch looks suspiciously like the given pseudo-code.

Comment 24

[Bas Schouten (:bas.schouten)]

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #23)
> Created attachment 638865 [details] [diff] [review]
> patch 1.5: implement DrawTargetCairo::CopySurface [v2]
> 
> I've made the suggested changes in comment 20 and now the patch looks
> suspiciously like the given pseudo-code.

I assume you checked if it worked? ;)

Comment 25

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Bas Schouten (:bas) from comment #24)
> I assume you checked if it worked? ;)

Your assumption is correct.

Comment 26

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 633023 [details] [diff] [review]
Possible logic error affecting Cairo win32

I haven't been able to figure out the problem fixed by this patch (possible logic error...). It seems it will not fail gracefully if it hits a later error condition. Anyway the logic appears inverted.

Comment 27

[Nick Cameron [:nrc]]

Attachment: patch 9.5 test tweaks

Comment 28

[Nick Cameron [:nrc]]

Attachment: patch 5: drawing changes to DrawTargetCairo

Comment 29

[Nick Cameron [:nrc]]

Comment on attachment 639196 [details] [diff] [review]
patch 9.5 test tweaks

Looks like Joe is away, so I've asked Bas for reviews on these two patches too, not sure if there is a better person for the Canvas stuff, but at least Bas has seen the other patches.

Comment 30

[Bas Schouten (:bas.schouten)]

Comment on attachment 636219 [details] [diff] [review]
patch 7: changes to nsCanvasRenderingContext2DAzure

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

Please explain why we need the mNewPath stuff, it's not clear to me.

Comment 31

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas) from comment #30)
> Comment on attachment 636219 [details] [diff] [review]
> patch 7: changes to nsCanvasRenderingContext2DAzure
> 
> Review of attachment 636219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please explain why we need the mNewPath stuff, it's not clear to me.

It was to meet the spec for quadratic curves (tests: http://philip.html5.org/tests/canvas/suite/tests/index.2d.path.quadraticCurveTo.ensuresubpath.html) - if there is no existing path then the first control point is used as the start of the path.

Comment 32

[Nick Cameron [:nrc]]

Attachment: patch 7: changes to nsCanvasRenderingContext2DAzure

rebased and fixed a bug

Comment 33

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 637009 [details] [diff] [review]
patch 10: Fixed build break for qt

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

jfkthame is better suited to review this I think.

::: gfx/thebes/gfxQtPlatform.cpp
@@ +514,3 @@
>      return gPlatformFTLibrary;
> +#else
> +    return nsnull;

Jonathan Kew wasn't too fond of this. Maybe we can NS_ASSERTION that we're not using Pango here?

@@ +518,3 @@
>  }
>  
> +#ifndef MOZ_PANGO

Where is the matching endif for this? Why is this necessary?

Comment 34

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 638865 [details] [diff] [review]
patch 1.5: implement DrawTargetCairo::CopySurface [v2]

It seems like this should use cairo_fill instead of clip and paint. Otherwise this looks ok.

Comment 35

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nick Cameron [:nrc] from comment #31)
> (In reply to Bas Schouten (:bas) from comment #30)
> > Comment on attachment 636219 [details] [diff] [review]
> > patch 7: changes to nsCanvasRenderingContext2DAzure
> > 
> > Review of attachment 636219 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please explain why we need the mNewPath stuff, it's not clear to me.
> 
> It was to meet the spec for quadratic curves (tests:
> http://philip.html5.org/tests/canvas/suite/tests/index.2d.path.
> quadraticCurveTo.ensuresubpath.html) - if there is no existing path then the
> first control point is used as the start of the path.

Does this affect other backends then? If so it would probably be better in another bug.

Comment 36

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 636215 [details] [diff] [review]
patch 3: pass around the size of a gfxASurface when creating a DrawTarget

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

This needs Bas's comment about cairo surfaces not having sizes addressed

Comment 37

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: patch 1.5: implement DrawTargetCairo::CopySurface [v3]

cairo_fill instead of cairo_clip and cairo_paint

Comment 38

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 639196 [details] [diff] [review]
patch 9.5 test tweaks

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

Do we understand the cause of these regressions? i.e. why it worked before with cairo but not with azure? The comment near the test talks about cairo limitations but isn't clear about what limitations and why we hit them now and not before.

Comment 39

[Nick Cameron [:nrc]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #35)
> (In reply to Nick Cameron [:nrc] from comment #31)
> > (In reply to Bas Schouten (:bas) from comment #30)
> > > Comment on attachment 636219 [details] [diff] [review]
> > > patch 7: changes to nsCanvasRenderingContext2DAzure
> > > 
> > > Review of attachment 636219 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Please explain why we need the mNewPath stuff, it's not clear to me.
> > 
> > It was to meet the spec for quadratic curves (tests:
> > http://philip.html5.org/tests/canvas/suite/tests/index.2d.path.
> > quadraticCurveTo.ensuresubpath.html) - if there is no existing path then the
> > first control point is used as the start of the path.
> 
> Does this affect other backends then? If so it would probably be better in
> another bug.

It does not seem to affect D2D, I do not know why, presumably because it is taken into account in the DrawTarget somehow. I don't know about other backends. It might be possible to move the logiv into the DrawTarget, but that seemed much more difficult. I believe this test fails for Thebes/Cairo canvas anyway, so might not be necessary to pass immediately.

Comment 40

[Nick Cameron [:nrc]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #36)
> Comment on attachment 636215 [details] [diff] [review]
> patch 3: pass around the size of a gfxASurface when creating a DrawTarget
> 
> Review of attachment 636215 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This needs Bas's comment about cairo surfaces not having sizes addressed

We discussed this on IRC, and iirc, we are not able to deal with size-less surfaces at the moment and it there is no easy way to sort this, so it didn't need addressing, but Bas was going to have a think about this before confirming.

Comment 41

[Nick Cameron [:nrc]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #38)
> Comment on attachment 639196 [details] [diff] [review]
> patch 9.5 test tweaks
> 
> Review of attachment 639196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we understand the cause of these regressions? i.e. why it worked before
> with cairo but not with azure? The comment near the test talks about cairo
> limitations but isn't clear about what limitations and why we hit them now
> and not before.

The limitation is that Cairo draws arcs by stroking perpendicular to the arc, and at very large stroke thicknesses, this becomes a fan. Where exactly the 'blades' of the fan appear seems to depend on exactly how the arc is defined and the platform. So if the blades of the fan are where pixels are tested it passes the test, if the testing pixels fall in between the blades, then we fail. Before, we were rendering wrong, but got lucky with the test, now we are not so lucky.

Comment 42

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 639197 [details] [diff] [review]
patch 5: drawing changes to DrawTargetCairo

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +220,5 @@
>    return pat;
>  }
>  
>  static bool
> +OperatorAffectsUncoveredAreas(CompositionOp op)

Please copy the comment from nsCanvasRenderingContext2D.cpp as well

@@ +327,5 @@
> +      cairo_new_path(mContext);
> +      cairo_rectangle(mContext, 0, 0, aDest.Width(), aDest.Height());
> +      cairo_clip(mContext);
> +      cairo_set_source(mContext, pat);
> +      cairo_paint(mContext);

We should be able to use fill() instead of clip() and paint()

@@ +418,5 @@
> +      cairo_new_path(mContext);
> +      cairo_rectangle(mContext, 0, 0, width, height);
> +      cairo_clip(mContext);
> +      cairo_set_source(mContext, pat);
> +      cairo_paint(mContext);

Same here. fill() instead of clip() and paint()

@@ +727,5 @@
> +  unsigned char* surfData = cairo_image_surface_get_data(surf);
> +  const int32_t pixelWidth = BytesPerPixel(aFormat);
> +  for (size_t y = 0; y < aSize.height; ++y) {
> +    memcpy(surfData + y*aSize.width*pixelWidth, aData + y*aStride, aSize.width*pixelWidth);
> +  }

I think it would probably be helpful to add a helper method that does this strided copy. We could use it the CoreGraphics backend too.

Comment 43

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 639196 [details] [diff] [review]
patch 9.5 test tweaks

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

It would be nice to know more about bug 764108 before pushing this, but I guess it doesn't need to block.

::: content/canvas/test/test_canvas.html
@@ +11309,5 @@
>  isPixel(ctx, 1,48, 0,255,0,255, 0);
> +// Fails on Linux with Azure/Cairo only
> +// The arc is drawn badly due to Cairo limitations, the error only becomes
> +// apparent on Linux because of anti-aliasing, probably due to X
> +// Bug 764125

Move the description you gave into this comment to make it more useful. Also we should only disable the test when using Azure/Cairo

Comment 44

[Nick Cameron [:nrc]]

Attachment: patch 9.5 test tweaks

Added comment, carrying r=jrmuizel

Comment 45

[Nick Cameron [:nrc]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #43)

> Move the description you gave into this comment to make it more useful. Also
> we should only disable the test when using Azure/Cairo

I added the comment, but the test only fails on Linux, it passes on other platforms. We do not have the facility for to mark a test passing on specific platforms, the policy elsewhere in the file seems to be to comment out tests which are platform-specific pass/fail.

Comment 46

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Nick Cameron [:nrc] from comment #45)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #43)
> 
> > Move the description you gave into this comment to make it more useful. Also
> > we should only disable the test when using Azure/Cairo
> 
> I added the comment, but the test only fails on Linux, it passes on other
> platforms. We do not have the facility for to mark a test passing on
> specific platforms, the policy elsewhere in the file seems to be to comment
> out tests which are platform-specific pass/fail.

You can, and you should. I object to commenting out any test.

Comment 47

[Nick Cameron [:nrc]]

Attachment: patch 9.5 test tweaks

with a conditional test, rather than commenting it out

Comment 48

[Nick Cameron [:nrc]]

(In reply to :Ms2ger from comment #46)
> (In reply to Nick Cameron [:nrc] from comment #45)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #43)
> > 
> > > Move the description you gave into this comment to make it more useful. Also
> > > we should only disable the test when using Azure/Cairo
> > 
> > I added the comment, but the test only fails on Linux, it passes on other
> > platforms. We do not have the facility for to mark a test passing on
> > specific platforms, the policy elsewhere in the file seems to be to comment
> > out tests which are platform-specific pass/fail.
> 
> You can, and you should. I object to commenting out any test.

OK, we can (very small bit of extra code, it turns out), but in this case I think we probably shouldn't, because whether we pass or not is basically non-deterministic, and we have established that we don't care (at the moment) whether we pass or not. I believe that is expressed by commenting out the test (we don't delete it because we might change our mind in the future). Making the test conditional says that it passes on one platform and fails on another, and we expect it to remain that way. It is easy to imagine some change somewhere which changes this situation and then a conditional test would be confusing.

Anyway, I have put up alternative versions of the patch, I can land whichever.

Comment 49

[Jonathan Kew [:jfkthame]]

Comment on attachment 637009 [details] [diff] [review]
patch 10: Fixed build break for qt

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

I'd like to know what makes this appear necessary; is there something in one of the other patches here that's requiring it?

::: gfx/thebes/gfxQtPlatform.cpp
@@ +518,1 @@
>  }

I'm not clear why we need to do this - what purpose does it serve (in the MOZ_PANGO case) if it's just going to return null?

If we do have to provide this as a stub in the MOZ_PANGO case, we should assert NS_NOTREACHED because it shouldn't ever be called (I presume).

Comment 50

[Nick Cameron [:nrc]]

Attachment: patch 9.5 test tweaks

made one test conditional and added an IsLinux method to do so. Carrying r=jrmuizel

Comment 51

[Nick Cameron [:nrc]]

Attachment: patch 5: drawing changes to DrawTargetCairo

changes as requested; carrying r=jrmuizel

Comment 52

[Nick Cameron [:nrc]]

Attachment: patch 7: changes to nsCanvasRenderingContext2DAzure

removed the mNewPath stuff (to Bug 775063).

Comment 53

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 637009 [details] [diff] [review]
patch 10: Fixed build break for qt

this patch is no longer needed because the patch causing the original build break is no longer being used.

Comment 54

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

So, for exceptions, I think we want different messages for the two places where we call onFailure. The one in wrapObjectTemplate should say something about the value not being an object, and the one next to the xpc_qsUnwrapArg call something about it not implementing descriptor.interface.identifier. (The latter never deals with non-object values.)

Comment 55

[Nick Cameron [:nrc]]

Attachment: patch 7: changes to nsCanvasRenderingContext2DAzure

removed all the bits Bas didn't like and fixed the bits which needed fixing

Comment 56

[Nick Cameron [:nrc]]

Attachment: patch 10: mochitest for isPointInPath and multiple transforms

Comment 57

[Nick Cameron [:nrc]]

Attachment: patch 10: mochitest for isPointInPath and multiple transforms

Comment 58

[Nick Cameron [:nrc]]

Attachment: patch 11: enabling Azure Canvas pt 1

Comment 59

[Nick Cameron [:nrc]]

Attachment: patch 12: enable Azure canvas pt 2

I'm a bit unsure about this one - is using the backend type as a bitmask really nasty? Is there a better way to do this?

Comment 60

[Nick Cameron [:nrc]]

Attachment: patch 13: enable Azure canvas pt 3

Comment 61

[Nick Cameron [:nrc]]

Attachment: patch 11: enabling Azure Canvas pt 1

core -> cg

Comment 62

[Nick Cameron [:nrc]]

Attachment: patch 12: enable Azure canvas pt 2

core -> cg

Comment 63

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

Comment on attachment 644833 [details] [diff] [review]
patch 12: enable Azure canvas pt 2

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

::: gfx/thebes/gfxPlatform.cpp
@@ +730,5 @@
> +  if (strcmp(aName, "skia") == 0)
> +    return BACKEND_SKIA;
> +  if (strcmp(aName, "direct2d") == 0)
> +    return BACKEND_DIRECT2D;
> +  if (strcmp(aName, "core") == 0)

Better to just take const nsCString& and call EqualsLiteral here. char*s are somewhat evil and unsafe so the more we can avoid using them, the better, especially when (like here) the nsCString code is just as simple.

I think "cg" is better then "core", which is a bit ambiguous.

::: gfx/thebes/gfxPlatform.h
@@ +467,5 @@
> +
> +    // returns the first backend named in the pref gfx.canvas.azure.backends
> +    // which is a component of aMask, a bitmask of backend types
> +    static mozilla::gfx::BackendType GetBackendPref(mozilla::gfx::BackendType aMask);
> +    static mozilla::gfx::BackendType BackendTypeForName(const char* aName);

GetBackendPref/BackendTypeForName are not good names for functions which are canvas-specific. Nor are mFallbackDrawTargetBackend and mPreferredDrawTargetBackend good names for data which are canvas-specific.

Either we keep these names, and make the pref cover content as well as canvas, or we parameterize these over "content" and "canvas".

Comment 64

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

What about different kinds of cairo backends? Should we split cairo into cairo-image, cairo-xlib, cairo-gdi, etc?

Comment 65

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #64)
> What about different kinds of cairo backends? Should we split cairo into
> cairo-image, cairo-xlib, cairo-gdi, etc?

I'd be inclined to use a different pref for that, so that the azure.backends pref is one-to-one with the actual Azure backends.

What do we need to be able to switch Cairo surface type for?

Comment 66

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #63)
> Comment on attachment 644833 [details] [diff] [review]
> patch 12: enable Azure canvas pt 2
> 
> Review of attachment 644833 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Either we keep these names, and make the pref cover content as well as
> canvas, or we parameterize these over "content" and "canvas".

Is it worth having different backend preferences for canvas and content? I guess we don't need to fallback for content; do we? I had assumed we'd use the same pref for canvas/content, but maybe we don't want to.

Comment 67

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 644834 [details] [diff] [review]
patch 13: enable Azure canvas pt 3

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

::: widget/xpwidgets/GfxInfoBase.cpp
@@ +731,5 @@
> +  static bool azureInited = false;
> +
> +  if (!azureInited) {
> +    nsresult rv = mozilla::Preferences::GetBool("gfx.canvas.azure.enabled", &azure);
> +    azure = NS_SUCCEEDED(rv) && azure;

Maybe 'azure = Preferences::GetBool("gfx.canvas.azure.enabled", false);'

Comment 68

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 644836 [details] [diff] [review]
patch 11: enabling Azure Canvas pt 1

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

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +569,5 @@
>  nsresult
>  NS_NewCanvasRenderingContext2DAzure(nsIDOMCanvasRenderingContext2D** aResult)
>  {
> +  // XXX[nrc] remove this check when Thebes canvas is removed
> +  // (because we will always support Azure)

*shrug*. This entire function will be able to go by then.

Comment 69

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 644831 [details] [diff] [review]
patch 10: mochitest for isPointInPath and multiple transforms

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

Now also <http://w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.path.isPointInPath.transform.4.html>.

Comment 70

[Nick Cameron [:nrc]]

Attachment: patch 11: enabling Azure Canvas pt 1

Comment 71

[Nick Cameron [:nrc]]

Attachment: patch 12: enable Azure canvas pt 2

Comment 72

[Nick Cameron [:nrc]]

Attachment: patch 13: enable Azure canvas pt 3

All three patches updated for comments by roc and Ms2ger (thanks for the comments!)

Comment 73

[Bas Schouten (:bas.schouten)]

Comment on attachment 645193 [details] [diff] [review]
patch 12: enable Azure canvas pt 2

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

::: gfx/layers/d3d10/LayerManagerD3D10.cpp
@@ +488,4 @@
>    if ((aFormat != FORMAT_B8G8R8A8 &&
> +       aFormat != FORMAT_B8G8R8X8) ||
> +       !gfxPlatform::GetPlatform()->SupportsAzure(backend) ||
> +       backend != BACKEND_DIRECT2D) {

This really can't happen with D3D10 layers.

::: gfx/thebes/gfxAndroidPlatform.h
@@ -32,5 @@
>      virtual already_AddRefed<gfxASurface>
>      CreateOffscreenSurface(const gfxIntSize& size,
>                             gfxASurface::gfxContentType contentType);
>      
> -    virtual bool SupportsAzure(mozilla::gfx::BackendType& aBackend) { aBackend = mozilla::gfx::BACKEND_SKIA; return true; }

Why was this removed?

::: gfx/thebes/gfxPlatform.h
@@ +469,5 @@
> +
> +    /**
> +     * initialise preferred and fallback canvas backends
> +     * both backends will be contained in aBackendBitmask, the order is
> +     * determined by the gfx.canvas.azure.backends pref

This comment isn't very clear in my opinion.

Comment 74

[Bas Schouten (:bas.schouten)]

(In reply to Bas Schouten (:bas) from comment #73)
> Comment on attachment 645193 [details] [diff] [review]
> patch 12: enable Azure canvas pt 2
> 
> Review of attachment 645193 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxAndroidPlatform.h
> @@ -32,5 @@
> >      virtual already_AddRefed<gfxASurface>
> >      CreateOffscreenSurface(const gfxIntSize& size,
> >                             gfxASurface::gfxContentType contentType);
> >      
> > -    virtual bool SupportsAzure(mozilla::gfx::BackendType& aBackend) { aBackend = mozilla::gfx::BACKEND_SKIA; return true; }
> 
> Why was this removed?

I should be more clear here, it was removed, does that mean support in gfxAndroidPlatform is identical to that in gfxPlatform?

Comment 75

[Bas Schouten (:bas.schouten)]

Comment on attachment 645194 [details] [diff] [review]
patch 13: enable Azure canvas pt 3

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

It would probably be worth making sure the fallback backend is also reported here, but we can do that later.

Comment 76

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas) from comment #74)
> (In reply to Bas Schouten (:bas) from comment #73)
> > Comment on attachment 645193 [details] [diff] [review]
> > patch 12: enable Azure canvas pt 2
> > 
> > Review of attachment 645193 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/thebes/gfxAndroidPlatform.h
> > @@ -32,5 @@
> > >      virtual already_AddRefed<gfxASurface>
> > >      CreateOffscreenSurface(const gfxIntSize& size,
> > >                             gfxASurface::gfxContentType contentType);
> > >      
> > > -    virtual bool SupportsAzure(mozilla::gfx::BackendType& aBackend) { aBackend = mozilla::gfx::BACKEND_SKIA; return true; }
> > 
> > Why was this removed?
> 
> I should be more clear here, it was removed, does that mean support in
> gfxAndroidPlatform is identical to that in gfxPlatform?

The behaviour is fixed to that of gfxPlatform, but the behaviour is to return mPreferredCanvasBackend, which is set differently for different platforms. Right now, we will return cairo for Android because Skia is kind of broken, but hopefully we'll fix that and switch to Skia, if it proves to have better perf (which it probably will).

Comment 77

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas) from comment #75)
> Comment on attachment 645194 [details] [diff] [review]
> patch 13: enable Azure canvas pt 3
> 
> Review of attachment 645194 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would probably be worth making sure the fallback backend is also reported
> here, but we can do that later.

What do you mean by report? I filed bug 776802 about providing better info from gfxPlatform about the Azure backend.

Comment 78

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas) from comment #73)
> Comment on attachment 645193 [details] [diff] [review]
> patch 12: enable Azure canvas pt 2
> 
> Review of attachment 645193 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d10/LayerManagerD3D10.cpp
> @@ +488,4 @@
> >    if ((aFormat != FORMAT_B8G8R8A8 &&
> > +       aFormat != FORMAT_B8G8R8X8) ||
> > +       !gfxPlatform::GetPlatform()->SupportsAzure(backend) ||
> > +       backend != BACKEND_DIRECT2D) {
> 
> This really can't happen with D3D10 layers.
> 

It can happen if the user sets the pref to use a different backend, and without this change, that pref would not be respected.

Comment 79

[Nick Cameron [:nrc]]

Attachment: patch 12: enable Azure canvas pt 2

improved comment, changed static variable with destructor to pointer

Comment 80

[Nick Cameron [:nrc]]

Attachment: patch 14: remove AzureEnabled from gfxInfo

Comment 81

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

Comment on attachment 646007 [details] [diff] [review]
patch 14: remove AzureEnabled from gfxInfo

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

::: gfx/thebes/gfxPlatform.cpp
@@ +1177,5 @@
> +    static bool azureEnabled;
> +    if (!checkedPref) {
> +        azureEnabled = Preferences::GetBool("gfx.canvas.azure.enabled", false);
> +        checkedPref = true;
> +    }

Why not just always check this pref? No need to cache it AFAICT.

@@ +1181,5 @@
> +    }
> +
> +    if (!azureEnabled) {
> +        mPreferredCanvasBackend = BACKEND_NONE;
> +        mFallbackCanvasBackend = BACKEND_NONE;

and return? Because you overwrite these below.

Comment 82

[Nick Cameron [:nrc]]

Attachment: patch 14: remove AzureEnabled from gfxInfo

fixed roc's comments and changed GetAzureBackendInfo slightly

Comment 83

[Nick Cameron [:nrc]]

Attachment: patch 14: remove AzureEnabled from gfxInfo

removed an unnecessary pref check from AzureCanvasEnabled in nsCanvasRenderingContext2DAzure and fixed some typo bugs. Carrying r=roc.

Comment 84

[Nick Cameron [:nrc]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=eb4f2872a6ca

Comment 85

[Nick Cameron [:nrc]]

backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ff7d09c5c945

Comment 86

[Nick Cameron [:nrc]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c5125dde4bbf

Comment 87

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/9b9acad7f6d0
https://hg.mozilla.org/mozilla-central/rev/06a5feea546f
https://hg.mozilla.org/mozilla-central/rev/f4c2417994aa
https://hg.mozilla.org/mozilla-central/rev/f5a58948203f
https://hg.mozilla.org/mozilla-central/rev/ad4139ad6247
https://hg.mozilla.org/mozilla-central/rev/f731bb5b22b8
https://hg.mozilla.org/mozilla-central/rev/e2deb8b45529
https://hg.mozilla.org/mozilla-central/rev/1260e5f010c0
https://hg.mozilla.org/mozilla-central/rev/0223c2ea0033
https://hg.mozilla.org/mozilla-central/rev/cce58a8050f3
https://hg.mozilla.org/mozilla-central/rev/0f8c4f6e6248
https://hg.mozilla.org/mozilla-central/rev/0278d5acd5f2
https://hg.mozilla.org/mozilla-central/rev/fcc82e42815f
https://hg.mozilla.org/mozilla-central/rev/03e169cce29a
https://hg.mozilla.org/mozilla-central/rev/79a562f4b12d
https://hg.mozilla.org/mozilla-central/rev/148d57e5d062
https://hg.mozilla.org/mozilla-central/rev/3c73222e6e1c