Closed
Bug 994856
Opened 10 years ago
Closed 10 years ago
Add support for Gecko rendering via GLScreenBuffer into external GL Context
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: tatiana, Assigned: tatiana)
References
Details
Attachments
(1 file, 4 obsolete files)
14.95 KB,
patch
|
tatiana
:
review+
|
Details | Diff | Splinter Review |
For Embedlite Rendering we decided to use GLScreenBuffer and make Main Gecko CompositorOGL render into texture stream so Native Toolkit UI (Qt/Gtk/...) can grab textures and render them. In order to implement that we should: 1) provide GLProvider interface for Wrapping external GL context into Gecko GLContext, so we can pass that into SharedSurface_EGLImage::AcquireConsumerTexture/SharedSurface_GLTexture::ConsTexture API 2) Provide way to initialize Offscreen GLContext in CompositorOGL and Fix viewport Y-Flip 3) Make GLScreenBuffer::Create do not build SurfaceFactory_Basic by default but create SurfaceFactory_EGLImage or SurfaceFactory_GLTexture if it possible.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → tanya.meshkova
Attachment #8405050 -
Flags: review?(bjacob)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8405054 -
Flags: review?(jgilbert)
Assignee | ||
Comment 3•10 years ago
|
||
Here is try build with both patches applied https://tbpl.mozilla.org/?tree=Try&rev=985cbade6ce0 I see some strange issues Assertion failure: alignment, at /builds/slave/try-l64-d-00000000000000000000/build/gfx/gl/GLReadTexImageHelper.cpp:323 Will try to figure out why it happens
Assignee | ||
Comment 4•10 years ago
|
||
Ok looks like Compositor patch does break some tests gl interface does not break anything https://tbpl.mozilla.org/?tree=Try&rev=f0d50df51cc2 Probably some tests are not expecting that GLTexture factory created instead of Basic.
Assignee | ||
Updated•10 years ago
|
Attachment #8405054 -
Flags: review?(jgilbert) → feedback?(jgilbert)
Assignee | ||
Updated•10 years ago
|
Attachment #8405050 -
Flags: review?(matt.woodrow)
Comment 5•10 years ago
|
||
Comment on attachment 8405054 [details] [diff] [review] Compositor Update which allow to render into GLScreenBuffer Review of attachment 8405054 [details] [diff] [review]: ----------------------------------------------------------------- It's really hard to take patches like this, since we have no visibility into the use of the APIs we're setting up here in this patch. Without a semi-stable API, (even if the API is just a documented usecase) we can't guarantee this code will be stable. For instance, since we have no usecase where the compositor GLContext is offscreen, so it will look like this is unnecessary dead code, which will then be removed. ::: gfx/gl/GLScreenBuffer.cpp @@ +61,5 @@ > + if (gl->GetContextType() == GLContextType::EGL) { > + bool isCrossProcess = !(XRE_GetProcessType() == GeckoProcessType_Default); > + if (!isCrossProcess) { > + // [Basic/OGL Layers, OMTC] WebGL layer init. > + factory = SurfaceFactory_EGLImage::Create(gl, caps); I would really like to avoid this. We also use EGL for ANGLE on Windows, so hardcoding an expectation of using EGLImages is wrong for the majority of users. EGL may also eventually be our platform of choice for not just mobile and ANGLE, but also windows+nativeGL and linux, where available. We already switch to EGLImage as soon as we're sure we can use it. Why is hard-coding this assumption here needed? ::: gfx/layers/opengl/CompositorOGL.cpp @@ +514,5 @@ > > // Matrix to transform (0, 0, aWidth, aHeight) to viewport space (-1.0, 1.0, > // 2, 2) and flip the contents. > Matrix viewMatrix; > + viewMatrix.Translate(-1.0, mGLContext->IsOffscreen() ? -1.0 : 1.0); Split this out into a `bool shouldYFlip`.
Attachment #8405054 -
Flags: feedback?(jgilbert) → feedback-
Assignee | ||
Comment 6•10 years ago
|
||
> It's really hard to take patches like this, since we have no visibility into > the use of the APIs we're setting up here in this patch. Without a Real use-case for this API will be embedding API's which we are going to land later, and pretty much with gtest. https://github.com/tmeshkova/gecko-dev/tree/embedlite/embedding/embedlite current user of this API is http://blog.idempotent.info/posts/whats-behind-sailfish-browser.html
Comment 7•10 years ago
|
||
Comment on attachment 8405050 [details] [diff] [review] New API to GL Providers which allow to wrap existing context Review of attachment 8405050 [details] [diff] [review]: ----------------------------------------------------------------- We may need a second pass here if only to 1) make sure we've minimized intrusiveness (see the CGL and WGL comments) and 2) make sure we understand what mOwnsContext really means (see SwapBuffers comment). ::: gfx/gl/GLContextProviderCGL.mm @@ +189,5 @@ > return static_cast<GLContextCGL*>(GLContextProviderCGL::GetGlobalContext()); > } > > already_AddRefed<GLContext> > +GLContextProviderCGL::CreateWrappingExisting(void* aContext, void* aSurface) Since you don't need CGL, just leave this unimplemented (return null). ::: gfx/gl/GLContextProviderEGL.cpp @@ +258,5 @@ > GLContextEGL::~GLContextEGL() > { > MarkDestroyed(); > > + // Wrapping GLContext cannot be destroyed "Wrapped OpenGL context should not be destroyed" ? We're still in a destructor, here. @@ +276,5 @@ > GLContextEGL::Init() > { > + if (mInitialized) { > + return true; > + } Why this change? It is nontrivial as mInitialized is set far away from here and not otherwise referenced in this file. @@ +443,5 @@ > > bool > GLContextEGL::SwapBuffers() > { > + if (mSurface && mOwnsContext) { I don't understand this change. It looks like mOwnsContext actually means something else than "owns context". ::: gfx/gl/GLContextProviderWGL.cpp @@ +400,5 @@ > return static_cast<GLContextWGL*>(GLContextProviderWGL::GetGlobalContext()); > } > > already_AddRefed<GLContext> > +GLContextProviderWGL::CreateWrappingExisting(void* aContext, void* aSurface) Since you don't need WGL, just leave this unimplemented (return null).
Attachment #8405050 -
Flags: review?(bjacob) → review-
Assignee | ||
Comment 8•10 years ago
|
||
> > + // Wrapping GLContext cannot be destroyed yep comment is wrong > > + if (mSurface && mOwnsContext) { yep, not this is not important, it was done before for external context which had AutoSwap behavior, but for now it is not needed because we don't use this wrapping context as Primary Gecko rendering context
Assignee | ||
Comment 9•10 years ago
|
||
> > + factory = SurfaceFactory_EGLImage::Create(gl, caps);
>
> I would really like to avoid this. We also use EGL for ANGLE on Windows, so
Problem here is that new created Screen buffer give us SurfaceFactory_Basic and GL texture generated by ReadPixels call, and that is not very good
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8405050 -
Attachment is obsolete: true
Attachment #8405050 -
Flags: review?(matt.woodrow)
Attachment #8405889 -
Flags: review?(bjacob)
Assignee | ||
Comment 11•10 years ago
|
||
Sounds like I can do Morph on Embedding side, so no need to do it here.
Attachment #8405054 -
Attachment is obsolete: true
Attachment #8405893 -
Flags: review?(jgilbert)
Comment 12•10 years ago
|
||
Comment on attachment 8405889 [details] [diff] [review] New API to GL Providers which allow to wrap existing context Review of attachment 8405889 [details] [diff] [review]: ----------------------------------------------------------------- R+, with some comments, one of them (about RenewSurface) really important: ::: gfx/gl/GLContextProviderCGL.mm @@ +190,5 @@ > } > > already_AddRefed<GLContext> > +GLContextProviderCGL::CreateWrappingExisting(void* aContext, void* aSurface) > +{ To avoid compiler warnings about unused function parameters, don't name the function parameters that you don't use in the function body. ::: gfx/gl/GLContextProviderEGL.cpp @@ +437,5 @@ > void > GLContextEGL::ReleaseSurface() { > + if (mOwnsContext) { > + DestroySurface(mSurface); > + } IMPORTANT: in RenewSurface(), if mOwnsContext, return immediately. Indeed, since mOwnsContext causes ReleaseSurface to do nothing, it seems important to have RenewSurface also do nothing, otherwise we'd be silently leaking surfaces. ::: gfx/gl/GLContextProviderWGL.cpp @@ +401,5 @@ > } > > already_AddRefed<GLContext> > +GLContextProviderWGL::CreateWrappingExisting(void* aContext, void* aSurface) > +{ Same remark about unused parameters as in the CGL case.
Attachment #8405889 -
Flags: review?(bjacob) → review+
Comment 13•10 years ago
|
||
> IMPORTANT: in RenewSurface(), if mOwnsContext, return immediately.
I mean of course, if !mOwnsContext.
Assignee | ||
Comment 14•10 years ago
|
||
Ok adding updated version, https://tbpl.mozilla.org/?tree=Try&rev=1607619f564c Compositor patch need some more work to do, I'll move it into another followup bug.
Attachment #8405889 -
Attachment is obsolete: true
Attachment #8405893 -
Attachment is obsolete: true
Attachment #8405893 -
Flags: review?(jgilbert)
Attachment #8407163 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d7dd6086a5a
Keywords: checkin-needed
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5d7dd6086a5a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•