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

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla17
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 13 obsolete attachments)

4.26 KB, patch
gw280
: review+
Details | Diff | Splinter Review
3.52 KB, patch
nrc
: review+
Details | Diff | Splinter Review
15.79 KB, patch
nrc
: review+
Details | Diff | Splinter Review
11.10 KB, patch
nrc
: review+
Details | Diff | Splinter Review
2.65 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Support Skia as a backend for Azure canvases on Windows and make sure all the tests pass.
(Assignee)

Updated

5 years ago
Blocks: 748116
Blocks: 749998
(Assignee)

Comment 1

5 years ago
Created attachment 620486 [details] [diff] [review]
patch 1: enabling Skia in Windows and misc. small changes
Attachment #620486 - Flags: review?(gwright)
(Assignee)

Comment 2

5 years ago
Created attachment 620487 [details] [diff] [review]
patch 2: fixing Skia fonts in Azure canvas
Attachment #620487 - Flags: review?(gwright)
(Assignee)

Comment 3

5 years ago
Created attachment 620489 [details] [diff] [review]
patch 3: fixed a bunch of nits I found
Attachment #620489 - Flags: review?(gwright)
(Assignee)

Comment 4

5 years ago
Created attachment 620490 [details] [diff] [review]
patch 4: tests which now pass
Attachment #620490 - Flags: review?(gwright)
(Assignee)

Comment 5

5 years ago
Created attachment 620491 [details] [diff] [review]
patch 5: tweaks to other tests
Attachment #620491 - Flags: review?(gwright)
(Assignee)

Comment 6

5 years ago
Created attachment 620492 [details] [diff] [review]
testing only: turn Skia/Azure on by default. DON'T LAND THIS!
(Assignee)

Comment 7

5 years ago
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 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 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 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
Attachment #620487 - Flags: review?(gwright) → review+
Attachment #620489 - Flags: review?(gwright) → review+
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 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.
Attachment #620490 - Flags: review?(gwright) → review+
Comment on attachment 620491 [details] [diff] [review]
patch 5: tweaks to other tests

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

Same as previous patch
Attachment #620491 - Flags: review?(gwright) → review+
Blocks: 687187
(Assignee)

Updated

5 years ago
Depends on: 751431
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.
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.
(Assignee)

Comment 16

5 years ago
Created attachment 620956 [details] [diff] [review]
patch 5: tweaks to other tests

r=gw280, but he asked for a second opinion
Attachment #620491 - Attachment is obsolete: true
Attachment #620956 - Flags: review?(jmuizelaar)
(Assignee)

Comment 17

5 years ago
Created attachment 620957 [details] [diff] [review]
patch 4: tests which now pass

r=gw280, but wants a second opinion, also removed bug numbers for passing tests
Attachment #620490 - Attachment is obsolete: true
Attachment #620957 - Flags: review?(jmuizelaar)
(Assignee)

Comment 18

5 years ago
(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?
(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.
(Assignee)

Comment 20

5 years ago
Created attachment 620960 [details] [diff] [review]
patch 2: fixing Skia fonts in Azure canvas

Please just check I've got the Skia update thing right, nothing else has changed.
Attachment #620487 - Attachment is obsolete: true
Attachment #620960 - Flags: review?(gwright)
(Assignee)

Comment 21

5 years ago
Created attachment 620961 [details] [diff] [review]
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)
Attachment #620961 - Flags: review+
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.
Attachment #620960 - Flags: review?(gwright) → review+
(Assignee)

Comment 23

5 years ago
(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?
(Assignee)

Comment 24

5 years ago
Created attachment 620964 [details] [diff] [review]
patch 1: enabling Skia in Windows and misc. small changes
Attachment #620486 - Attachment is obsolete: true
Attachment #620486 - Flags: review?(gwright)
Attachment #620964 - Flags: review?(gwright)
(Assignee)

Comment 25

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 718849
Attachment #620956 - Flags: review?(jmuizelaar) → review+
Attachment #620957 - Flags: review?(jmuizelaar) → review+
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
Attachment #620964 - Flags: review?(gwright) → review+
(Assignee)

Updated

5 years ago
Blocks: 761890
(Assignee)

Updated

5 years ago
No longer blocks: 761890
Depends on: 761890
Blocks: 761895
(Assignee)

Comment 27

5 years ago
Created attachment 630441 [details] [diff] [review]
patch 1: misc. small changes

Moved some things out of the patch and to Bug 761890. Carrying r=gw280
Attachment #620964 - Attachment is obsolete: true
Attachment #630441 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #620961 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #620960 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #620956 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #620957 - Attachment is obsolete: true
No longer blocks: 761895
(Assignee)

Updated

5 years ago
Blocks: 773460
(Assignee)

Comment 28

5 years ago
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).
Target Milestone: --- → mozilla16
Version: unspecified → Trunk
(Assignee)

Updated

5 years ago
Depends on: 746896
(Assignee)

Updated

5 years ago
Attachment #620492 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
Created attachment 646003 [details] [diff] [review]
patch x: fix Skia bustage due to bug 746896
Attachment #646003 - Flags: review?(matt.woodrow)
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.
Attachment #646003 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 31

5 years ago
Created attachment 646015 [details] [diff] [review]
patch x1: fix Skia bustage due to bug 746896

splitting up patch x, carrying r=mattwoodrow
Attachment #646003 - Attachment is obsolete: true
Attachment #646015 - Flags: review+
(Assignee)

Comment 32

5 years ago
Created attachment 646016 [details] [diff] [review]
patch x2: pref on Skia

splitting up patch x, carrying r=mattwoodrow

Not going to land this until Azure/Skia canvas works properly
Attachment #646016 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
(Assignee)

Comment 33

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=eb4f2872a6ca
(Assignee)

Comment 34

5 years ago
backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ff7d09c5c945
(Assignee)

Comment 35

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c5125dde4bbf
https://hg.mozilla.org/mozilla-central/rev/05736508e29d
https://hg.mozilla.org/mozilla-central/rev/f27b4da28ef4
https://hg.mozilla.org/mozilla-central/rev/c5125dde4bbf
(Assignee)

Comment 37

5 years ago
Created attachment 647049 [details] [diff] [review]
patch x1.5: test fuzzing for gradients
Attachment #647049 - Flags: review?(roc)
Attachment #647049 - Flags: review?(roc) → review+
(Assignee)

Comment 38

5 years ago
Created attachment 647070 [details] [diff] [review]
patch x1.5: test fuzzing for gradients

changed the name of the flag; carrying r=roc
Attachment #647049 - Attachment is obsolete: true
Attachment #647070 - Flags: review+
(Assignee)

Comment 39

5 years ago
Created attachment 647324 [details] [diff] [review]
patch x1.6: more fuzz
Attachment #647324 - Flags: review?(roc)
Attachment #647324 - Flags: review?(roc) → review+
(Assignee)

Comment 40

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e4baff472b7f
Whiteboard: [leave open]
(Assignee)

Comment 41

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

Updated

5 years ago
Attachment #646016 - Attachment is obsolete: true
(Assignee)

Comment 42

5 years ago
backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=cda1c40da986
(Assignee)

Comment 43

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7198b3ea1763
https://hg.mozilla.org/mozilla-central/rev/9a41f88085af
https://hg.mozilla.org/mozilla-central/rev/b7b30f362def
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla16 → mozilla17
Depends on: 835045
(Assignee)

Updated

4 years ago
No longer depends on: 835045

Updated

3 years ago
Depends on: 981391
You need to log in before you can comment on or make changes to this bug.