Closed Bug 746883 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: nrc, Assigned: nrc)

References

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

Details

Attachments

(5 files, 13 obsolete files)

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
Support Skia as a backend for Azure canvases on Windows and make sure all the tests pass.
Blocks: 748116
Attachment #620486 - Flags: review?(gwright)
Attachment #620487 - Flags: review?(gwright)
Attachment #620489 - Flags: review?(gwright)
Attached patch patch 4: tests which now pass (obsolete) — Splinter Review
Attachment #620490 - Flags: review?(gwright)
Attached patch patch 5: tweaks to other tests (obsolete) — Splinter Review
Attachment #620491 - Flags: review?(gwright)
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+
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.
Attached patch patch 5: tweaks to other tests (obsolete) — Splinter Review
r=gw280, but he asked for a second opinion
Attachment #620491 - Attachment is obsolete: true
Attachment #620956 - Flags: review?(jmuizelaar)
Attached patch patch 4: tests which now pass (obsolete) — Splinter Review
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)
(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.
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)
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+
(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?
Attachment #620486 - Attachment is obsolete: true
Attachment #620486 - Flags: review?(gwright)
Attachment #620964 - Flags: review?(gwright)
(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.
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+
Blocks: 761890
No longer blocks: 761890
Depends on: 761890
Moved some things out of the patch and to Bug 761890. Carrying r=gw280
Attachment #620964 - Attachment is obsolete: true
Attachment #630441 - Flags: review+
Attachment #620961 - Attachment is obsolete: true
Attachment #620960 - Attachment is obsolete: true
Attachment #620956 - Attachment is obsolete: true
Attachment #620957 - Attachment is obsolete: true
Blocks: 773460
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
Depends on: 746896
Attachment #620492 - Attachment is obsolete: true
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+
splitting up patch x, carrying r=mattwoodrow
Attachment #646003 - Attachment is obsolete: true
Attachment #646015 - Flags: review+
Attached patch patch x2: pref on Skia (obsolete) — Splinter Review
splitting up patch x, carrying r=mattwoodrow

Not going to land this until Azure/Skia canvas works properly
Attachment #646016 - Flags: review+
Whiteboard: [leave open]
Attachment #647049 - Flags: review?(roc)
changed the name of the flag; carrying r=roc
Attachment #647049 - Attachment is obsolete: true
Attachment #647070 - Flags: review+
Attachment #647324 - Flags: review?(roc)
and this too (missing fails-if(Android) in earlier patch): https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=af118f5f9444
Attachment #646016 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9a41f88085af
https://hg.mozilla.org/mozilla-central/rev/b7b30f362def
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: mozilla16 → mozilla17
No longer depends on: 835045
Depends on: 981391
You need to log in before you can comment on or make changes to this bug.