Last Comment Bug 746883 - [Azure] Get Skia backend for Azure canvas working on Windows
: [Azure] Get Skia backend for Azure canvas working on Windows
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal with 2 votes (vote)
: mozilla17
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on: 981391 718849 746896 751431 761890
Blocks: 687187 749998 734668 748116 773460
  Show dependency treegraph
 
Reported: 2012-04-18 22:20 PDT by Nick Cameron [:nrc]
Modified: 2014-03-10 09:34 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1: enabling Skia in Windows and misc. small changes (3.91 KB, patch)
2012-05-02 15:41 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 2: fixing Skia fonts in Azure canvas (8.04 KB, patch)
2012-05-02 15:42 PDT, Nick Cameron [:nrc]
gwright: review+
Details | Diff | Review
patch 3: fixed a bunch of nits I found (4.26 KB, patch)
2012-05-02 15:42 PDT, Nick Cameron [:nrc]
gwright: review+
Details | Diff | Review
patch 4: tests which now pass (6.73 KB, patch)
2012-05-02 15:43 PDT, Nick Cameron [:nrc]
gwright: review+
Details | Diff | Review
patch 5: tweaks to other tests (12.55 KB, patch)
2012-05-02 15:44 PDT, Nick Cameron [:nrc]
gwright: review+
Details | Diff | Review
testing only: turn Skia/Azure on by default. DON'T LAND THIS! (1018 bytes, patch)
2012-05-02 15:45 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 5: tweaks to other tests (12.55 KB, patch)
2012-05-03 21:37 PDT, Nick Cameron [:nrc]
jmuizelaar: review+
Details | Diff | Review
patch 4: tests which now pass (6.63 KB, patch)
2012-05-03 21:38 PDT, Nick Cameron [:nrc]
jmuizelaar: review+
Details | Diff | Review
patch 2: fixing Skia fonts in Azure canvas (9.02 KB, patch)
2012-05-03 21:56 PDT, Nick Cameron [:nrc]
gwright: review+
Details | Diff | Review
patch 2.5: font fixes, the changes to Skia library (5.63 KB, patch)
2012-05-03 21:57 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review
patch 1: enabling Skia in Windows and misc. small changes (4.19 KB, patch)
2012-05-03 22:27 PDT, Nick Cameron [:nrc]
gwright: review+
Details | Diff | Review
patch 1: misc. small changes (3.52 KB, patch)
2012-06-05 21:59 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review
patch x: fix Skia bustage due to bug 746896 (16.83 KB, patch)
2012-07-25 20:46 PDT, Nick Cameron [:nrc]
matt.woodrow: review+
Details | Diff | Review
patch x1: fix Skia bustage due to bug 746896 (15.79 KB, patch)
2012-07-25 21:25 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review
patch x2: pref on Skia (1.20 KB, patch)
2012-07-25 21:26 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review
patch x1.5: test fuzzing for gradients (10.54 KB, patch)
2012-07-29 20:39 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Review
patch x1.5: test fuzzing for gradients (11.10 KB, patch)
2012-07-29 22:43 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review
patch x1.6: more fuzz (2.65 KB, patch)
2012-07-30 15:31 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Review

Description Nick Cameron [:nrc] 2012-04-18 22:20:03 PDT
Support Skia as a backend for Azure canvases on Windows and make sure all the tests pass.
Comment 1 Nick Cameron [:nrc] 2012-05-02 15:41:09 PDT
Created attachment 620486 [details] [diff] [review]
patch 1: enabling Skia in Windows and misc. small changes
Comment 2 Nick Cameron [:nrc] 2012-05-02 15:42:02 PDT
Created attachment 620487 [details] [diff] [review]
patch 2: fixing Skia fonts in Azure canvas
Comment 3 Nick Cameron [:nrc] 2012-05-02 15:42:43 PDT
Created attachment 620489 [details] [diff] [review]
patch 3: fixed a bunch of nits I found
Comment 4 Nick Cameron [:nrc] 2012-05-02 15:43:59 PDT
Created attachment 620490 [details] [diff] [review]
patch 4: tests which now pass
Comment 5 Nick Cameron [:nrc] 2012-05-02 15:44:35 PDT
Created attachment 620491 [details] [diff] [review]
patch 5: tweaks to other tests
Comment 6 Nick Cameron [:nrc] 2012-05-02 15:45:36 PDT
Created attachment 620492 [details] [diff] [review]
testing only: turn Skia/Azure on by default. DON'T LAND THIS!
Comment 7 Nick Cameron [:nrc] 2012-05-02 15:49:48 PDT
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) 2012-05-02 16:06:08 PDT
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) (:gwright) 2012-05-02 16:15:38 PDT
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) (:gwright) 2012-05-02 16:19:03 PDT
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) 2012-05-02 16:21:40 PDT
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) (:gwright) 2012-05-02 16:22:45 PDT
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) (:gwright) 2012-05-02 16:23:10 PDT
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 2012-05-03 04:20:22 PDT
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) 2012-05-03 07:05:46 PDT
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] 2012-05-03 21:37:10 PDT
Created attachment 620956 [details] [diff] [review]
patch 5: tweaks to other tests

r=gw280, but he asked for a second opinion
Comment 17 Nick Cameron [:nrc] 2012-05-03 21:38:05 PDT
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
Comment 18 Nick Cameron [:nrc] 2012-05-03 21:48:15 PDT
(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) (:gwright) 2012-05-03 21:56:14 PDT
(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] 2012-05-03 21:56:38 PDT
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.
Comment 21 Nick Cameron [:nrc] 2012-05-03 21:57:32 PDT
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)
Comment 22 George Wright (:gw280) (:gwright) 2012-05-03 22:01:44 PDT
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] 2012-05-03 22:03:13 PDT
(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] 2012-05-03 22:27:21 PDT
Created attachment 620964 [details] [diff] [review]
patch 1: enabling Skia in Windows and misc. small changes
Comment 25 Nick Cameron [:nrc] 2012-05-03 22:30:09 PDT
(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) (:gwright) 2012-05-22 14:55:46 PDT
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] 2012-06-05 21:59:26 PDT
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
Comment 28 Nick Cameron [:nrc] 2012-07-24 02:12:50 PDT
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] 2012-07-25 20:46:46 PDT
Created attachment 646003 [details] [diff] [review]
patch x: fix Skia bustage due to bug 746896
Comment 30 Matt Woodrow (:mattwoodrow) 2012-07-25 21:18:06 PDT
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] 2012-07-25 21:25:24 PDT
Created attachment 646015 [details] [diff] [review]
patch x1: fix Skia bustage due to bug 746896

splitting up patch x, carrying r=mattwoodrow
Comment 32 Nick Cameron [:nrc] 2012-07-25 21:26:33 PDT
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
Comment 34 Nick Cameron [:nrc] 2012-07-25 23:51:00 PDT
backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ff7d09c5c945
Comment 37 Nick Cameron [:nrc] 2012-07-29 20:39:50 PDT
Created attachment 647049 [details] [diff] [review]
patch x1.5: test fuzzing for gradients
Comment 38 Nick Cameron [:nrc] 2012-07-29 22:43:21 PDT
Created attachment 647070 [details] [diff] [review]
patch x1.5: test fuzzing for gradients

changed the name of the flag; carrying r=roc
Comment 39 Nick Cameron [:nrc] 2012-07-30 15:31:59 PDT
Created attachment 647324 [details] [diff] [review]
patch x1.6: more fuzz
Comment 41 Nick Cameron [:nrc] 2012-07-30 23:01:20 PDT
and this too (missing fails-if(Android) in earlier patch): https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=af118f5f9444
Comment 42 Nick Cameron [:nrc] 2012-07-30 23:22:41 PDT
backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=cda1c40da986

Note You need to log in before you can comment on or make changes to this bug.