[Skia] Get Skia backend working on Windows

RESOLVED FIXED in mozilla11

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: mattwoodrow, Assigned: marco)

Tracking

unspecified
mozilla11
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: mentor=mattwoodrow)

Attachments

(2 attachments, 11 obsolete attachments)

41.83 KB, patch
Details | Diff | Splinter Review
19.35 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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?
(Reporter)

Updated

6 years ago
Blocks: 687187
(Assignee)

Comment 1

6 years ago
I'll try to work on this bug, can I contact you if I need some help?
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Comment 3

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

Comment 4

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

Comment 5

6 years ago
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
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #572302 - Flags: review?(matt.woodrow)
(Assignee)

Comment 6

6 years ago
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
Attachment #572302 - Attachment is obsolete: true
Attachment #572302 - Flags: review?(matt.woodrow)
Attachment #572305 - Flags: review?(matt.woodrow)
(Assignee)

Comment 7

6 years ago
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
Attachment #572305 - Attachment is obsolete: true
Attachment #572305 - Flags: review?(matt.woodrow)
Attachment #572314 - Flags: review?(matt.woodrow)
(Assignee)

Comment 8

6 years ago
Created attachment 572316 [details] [diff] [review]
skia_restrict_problem.patch

The patch to solve the latest VS2005 compilation problems.
Attachment #572316 - Flags: review?(matt.woodrow)
(Reporter)

Comment 9

6 years ago
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
Attachment #572314 - Flags: review?(matt.woodrow) → review+
(Reporter)

Comment 10

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

Comment 11

6 years ago
(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 )
(Reporter)

Comment 12

6 years ago
(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.
(Reporter)

Updated

6 years ago
Blocks: 700179
(Reporter)

Comment 13

6 years ago
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
(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.
(Reporter)

Comment 15

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

Comment 16

6 years ago
Created attachment 572581 [details] [diff] [review]
Enable Skia for Windows v4

Removed the skia_restrict_problem bits and added the check for prefer-skia.
Attachment #572314 - Attachment is obsolete: true
Attachment #572581 - Flags: review?(matt.woodrow)
(Assignee)

Comment 17

6 years ago
Created attachment 572582 [details] [diff] [review]
Skia Restrict Problem
Attachment #572316 - Attachment is obsolete: true
Attachment #572316 - Flags: review?(matt.woodrow)
Attachment #572582 - Flags: review?(matt.woodrow)
(Assignee)

Updated

6 years ago
Attachment #572582 - Attachment is patch: true
(Reporter)

Comment 18

6 years ago
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.
Attachment #572581 - Flags: review?(matt.woodrow) → review+
(Reporter)

Updated

6 years ago
Attachment #572582 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 19

6 years ago
Created attachment 573186 [details] [diff] [review]
Enable Skia for Windows v5

Addressed last comment.
Attachment #572581 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 20

6 years ago
Created attachment 573939 [details] [diff] [review]
Skia Restrict Problem

Added bug number to the patch.
Attachment #572582 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Created attachment 573940 [details]
Enable Skia for Windows v5

Removed try text.
Attachment #573186 - Attachment is obsolete: true
(Assignee)

Comment 22

6 years ago
Created attachment 573945 [details] [diff] [review]
Skia Restrict Problem
Attachment #573939 - Attachment is obsolete: true
(Assignee)

Comment 23

6 years ago
Created attachment 573946 [details] [diff] [review]
Enable Skia for Windows v5
Attachment #573940 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/3abfdb807a4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddda2b25e28
status-firefox11: --- → fixed
Keywords: checkin-needed
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
status-firefox11: fixed → ---
(Reporter)

Comment 26

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

Comment 27

6 years ago
Created attachment 574449 [details] [diff] [review]
Enable Skia for Windows v6
Attachment #573946 - Attachment is obsolete: true
Attachment #574449 - Flags: review?(matt.woodrow)
(Reporter)

Comment 28

6 years ago
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.
Attachment #574449 - Flags: review?(matt.woodrow) → review+
Crash Signature: [@ nsCanvasRenderingContext2DAzure::InitializeWithTarget]
Keywords: crash
Whiteboard: mentor=mattwoodrow → mentor=mattwoodrow, [mobile-crash]

Comment 29

6 years ago
Just FYI, as probably also seen from the whiteboard Naoki added, we have hit the comment #25 crash on Android as well.
(Reporter)

Comment 30

6 years ago
Filed bug 703109 for the mobile problem, this bug is for a windows implementation.
Crash Signature: [@ nsCanvasRenderingContext2DAzure::InitializeWithTarget]
Keywords: crash
(Reporter)

Updated

6 years ago
Whiteboard: mentor=mattwoodrow, [mobile-crash] → mentor=mattwoodrow
(Assignee)

Comment 31

6 years ago
Created attachment 575295 [details] [diff] [review]
Enable Skia for Windows v7

Moved SK_IGNORE_STDINT_DOT_H definition to SkUserConfig.h.
Attachment #574449 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Reporter)

Comment 32

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e63e256daecc
https://hg.mozilla.org/integration/mozilla-inbound/rev/215593486382
https://hg.mozilla.org/mozilla-central/rev/e63e256daecc
https://hg.mozilla.org/mozilla-central/rev/215593486382
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

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