Last Comment Bug 699258 - [Skia] Get Skia backend working on Windows
: [Skia] Get Skia backend working on Windows
Status: RESOLVED FIXED
mentor=mattwoodrow
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Windows XP
: -- normal with 2 votes (vote)
: mozilla11
Assigned To: Marco Castelluccio [:marco]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 704124
Blocks: 687187 700179
  Show dependency treegraph
 
Reported: 2011-11-02 15:36 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2013-01-11 11:38 PST (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Enable Skia for Windows (18.98 KB, patch)
2011-11-06 07:54 PST, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Enable Skia for Windows v2 (19.29 KB, patch)
2011-11-06 09:01 PST, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Enable Skia for Windows v3 (40.73 KB, patch)
2011-11-06 12:07 PST, Marco Castelluccio [:marco]
matt.woodrow: review+
Details | Diff | Splinter Review
skia_restrict_problem.patch (20.35 KB, patch)
2011-11-06 12:16 PST, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Enable Skia for Windows v4 (19.25 KB, patch)
2011-11-07 12:52 PST, Marco Castelluccio [:marco]
matt.woodrow: review+
Details | Diff | Splinter Review
Skia Restrict Problem (41.80 KB, patch)
2011-11-07 12:52 PST, Marco Castelluccio [:marco]
matt.woodrow: review+
Details | Diff | Splinter Review
Enable Skia for Windows v5 (19.37 KB, patch)
2011-11-09 06:58 PST, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Skia Restrict Problem (41.82 KB, patch)
2011-11-11 15:39 PST, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Enable Skia for Windows v5 (19.19 KB, text/plain)
2011-11-11 15:40 PST, Marco Castelluccio [:marco]
no flags Details
Skia Restrict Problem (41.83 KB, patch)
2011-11-11 15:51 PST, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Enable Skia for Windows v5 (19.20 KB, patch)
2011-11-11 15:51 PST, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Enable Skia for Windows v6 (19.43 KB, patch)
2011-11-14 15:17 PST, Marco Castelluccio [:marco]
matt.woodrow: review+
Details | Diff | Splinter Review
Enable Skia for Windows v7 (19.35 KB, patch)
2011-11-17 14:17 PST, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review

Description Matt Woodrow (:mattwoodrow) 2011-11-02 15:36:42 PDT
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] 2011-11-02 15:45:25 PDT
I'll try to work on this bug, can I contact you if I need some help?
Comment 2 Matt Woodrow (:mattwoodrow) 2011-11-02 16:09:52 PDT
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) 2011-11-04 20:26:50 PDT
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] 2011-11-05 01:27:24 PDT
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] 2011-11-06 07:54:16 PST
Created attachment 572302 [details] [diff] [review]
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] 2011-11-06 09:01:03 PST
Created attachment 572305 [details] [diff] [review]
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] 2011-11-06 12:07:00 PST
Created attachment 572314 [details] [diff] [review]
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] 2011-11-06 12:16:02 PST
Created attachment 572316 [details] [diff] [review]
skia_restrict_problem.patch

The patch to solve the latest VS2005 compilation problems.
Comment 9 Matt Woodrow (:mattwoodrow) 2011-11-06 12:19:58 PST
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) 2011-11-06 12:21:29 PST
(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] 2011-11-06 13:56:50 PST
(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) 2011-11-06 14:19:53 PST
(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) 2011-11-06 16:34:59 PST
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) 2011-11-06 17:56:06 PST
(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) 2011-11-06 19:06:55 PST
(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] 2011-11-07 12:52:06 PST
Created attachment 572581 [details] [diff] [review]
Enable Skia for Windows v4

Removed the skia_restrict_problem bits and added the check for prefer-skia.
Comment 17 Marco Castelluccio [:marco] 2011-11-07 12:52:52 PST
Created attachment 572582 [details] [diff] [review]
Skia Restrict Problem
Comment 18 Matt Woodrow (:mattwoodrow) 2011-11-07 14:48:52 PST
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] 2011-11-09 06:58:24 PST
Created attachment 573186 [details] [diff] [review]
Enable Skia for Windows v5

Addressed last comment.
Comment 20 Marco Castelluccio [:marco] 2011-11-11 15:39:50 PST
Created attachment 573939 [details] [diff] [review]
Skia Restrict Problem

Added bug number to the patch.
Comment 21 Marco Castelluccio [:marco] 2011-11-11 15:40:21 PST
Created attachment 573940 [details]
Enable Skia for Windows v5

Removed try text.
Comment 22 Marco Castelluccio [:marco] 2011-11-11 15:51:31 PST
Created attachment 573945 [details] [diff] [review]
Skia Restrict Problem
Comment 23 Marco Castelluccio [:marco] 2011-11-11 15:51:58 PST
Created attachment 573946 [details] [diff] [review]
Enable Skia for Windows v5
Comment 25 Matt Brubeck (:mbrubeck) 2011-11-11 17:53:28 PST
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) 2011-11-11 19:30:51 PST
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] 2011-11-14 15:17:44 PST
Created attachment 574449 [details] [diff] [review]
Enable Skia for Windows v6
Comment 28 Matt Woodrow (:mattwoodrow) 2011-11-15 03:37:52 PST
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 2011-11-16 14:44:36 PST
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) 2011-11-16 14:49:36 PST
Filed bug 703109 for the mobile problem, this bug is for a windows implementation.
Comment 31 Marco Castelluccio [:marco] 2011-11-17 14:17:12 PST
Created attachment 575295 [details] [diff] [review]
Enable Skia for Windows v7

Moved SK_IGNORE_STDINT_DOT_H definition to SkUserConfig.h.

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