Enable OpenGL acceleration on Skia

RESOLVED FIXED in mozilla22

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: marco, Assigned: gw280)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 5 obsolete attachments)

5.31 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
788 bytes, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1003 bytes, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.58 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
29.32 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.30 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.29 KB, patch
ted
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Enable by default OpenGL acceleration on Skia.
It makes sense to wait for the rebase before starting this.
Depends on: 755869
(Reporter)

Updated

5 years ago
Depends on: 777614
(Assignee)

Updated

5 years ago
Assignee: nobody → gwright
(Assignee)

Updated

5 years ago
Depends on: 814174
https://hg.mozilla.org/mozilla-central/rev/8fdd86263b2f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Reporter)

Comment 3

5 years ago
Ed, I think that patch isn't related to this bug.
My bad. It should be https://bugzilla.mozilla.org/show_bug.cgi?id=814174. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 708871 [details] [diff] [review]
Build Skia with GPU support
Attachment #708871 - Flags: review?(matt.woodrow)
Created attachment 708872 [details] [diff] [review]
Add our own GrUserConfig
Attachment #708872 - Flags: review?(matt.woodrow)
Created attachment 708874 [details] [diff] [review]
Fix compile error
Attachment #708874 - Flags: review?(matt.woodrow)
Created attachment 708876 [details] [diff] [review]
GrGLInterface for mozilla's GLContext
Attachment #708876 - Flags: review?(matt.woodrow)
Created attachment 708878 [details] [diff] [review]
Support Ganesh in DrawTargetSkia
Attachment #708878 - Flags: review?(matt.woodrow)
Created attachment 708879 [details] [diff] [review]
Plumb through GL-backed CanvasLayers for Skia
Attachment #708879 - Flags: review?(ncameron)
Attachment #708871 - Flags: review?(matt.woodrow) → review-
Attachment #708871 - Flags: review- → review+
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?
Attachment #708872 - Flags: review?(matt.woodrow) → review+
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 :)
Attachment #708874 - Flags: review?(matt.woodrow) → review+
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.
Attachment #708876 - Flags: review?(matt.woodrow) → review+
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

4 years ago
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.
Attachment #708879 - Flags: review?(ncameron)
(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?
(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.
Attachment #708878 - Flags: review?(matt.woodrow) → review+
(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.
Created attachment 711639 [details] [diff] [review]
Updated DrawTarget
Attachment #708878 - Attachment is obsolete: true
Attachment #711639 - Flags: review?(matt.woodrow)
Created attachment 711640 [details] [diff] [review]
Updated plumbing
Attachment #708879 - Attachment is obsolete: true
Attachment #711640 - Flags: review?(matt.woodrow)
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.
Attachment #711640 - Flags: review?(matt.woodrow) → review+
Attachment #711639 - Flags: review?(matt.woodrow) → review+
(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).
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.
(Note that the mac version should probably be UseAcceleratedQuartzCanvas too)
Very good point. I will have a ponder and see if I can think of a solution that makes more sense.
(Assignee)

Updated

4 years ago
Depends on: 807500
Blocks: 845200, 845202
Created attachment 720878 [details] [diff] [review]
Define USE_SKIA globally
Attachment #720878 - Flags: review?(khuey)
Created attachment 720879 [details] [diff] [review]
Updated GrGLInterface
Attachment #708876 - Attachment is obsolete: true
Attachment #720879 - Flags: review?(matt.woodrow)
Created attachment 720882 [details] [diff] [review]
Updated plumbing take 2
Attachment #711640 - Attachment is obsolete: true
Attachment #720882 - Flags: review?(matt.woodrow)
Attachment #720879 - Flags: review?(matt.woodrow) → review+
Attachment #720882 - Flags: review?(matt.woodrow) → review+
(Assignee)

Updated

4 years ago
Attachment #720878 - Flags: review?(khuey) → review?(ted)
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.
(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.
Created attachment 721378 [details] [diff] [review]
Move USE_SKIA into configure
Attachment #720878 - Attachment is obsolete: true
Attachment #720878 - Flags: review?(ted)
Attachment #721378 - Flags: review?(ted)
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.
Attachment #721378 - Flags: review?(ted) → review+
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
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
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
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.
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.
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/894268df2c25 for the win64 fix.
https://hg.mozilla.org/mozilla-central/rev/894268df2c25

Updated

4 years ago
Depends on: 849207
Blocks: 849253
Target Milestone: mozilla20 → mozilla22
You need to log in before you can comment on or make changes to this bug.