746883 - [Azure] Get Skia backend for Azure canvas working on Windows

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nick Cameron [:nrc]]

Support Skia as a backend for Azure canvases on Windows and make sure all the tests pass.

Comment 1

[Nick Cameron [:nrc]]

Attachment: patch 1: enabling Skia in Windows and misc. small changes

Comment 2

[Nick Cameron [:nrc]]

Attachment: patch 2: fixing Skia fonts in Azure canvas

Comment 3

[Nick Cameron [:nrc]]

Attachment: patch 3: fixed a bunch of nits I found

Comment 4

[Nick Cameron [:nrc]]

Attachment: patch 4: tests which now pass

Comment 5

[Nick Cameron [:nrc]]

Attachment: patch 5: tweaks to other tests

Comment 6

[Nick Cameron [:nrc]]

Attachment: testing only: turn Skia/Azure on by default. DON'T LAND THIS!

Comment 7

[Nick Cameron [:nrc]]

Try run, forced Skia in Windows (note the fails are because direct2D is disabled, otherwise it is used as the Azure backend): https://tbpl.mozilla.org/?tree=Try&rev=9aedcc5c6d6e

Try run, all platforms, but Azure/Skia not on by default: https://tbpl.mozilla.org/?tree=Try&rev=9776ce13ff49

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 620487 [details] [diff] [review]
patch 2: fixing Skia fonts in Azure canvas

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

Please put changes to skia source files in a separate patch, include the patch in gfx/skia, and update update.sh.

Comment 9

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

Comment on attachment 620486 [details] [diff] [review]
patch 1: enabling Skia in Windows and misc. small changes

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

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +4462,5 @@
> +bool
> +nsCanvasRenderingContext2DAzure::ShouldForceInactiveLayer(LayerManager *aManager)
> +{
> +    return !aManager->CanUseCanvasLayerForSize(gfxIntSize(mWidth, mHeight));
> +}

What exactly is this for?

::: modules/libpref/src/init/all.js
@@ +252,5 @@
>  #ifdef XP_MACOSX
>  pref("gfx.canvas.azure.enabled", true);
> +#ifdef MOZ_ENABLE_SKIA
> +// currently disabled on Mac
> +// pref("gfx.canvas.azure.prefer-skia", false);

Is this supposed to be commented out?

::: widget/windows/GfxInfo.cpp
@@ +100,5 @@
>    bool d2dEnabled = 
>      gfxWindowsPlatform::GetPlatform()->GetRenderMode() == gfxWindowsPlatform::RENDER_DIRECT2D;
>  
> +  if (mozilla::Preferences::GetBool("gfx.canvas.azure.prefer-skia", false) ||
> +      d2dEnabled) {

You should probably grab this pref on startup instead of getting it every time we call GetAzureEnabled

Comment 10

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

Comment on attachment 620487 [details] [diff] [review]
patch 2: fixing Skia fonts in Azure canvas

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

Looks fine, but please separate out the diffs to skia itself into a separate patch

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 620486 [details] [diff] [review]
patch 1: enabling Skia in Windows and misc. small changes

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

::: widget/windows/GfxInfo.cpp
@@ +100,5 @@
>    bool d2dEnabled = 
>      gfxWindowsPlatform::GetPlatform()->GetRenderMode() == gfxWindowsPlatform::RENDER_DIRECT2D;
>  
> +  if (mozilla::Preferences::GetBool("gfx.canvas.azure.prefer-skia", false) ||
> +      d2dEnabled) {

How about using gfxPlatform::SupportsAzure here instead?

That stops us needing to duplicate this logic.

Comment 12

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

Comment on attachment 620490 [details] [diff] [review]
patch 4: tests which now pass

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

Looks fine to me, but please ask someone who is more well versed with the canvas reftests to review as well.

Comment 13

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

Comment on attachment 620491 [details] [diff] [review]
patch 5: tweaks to other tests

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

Same as previous patch

Comment 14

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

Comment on attachment 620490 [details] [diff] [review]
patch 4: tests which now pass

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

::: layout/reftests/bidi/reftest.list
@@ +91,5 @@
>  == 588739-3.html 588739-ref.html
>  == 612843-1.html 612843-1-ref.html
>  == 613149-1a.html 613149-1-ref.html
>  == 613149-1b.html 613149-1-ref.html
> +== 613149-2a.html 613149-2-ref.html # bug 696672

The bug number at the end of that line is to track the failure. If you fix the failure, remove the bug number and resolve the bug.

Comment 15

[Bas Schouten (:bas.schouten)]

The order here is a little off, enabling Skia cannot work without the later patches. So Skia should be enabled -after- the patches that actually make it work. We should try to make sure after each patch the tree is in a workable state, with tests passing, this reduces pain when people are bisecting bugs and such.

Comment 16

[Nick Cameron [:nrc]]

Attachment: patch 5: tweaks to other tests

r=gw280, but he asked for a second opinion

Comment 17

[Nick Cameron [:nrc]]

Attachment: patch 4: tests which now pass

r=gw280, but wants a second opinion, also removed bug numbers for passing tests

Comment 18

[Nick Cameron [:nrc]]

(In reply to George Wright (:gw280) from comment #9)
> Comment on attachment 620486 [details] [diff] [review]
> patch 1: enabling Skia in Windows and misc. small changes
> 
> Review of attachment 620486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
> @@ +4462,5 @@
> > +bool
> > +nsCanvasRenderingContext2DAzure::ShouldForceInactiveLayer(LayerManager *aManager)
> > +{
> > +    return !aManager->CanUseCanvasLayerForSize(gfxIntSize(mWidth, mHeight));
> > +}
> 
> What exactly is this for?

If the canvas is larger than the largest texture supported by the hardware backend then bad things happen, so in that case we fallback to software rendering. I.e., its for supporting very large canvases.
> 
> ::: modules/libpref/src/init/all.js
> @@ +252,5 @@
> >  #ifdef XP_MACOSX
> >  pref("gfx.canvas.azure.enabled", true);
> > +#ifdef MOZ_ENABLE_SKIA
> > +// currently disabled on Mac
> > +// pref("gfx.canvas.azure.prefer-skia", false);
> 
> Is this supposed to be commented out?

Yes, but it may as well not be there, I've removed it.
> 
> ::: widget/windows/GfxInfo.cpp
> @@ +100,5 @@
> >    bool d2dEnabled = 
> >      gfxWindowsPlatform::GetPlatform()->GetRenderMode() == gfxWindowsPlatform::RENDER_DIRECT2D;
> >  
> > +  if (mozilla::Preferences::GetBool("gfx.canvas.azure.prefer-skia", false) ||
> > +      d2dEnabled) {
> 
> You should probably grab this pref on startup instead of getting it every
> time we call GetAzureEnabled

I got rid of this pref check by using SupportsAzure as mattwoodrow suggested. Does this advice hold in general for prefs that require a restart to take effect? In partticular for the azure-enabled check a few lines down?

Comment 19

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

(In reply to Nick Cameron [:nrc] from comment #18)
> > ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
> > @@ +4462,5 @@
> > > +bool
> > > +nsCanvasRenderingContext2DAzure::ShouldForceInactiveLayer(LayerManager *aManager)
> > > +{
> > > +    return !aManager->CanUseCanvasLayerForSize(gfxIntSize(mWidth, mHeight));
> > > +}
> > 
> > What exactly is this for?
> 
> If the canvas is larger than the largest texture supported by the hardware
> backend then bad things happen, so in that case we fallback to software
> rendering. I.e., its for supporting very large canvases.

Oh right; I was confused because you implement this method but never actually call it anywhere in this patch.

> > ::: modules/libpref/src/init/all.js
> > @@ +252,5 @@
> > >  #ifdef XP_MACOSX
> > >  pref("gfx.canvas.azure.enabled", true);
> > > +#ifdef MOZ_ENABLE_SKIA
> > > +// currently disabled on Mac
> > > +// pref("gfx.canvas.azure.prefer-skia", false);
> > 
> > Is this supposed to be commented out?
> 
> Yes, but it may as well not be there, I've removed it.

Great

> > ::: widget/windows/GfxInfo.cpp
> > @@ +100,5 @@
> > >    bool d2dEnabled = 
> > >      gfxWindowsPlatform::GetPlatform()->GetRenderMode() == gfxWindowsPlatform::RENDER_DIRECT2D;
> > >  
> > > +  if (mozilla::Preferences::GetBool("gfx.canvas.azure.prefer-skia", false) ||
> > > +      d2dEnabled) {
> > 
> > You should probably grab this pref on startup instead of getting it every
> > time we call GetAzureEnabled
> 
> I got rid of this pref check by using SupportsAzure as mattwoodrow
> suggested. Does this advice hold in general for prefs that require a restart
> to take effect? In partticular for the azure-enabled check a few lines down?

I believe so; if it doesn't make sense for the pref to be on-the-fly switchable then only grab it in the constructor. If not, and it's going to be grabbed frequently, then use a var cache (AddVarBoolCache()) and a static variable.

Comment 20

[Nick Cameron [:nrc]]

Attachment: patch 2: fixing Skia fonts in Azure canvas

Please just check I've got the Skia update thing right, nothing else has changed.

Comment 21

[Nick Cameron [:nrc]]

Attachment: patch 2.5: font fixes, the changes to Skia library

carrying r=gw280 from his review of patch 2 (of which this is a subset)

Comment 22

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

Comment on attachment 620960 [details] [diff] [review]
patch 2: fixing Skia fonts in Azure canvas

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

::: gfx/skia/update.sh
@@ +114,5 @@
>  patch -p3 < old-android-fonthost.patch
>  # Bug 731384 - Fix compile errors on older versions of clang
>  patch -p3 < SkPostConfig.patch
> +# Bug 746883  - Correct some bugs with fonts - iterating over whitespace and scaling/canonical size
> +patch -p3 < skia_fonts.patch

I believe you'll actually need to apply this to the repository as well as put it into this script and have the .patch file.

Comment 23

[Nick Cameron [:nrc]]

(In reply to George Wright (:gw280) from comment #22)
> Comment on attachment 620960 [details] [diff] [review]
> patch 2: fixing Skia fonts in Azure canvas
> 
> Review of attachment 620960 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/skia/update.sh
> @@ +114,5 @@
> >  patch -p3 < old-android-fonthost.patch
> >  # Bug 731384 - Fix compile errors on older versions of clang
> >  patch -p3 < SkPostConfig.patch
> > +# Bug 746883  - Correct some bugs with fonts - iterating over whitespace and scaling/canonical size
> > +patch -p3 < skia_fonts.patch
> 
> I believe you'll actually need to apply this to the repository as well as
> put it into this script and have the .patch file.

doesn't having the patch in the patch queue cover that?

Comment 24

[Nick Cameron [:nrc]]

Attachment: patch 1: enabling Skia in Windows and misc. small changes

Comment 25

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas) from comment #15)
> The order here is a little off, enabling Skia cannot work without the later
> patches. So Skia should be enabled -after- the patches that actually make it
> work. We should try to make sure after each patch the tree is in a workable
> state, with tests passing, this reduces pain when people are bisecting bugs
> and such.

I've only been trying to make queue prefixes compile (and not even managing that, sometimes). In the future I'll try to make the prefixes pass tests too.

Comment 26

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

Comment on attachment 620964 [details] [diff] [review]
patch 1: enabling Skia in Windows and misc. small changes

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

Looks fine

Comment 27

[Nick Cameron [:nrc]]

Attachment: patch 1: misc. small changes

Moved some things out of the patch and to Bug 761890. Carrying r=gw280

Comment 28

[Nick Cameron [:nrc]]

This seems to have regressed a tonne while we were busy fixing Cairo. Need to fix it again before turning it on.

try push: https://tbpl.mozilla.org/?tree=Try&rev=16eda4cda712

(To be clear, I will land the existing patches with the Azure/Cairo stuff, but make sure Skia is pref'ed off).

Comment 29

[Nick Cameron [:nrc]]

Attachment: patch x: fix Skia bustage due to bug 746896

Comment 30

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 646003 [details] [diff] [review]
patch x: fix Skia bustage due to bug 746896

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

Looks good to me.

I think we still want to remove the Snapshot()->GetDataSurface() path in the gfxPlatform implementation and replace it with a GetNativeSurface(NATIVE_SURFACE_SKIA) equivalent but that can be done separately.

Comment 31

[Nick Cameron [:nrc]]

Attachment: patch x1: fix Skia bustage due to bug 746896

splitting up patch x, carrying r=mattwoodrow

Comment 32

[Nick Cameron [:nrc]]

Attachment: patch x2: pref on Skia

splitting up patch x, carrying r=mattwoodrow

Not going to land this until Azure/Skia canvas works properly

Comment 33

[Nick Cameron [:nrc]]

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

Comment 34

[Nick Cameron [:nrc]]

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

Comment 35

[Nick Cameron [:nrc]]

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

Comment 36

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/05736508e29d
https://hg.mozilla.org/mozilla-central/rev/f27b4da28ef4
https://hg.mozilla.org/mozilla-central/rev/c5125dde4bbf

Comment 37

[Nick Cameron [:nrc]]

Attachment: patch x1.5: test fuzzing for gradients

Comment 38

[Nick Cameron [:nrc]]

Attachment: patch x1.5: test fuzzing for gradients

changed the name of the flag; carrying r=roc

Comment 39

[Nick Cameron [:nrc]]

Attachment: patch x1.6: more fuzz

Comment 40

[Nick Cameron [:nrc]]

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

Comment 41

[Nick Cameron [:nrc]]

and this too (missing fails-if(Android) in earlier patch): https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=af118f5f9444

Comment 42

[Nick Cameron [:nrc]]

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

Comment 43

[Nick Cameron [:nrc]]

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

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a41f88085af
https://hg.mozilla.org/mozilla-central/rev/b7b30f362def