699258 - [Skia] Get Skia backend working on Windows

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Currently skia only works on Mac, we should get it building and usable on windows (maybe only when we don't have direct2d).

This should be fairly easy, just needs the Makefiles updated to build Skia on windows, and the Azure initialization code changed to create a Skia DrawTarget.

Anyone interested in taking this?

Comment 1

[Marco Castelluccio [:marco]]

I'll try to work on this bug, can I contact you if I need some help?

Comment 2

[Matt Woodrow (:mattwoodrow)]

Absolutely!

I'm 'mattwoodrow' on irc if that's easier for you.

I forgot to mention in the initial comment that we'll also need to hook up text drawing for windows.

This shouldn't be *too* hard, you need to find the windows specific skia source files (src/opts) and add them to the Makefile and then look at ScaledFontSkia/ScaledFontMac to get them usable.

Comment 3

[Matt Woodrow (:mattwoodrow)]

A quick rundown on the work required for this/files involved.

gfx/Makefile.in is where we decide to compile skia at all.

gfx/skia/Makefile.in is the skia source makefile, the platform specific parts of this will need windows equivalents. In particular deciding which platform files to compile (files from skia/src/opts).

gfx/2d/Makefile.in contains the makefile for Azure (and in particular, our Skia backend)
content/canvas/src/nsCanvasRenderingContext2DAzure.cpp contains the canvas implementation, I believe the constructor here has direct2d specific checks.

gfx/platform/gfxWindowsPlatform.cpp:CreateOffscreenDrawTarget chooses which Azure backend to use on windows.

gfx/platform/gfxWindowsPlatform.cpp:GetScaledFontForFont creates an Azure font object from a Thebes font object. The skia backed Azure font object (ScaledFontSkia) wraps around an SkTypeface. Since Skia provides an explicit interface for creating SkTypefaces from mac font objects (CGFontRef) we have an Azure font subclass that uses this (ScaledFontMac). We instantiate these in gfx/2d/Factory.cpp and thebes/gfxPlatformMac.cpp. In the cases where we don't have a platform specific method to create fonts we just lookup the string name of the gfxFont object and use this. See thebes/gfxAndroidPlatform.cpp for an example of this.

Comment 4

[Marco Castelluccio [:marco]]

Thank you Matt, I've yet done the changes in the makefiles and in the other files. My doubt was if in ScaledFontWin I have to use gfxGDIFont or not. For now I'm using it with SkCreateTypefaceFromLOGFONT, because gfxGDIFont provide a HFONT, and so a LOGFONT.

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Enable Skia for Windows

I've tested it and it works when D3D9 is used for layers acceleration. With OpenGL instead it doesn't work.
I sent the patch to try: https://tbpl.mozilla.org/?tree=Try&rev=6a421bef0733

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Enable Skia for Windows v2

Modified the patch to define SK_IGNORE_STDINT_DOT_H when building for Windows to not include stdint.h. Probabily the build wasn't failing on my system because I have stdint.h.
Resent to try: https://tbpl.mozilla.org/?tree=Try&rev=3998bc308c58

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Enable Skia for Windows v3

The compilation error is:
error C2259: 'Radial_Gradient' : cannot instantiate abstract class
due to following members:
  'void SkShader::shadeSpan(int,int,SkPMColor [],int)' : is abstract

And the same with the class Two_Point_Radial_Gradient.

The declaration of shadeSpan in SkShader class is: virtual void shadeSpan(int x, int y, SkPMColor[], int count) = 0;

The function is implemented in Radial_Gradient (and Two_Point_Radial_Gradient) as: void Radial_Gradient::shadeSpan(int x, int y, SkPMColor* SK_RESTRICT dstC, int count)

So the compiler gives the warning:
warning C4301: 'Radial_Gradient::shadeSpan': overriding virtual function only differs from 'SkShader::shadeSpan' by const/volatile qualifier

The problem is that this warning prevents the use of the function, so the abstract class can't be instantiated.
The problem however is present only in VS2005: http://msdn.microsoft.com/en-US/library/bby1ahfc%28v=VS.80%29.aspx

I've created another patch to resolve this problem, but as it involves the Skia code I don't know if it's the right approach.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=8c1234d5877e

Comment 8

[Marco Castelluccio [:marco]]

Attachment: skia_restrict_problem.patch

The patch to solve the latest VS2005 compilation problems.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 572314 [details] [diff] [review]
Enable Skia for Windows v3

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

skia_restrict_problem.patch and the update.sh changes should be part of the other patch.

The rest looks awesome :)

::: gfx/skia/Makefile.in
@@ -171,5 @@
>  	include/effects/SkBlurMaskFilter.h \
>  	include/effects/SkLayerDrawLooper.h \
>  	include/effects/SkLayerRasterizer.h \
>  	include/effects/SkDashPathEffect.h \
> -	include/ports/SkTypeface_mac.h \

Do we still compile on mac without this file being exported? This should be needed for ScaledFontMac.cpp

@@ +305,5 @@
>  	SkBitmapProcState_opts_SSE2.cpp \
>  	SkBlitRow_opts_SSE2.cpp \
>  	SkUtils_opts_SSE2.cpp \
>  	opts_check_SSE2.cpp \
> +	SkTime_Unix.cpp \

Same here without SkMMapStream.cpp

Comment 10

[Matt Woodrow (:mattwoodrow)]

(In reply to Marco Castelluccio from comment #8)
> Created attachment 572316 [details] [diff] [review] [diff] [details] [review]
> skia_restrict_problem.patch
> 
> The patch to solve the latest VS2005 compilation problems.

I'll wait to see if this works on try.

Comment 11

[Marco Castelluccio [:marco]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> Comment on attachment 572314 [details] [diff] [review] [diff] [details] [review]
> Enable Skia for Windows v3
> 
> Review of attachment 572314 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> skia_restrict_problem.patch and the update.sh changes should be part of the
> other patch.
> 
> The rest looks awesome :)

Perfect, I'll do this change ;)
 
> ::: gfx/skia/Makefile.in
> @@ -171,5 @@
> >  	include/effects/SkBlurMaskFilter.h \
> >  	include/effects/SkLayerDrawLooper.h \
> >  	include/effects/SkLayerRasterizer.h \
> >  	include/effects/SkDashPathEffect.h \
> > -	include/ports/SkTypeface_mac.h \
> 
> Do we still compile on mac without this file being exported? This should be
> needed for ScaledFontMac.cpp

This file is yet exported when building for Mac:
ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
EXPORTS_skia += \
	include/ports/SkTypeface_mac.h \

> @@ +305,5 @@
> >  	SkBitmapProcState_opts_SSE2.cpp \
> >  	SkBlitRow_opts_SSE2.cpp \
> >  	SkUtils_opts_SSE2.cpp \
> >  	opts_check_SSE2.cpp \
> > +	SkTime_Unix.cpp \
> 
> Same here without SkMMapStream.cpp

Instead I just forgot to add this :D
However Skia continues to be successfully built for Mac also without it ( https://tbpl.mozilla.org/?tree=Try&rev=6a421bef0733 )

Comment 12

[Matt Woodrow (:mattwoodrow)]

(In reply to Marco Castelluccio from comment #11)
> Instead I just forgot to add this :D
> However Skia continues to be successfully built for Mac also without it (
> https://tbpl.mozilla.org/?tree=Try&rev=6a421bef0733 )

Fine by me, no point compiling code that isn't used.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Looks like your fix gets this building on windows on tryserver. However we now have Azure enabled by default (on windows only) and this fails some test and crashes.

I think the best thing to do here is introduce a new about:config pref, and only use skia on windows if that is set. maybe gfx.canvas.azure.prefer-skia = true.

Then move the skia DrawTarget creation code in gfxWindowsPlatform above the direct2d version and only call it if the pref is true.

Example code: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLCanvasElement.cpp#780

CC'ing Bas and Jeff since they might have other ideas here.

Filed bug 700179 for the crash I saw on tryserver

Comment 14

[Bas Schouten (:bas.schouten)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> I think the best thing to do here is introduce a new about:config pref, and
> only use skia on windows if that is set. maybe gfx.canvas.azure.prefer-skia
> = true.
> 
> Then move the skia DrawTarget creation code in gfxWindowsPlatform above the
> direct2d version and only call it if the pref is true.

Below, probably, especially at this point, you don't really want to use Skia when Direct2D is enabled, so to get Skia, you'd switch off Direct2D and turn on azure.prefer-skia.

If we'd use Direct2D for content, surfaces from content being drawn into Skia would result in expensive readback which is undesirable.

Comment 15

[Matt Woodrow (:mattwoodrow)]

(In reply to Bas Schouten (:bas) from comment #14)

> Below, probably, especially at this point, you don't really want to use Skia
> when Direct2D is enabled, so to get Skia, you'd switch off Direct2D and turn
> on azure.prefer-skia.
> 
> If we'd use Direct2D for content, surfaces from content being drawn into
> Skia would result in expensive readback which is undesirable.

Sure, I can't see skia ever being a better choice than direct2d where it's available. That said, giving us the option of forcing it on for debugging/testing purposes doesn't sound like a bad thing. That was at least the intent of the 'prefer-skia' name choice. I'm not overly worried either way though.

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Enable Skia for Windows v4

Removed the skia_restrict_problem bits and added the check for prefer-skia.

Comment 17

[Marco Castelluccio [:marco]]

Attachment: Skia Restrict Problem

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 572581 [details] [diff] [review]
Enable Skia for Windows v4

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

Just one comment, r=mattwoodrow with that fixed.

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +479,5 @@
>  {
> +  if (Preferences::GetBool("gfx.canvas.azure.prefer-skia", false)) {
> +    return Factory::CreateDrawTarget(BACKEND_SKIA, aSize, aFormat);
> +  }
> +

After discussing this with Bas, we've agreed putting this below the DIRECT2D initialization would be better.

Performance in combination with d2d will be terrible, and this will just lead to needless bug reports.

If people want to enable skia on direct2d capable machines, then they will need to manually disable direct2d as well.

Comment 19

[Marco Castelluccio [:marco]]

Attachment: Enable Skia for Windows v5

Addressed last comment.

Comment 20

[Marco Castelluccio [:marco]]

Attachment: Skia Restrict Problem

Added bug number to the patch.

Comment 21

[Marco Castelluccio [:marco]]

Attachment: Enable Skia for Windows v5

Removed try text.

Comment 22

[Marco Castelluccio [:marco]]

Attachment: Skia Restrict Problem

Comment 23

[Marco Castelluccio [:marco]]

Attachment: Enable Skia for Windows v5

Comment 24

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3abfdb807a4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddda2b25e28

Comment 25

[Matt Brubeck (:mbrubeck)]

Backed out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c02f29cef915

because it appears to have caused crashes in Windows XP tests, e.g.:
https://tbpl.mozilla.org/php/getParsedLog.php?id=7359768&tree=Mozilla-Inbound

Thread 0 (crashed)
 0  xul.dll!nsCanvasRenderingContext2DAzure::InitializeWithTarget(mozilla::gfx::DrawTarget *,int,int) [nsCanvasRenderingContext2DAzure.cpp:5ddda2b25e28 : 1307 + 0x26]
    eip = 0x012552f0   esp = 0x0012bd44   ebp = 0x0012bd6c   ebx = 0x03bba424
    esi = 0x03bba400   edi = 0x03bba45c   eax = 0x03bba468   ecx = 0x00000000
    edx = 0x0012bd54   efl = 0x00010246
    Found by: given as instruction pointer in context
 1  xul.dll!nsCanvasRenderingContext2DAzure::SetDimensions(int,int) [nsCanvasRenderingContext2DAzure.cpp:5ddda2b25e28 : 1266 + 0x10]
    eip = 0x01255532   esp = 0x0012bd74   ebp = 0x0012bda8   ebx = 0x00000000
    Found by: call frame info
 2  xul.dll!nsHTMLCanvasElement::UpdateContext(nsIPropertyBag *) [nsHTMLCanvasElement.cpp:5ddda2b25e28 : 622 + 0xd]
    eip = 0x01291d4f   esp = 0x0012bdb0   ebp = 0x0012bdd4   ebx = 0x0665f400
    Found by: call frame info
 3  xul.dll!nsHTMLCanvasElement::GetContext(nsAString_internal const &,JS::Value const &,nsISupports * *) [nsHTMLCanvasElement.cpp:5ddda2b25e28 : 540 + 0xc]
    eip = 0x01292320   esp = 0x0012bddc   ebp = 0x0012be44   ebx = 0x0665f444
    Found by: call frame info
 4  xul.dll!NS_InvokeByIndex_P [xptcinvoke.cpp:5ddda2b25e28 : 102 + 0x2]
    eip = 0x01796229   esp = 0x0012be4c   ebp = 0x0012be68   ebx = 0x0012bef8
    Found by: call frame info
 5  xul.dll!XPCWrappedNative::CallMethod(XPCCallContext &,XPCWrappedNative::CallMode) [XPCWrappedNative.cpp:5ddda2b25e28 : 2200 + 0xa54]
    eip = 0x014f0a78   esp = 0x0012be70   ebp = 0x0012c09c
    Found by: call frame info

Comment 26

[Matt Woodrow (:mattwoodrow)]

So once we get past NS_NewCanvasRenderingContext2DAzure, our Azure code assumes that we can always create a DrawTarget of some form, which isn't true on XP with prefer-skia set to false.

Quick fix would be check this pref in this function - and essentially replicate the logic used in gfxWindowsPlatform::CreateOffscreenDrawTarget.

We could share this logic in a gfxPlatform::GetPreferredAzureBackend() function (which would include a 'none' option), though since the original assumption probably should be true in the long run, this is probably not worth it.

Comment 27

[Marco Castelluccio [:marco]]

Attachment: Enable Skia for Windows v6

Comment 28

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 574449 [details] [diff] [review]
Enable Skia for Windows v6

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

::: gfx/skia/include/core/SkPreConfig.h
@@ +58,5 @@
>      #if !defined(SK_RESTRICT)
>          #define SK_RESTRICT __restrict
>      #endif
>      #include "sk_stdint.h"
> +    #define SK_IGNORE_STDINT_DOT_H

Can you move this to config/SkUserConfig.h please, it will be easier if we have all define changes in one place when updating skia.

Comment 29

[Robert Kaiser]

Just FYI, as probably also seen from the whiteboard Naoki added, we have hit the comment #25 crash on Android as well.

Comment 30

[Matt Woodrow (:mattwoodrow)]

Filed bug 703109 for the mobile problem, this bug is for a windows implementation.

Comment 31

[Marco Castelluccio [:marco]]

Attachment: Enable Skia for Windows v7

Moved SK_IGNORE_STDINT_DOT_H definition to SkUserConfig.h.

Comment 32

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e63e256daecc
https://hg.mozilla.org/integration/mozilla-inbound/rev/215593486382

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e63e256daecc
https://hg.mozilla.org/mozilla-central/rev/215593486382