751418 - Enable OpenGL acceleration on Skia

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Marco Castelluccio [:marco]]

Enable by default OpenGL acceleration on Skia.

Comment 1

[George Wright (:gw280) (needinfo me!)]

It makes sense to wait for the rebase before starting this.

Comment 2

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8fdd86263b2f

Comment 3

[Marco Castelluccio [:marco]]

Ed, I think that patch isn't related to this bug.

Comment 4

[George Wright (:gw280) (needinfo me!)]

My bad. It should be https://bugzilla.mozilla.org/show_bug.cgi?id=814174. Reopening.

Comment 5

[George Wright (:gw280) (needinfo me!)]

Attachment: Build Skia with GPU support

Comment 6

[George Wright (:gw280) (needinfo me!)]

Attachment: Add our own GrUserConfig

Comment 7

[George Wright (:gw280) (needinfo me!)]

Attachment: Fix compile error

Comment 8

[George Wright (:gw280) (needinfo me!)]

Attachment: GrGLInterface for mozilla's GLContext

Comment 9

[George Wright (:gw280) (needinfo me!)]

Attachment: Support Ganesh in DrawTargetSkia

Comment 10

[George Wright (:gw280) (needinfo me!)]

Attachment: Plumb through GL-backed CanvasLayers for Skia

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 708872 [details] [diff] [review]
Add our own GrUserConfig

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

Do we need to add something to the patches dir to prevent this from being lost when we do a rebase?

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 708874 [details] [diff] [review]
Fix compile error

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

Same here, I think we need a patch file. And upstreaming :)

Comment 13

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 708876 [details] [diff] [review]
GrGLInterface for mozilla's GLContext

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

This looks fine, definitely haven't checked every line of it though.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 708879 [details] [diff] [review]
Plumb through GL-backed CanvasLayers for Skia

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +804,5 @@
> +       if (gfxPlatform::GetPlatform()->UseAcceleratedCanvas()) {
> +         mGLContext = mozilla::gl::GLContextProvider::CreateOffscreen(gfxIntSize(size.width,
> +                                                                                 size.height));
> +         GrGLInterface* interface = CreateGrInterfaceFromGLContext(mGLContext);
> +         mGrContext = GrContext::Create(kOpenGL_Shaders_GrEngine, (GrPlatform3DContext)interface);

Why isn't the GrContext created and stored within the DrawTarget? It seems really painful to expose skia specific stuff here.

We can expose the GLContext through GetNativeData easily

Comment 15

[Nick Cameron [:nrc]]

Comment on attachment 708879 [details] [diff] [review]
Plumb through GL-backed CanvasLayers for Skia

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

I think we should try to encapsulate more within the DrawTarget, that might mean creating a new SkiaGL draw target, but I can live with that if you can. As mattwoodrow says, I think the GrContext should be kept in the draw target, also the GLContext. In fact, if it is possible I would like CanvasRenderingContext2D not to have to know about this stuff at all and just use the accelerated draw target.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +805,5 @@
> +         mGLContext = mozilla::gl::GLContextProvider::CreateOffscreen(gfxIntSize(size.width,
> +                                                                                 size.height));
> +         GrGLInterface* interface = CreateGrInterfaceFromGLContext(mGLContext);
> +         mGrContext = GrContext::Create(kOpenGL_Shaders_GrEngine, (GrPlatform3DContext)interface);
> +         mTarget = Factory::CreateAcceleratedDrawTarget(mGrContext, size);

Can we use layerManager->CreateDrawTarget here too? And could this choice all be encapsulated in the gfxPlatform rather than being put in canvas?

::: gfx/2d/2D.h
@@ +917,5 @@
> +#ifdef USE_SKIA
> +  static TemporaryRef<DrawTarget>
> +    CreateAcceleratedDrawTarget(GrContext *aContext, const IntSize &aSize);
> +#endif
> +

Could you add 'Skia' to the name of this function since 'accelerated' is getting pretty overloaded.

Comment 16

[George Wright (:gw280) (needinfo me!)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #14)
> Comment on attachment 708879 [details] [diff] [review]
> Plumb through GL-backed CanvasLayers for Skia
> 
> Review of attachment 708879 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +804,5 @@
> > +       if (gfxPlatform::GetPlatform()->UseAcceleratedCanvas()) {
> > +         mGLContext = mozilla::gl::GLContextProvider::CreateOffscreen(gfxIntSize(size.width,
> > +                                                                                 size.height));
> > +         GrGLInterface* interface = CreateGrInterfaceFromGLContext(mGLContext);
> > +         mGrContext = GrContext::Create(kOpenGL_Shaders_GrEngine, (GrPlatform3DContext)interface);
> 
> Why isn't the GrContext created and stored within the DrawTarget? It seems
> really painful to expose skia specific stuff here.
> 
> We can expose the GLContext through GetNativeData easily

Mainly because there needs to be a 1:1 mapping between GrContext and GLContext objects, and in the future I want to have a single GLContext backing all canvases for the entire process. It seemed to me that the easiest place to manage the GrContext in this case was CanvasRenderingContext2D.

However, thinking about it, maybe we can store the associated GrContext for a GLContext on the GLContext object itself? How would you feel about adding a GetSkiaContext() method to GLContext?

Comment 17

[George Wright (:gw280) (needinfo me!)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> Comment on attachment 708874 [details] [diff] [review]
> Fix compile error
> 
> Review of attachment 708874 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Same here, I think we need a patch file. And upstreaming :)

Problem doesn't exist any more upstream :)

Patch files I will commit in a separate commit when doing the final commit. Don't see the point in making them yet.

Comment 18

[Matt Woodrow (:mattwoodrow)]

(In reply to George Wright (:gw280) from comment #16)
> 
> Mainly because there needs to be a 1:1 mapping between GrContext and
> GLContext objects, and in the future I want to have a single GLContext
> backing all canvases for the entire process. It seemed to me that the
> easiest place to manage the GrContext in this case was
> CanvasRenderingContext2D.
> 
> However, thinking about it, maybe we can store the associated GrContext for
> a GLContext on the GLContext object itself? How would you feel about adding
> a GetSkiaContext() method to GLContext?


Ok, that makes sense. We have GetNativeData() on GLContext for exposing platform-specific objects. Not sure where you'd implement this though, since skia isn't specific to a single platform.

Comment 19

[George Wright (:gw280) (needinfo me!)]

Attachment: Updated DrawTarget

Comment 20

[George Wright (:gw280) (needinfo me!)]

Attachment: Updated plumbing

Comment 21

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 711640 [details] [diff] [review]
Updated plumbing

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

::: gfx/thebes/gfxPlatform.cpp
@@ +702,5 @@
>    static_cast<DrawTarget*>(aTarget)->Release();
>  }
>  
> +bool
> +gfxPlatform::UseAcceleratedCanvas()

UseGLCanvas? or similar. Since it being true means that we should create a GLContext and call CreateDrawTargetForFBO to create canvases.

Comment 22

[George Wright (:gw280) (needinfo me!)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> Comment on attachment 711640 [details] [diff] [review]
> Updated plumbing
> 
> Review of attachment 711640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxPlatform.cpp
> @@ +702,5 @@
> >    static_cast<DrawTarget*>(aTarget)->Release();
> >  }
> >  
> > +bool
> > +gfxPlatform::UseAcceleratedCanvas()
> 
> UseGLCanvas? or similar. Since it being true means that we should create a
> GLContext and call CreateDrawTargetForFBO to create canvases.

Well the function was already defined in gfxPlatformMac.h; I've simply effectively moved it into the base class (it's reimplemented in gfxPlatformMac because it only returns true on 10.7 or later).

Comment 23

[Matt Woodrow (:mattwoodrow)]

The mac version is quite different in both meaning and usage, these really shouldn't be overloads.

The mac version is used to determine if we should pass BACKEND_COREGRAPHICS or BACKEND_COREGRAPHICS_ACCELERATED to CreateDrawTarget. The 'accelerated' part is quite literal.

The version you are adding means that the caller should create their own GLContext, and then call a different function entirely (CreateDrawTargetForFBO instead of CreateDrawTarget). Accelerated is a pretty meaningless word here, except for the vague notion that drawing through GL is probably faster in most cases. It certainly doesn't describe the different code paths that will get run as a result of it being true.

Comment 24

[Matt Woodrow (:mattwoodrow)]

(Note that the mac version should probably be UseAcceleratedQuartzCanvas too)

Comment 25

[George Wright (:gw280) (needinfo me!)]

Very good point. I will have a ponder and see if I can think of a solution that makes more sense.

Comment 26

[George Wright (:gw280) (needinfo me!)]

Attachment: Define USE_SKIA globally

Comment 27

[George Wright (:gw280) (needinfo me!)]

Attachment: Updated GrGLInterface

Comment 28

[George Wright (:gw280) (needinfo me!)]

Attachment: Updated plumbing take 2

Comment 29

[(not currently active) Ted Mielczarek]

Comment on attachment 720878 [details] [diff] [review]
Define USE_SKIA globally

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

Any reason not to just push this all the way up into configure? Seems like the natural place for it.

Comment 30

[George Wright (:gw280) (needinfo me!)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #29)
> Comment on attachment 720878 [details] [diff] [review]
> Define USE_SKIA globally
> 
> Review of attachment 720878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Any reason not to just push this all the way up into configure? Seems like
> the natural place for it.

Makes sense. I'll prepare a patch.

Comment 31

[George Wright (:gw280) (needinfo me!)]

Attachment: Move USE_SKIA into configure

Comment 32

[(not currently active) Ted Mielczarek]

Comment on attachment 721378 [details] [diff] [review]
Move USE_SKIA into configure

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

::: configure.in
@@ +8284,5 @@
>  fi
>  AC_SUBST(MOZ_ENABLE_SKIA)
>  AC_SUBST(MOZ_SKIA_LIBS)
> +AC_SUBST(USE_SKIA)
> +AC_SUBST(SK_BUILD_FOR_ANDROID_NDK)

These won't have any values if you haven't set them as shell variables in configure. That being said, I'm pretty sure you don't need them, you only want the AC_DEFINEs.

Comment 33

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb533ca69d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/4553d27ccfe4
https://hg.mozilla.org/integration/mozilla-inbound/rev/044c2727949d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1bdd714cb57
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b484520fcf6
https://hg.mozilla.org/integration/mozilla-inbound/rev/44ed902f0843
https://hg.mozilla.org/integration/mozilla-inbound/rev/3282ef4a4827
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b6914fccc4f

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4b6914fccc4f
https://hg.mozilla.org/mozilla-central/rev/3282ef4a4827
https://hg.mozilla.org/mozilla-central/rev/44ed902f0843
https://hg.mozilla.org/mozilla-central/rev/0b484520fcf6
https://hg.mozilla.org/mozilla-central/rev/b1bdd714cb57
https://hg.mozilla.org/mozilla-central/rev/044c2727949d
https://hg.mozilla.org/mozilla-central/rev/4553d27ccfe4
https://hg.mozilla.org/mozilla-central/rev/8eb533ca69d7

Comment 35

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This is breaking Win64 builds:

gkmedias.exp : error LNK2001: unresolved external symbol "public: __thiscall GrGLInterface::GrGLInterface(void)" (??0GrGLInterface@@QAE@XZ)
gkmedias.exp : error LNK2001: unresolved external symbol "public: __thiscall SkString::SkString(void)" (??0SkString@@QAE@XZ)
gkmedias.exp : error LNK2001: unresolved external symbol "public: __thiscall SkString::~SkString(void)" (??1SkString@@QAE@XZ)
gkmedias.exp : error LNK2001: unresolved external symbol "public: static class GrContext * __cdecl GrContext::Create(enum GrEngine,int)" (?Create@GrContext@@SAPAV1@W4GrEngine@@H@Z)
gkmedias.exp : error LNK2001: unresolved external symbol "public: void __thiscall SkString::insert(unsigned int,char const * const)" (?insert@SkString@@QAEXIQBD@Z)
gkmedias.dll : fatal error LNK1120: 5 unresolved externals

It has me pretty confused, since, e.g., the SkString::SkString constructor is __cdecl in SkString.obj.  It doesn't have any calling convention decorations though, and the command-line options to select don't exist on x64 thus making me very confused.

Comment 36

[Joe Drew (not getting mail)]

Is it possible that win64 mangles these symbols differently than win32, or alternately it's using a different set of symbols? See layout/media/symbols.def.in for what we're exporting.

Comment 37

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/894268df2c25 for the win64 fix.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/894268df2c25