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)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: tatiana, Assigned: tatiana)

References

Details

Attachments

(1 file, 4 obsolete files)

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: nobody → tanya.meshkova
Attachment #8405050 - Flags: review?(bjacob)
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
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.
Attachment #8405054 - Flags: review?(jgilbert) → feedback?(jgilbert)
Attachment #8405050 - Flags: review?(matt.woodrow)
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-
> 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 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-
> > +    // 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
> > +            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
Attachment #8405050 - Attachment is obsolete: true
Attachment #8405050 - Flags: review?(matt.woodrow)
Attachment #8405889 - Flags: review?(bjacob)
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 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+
> IMPORTANT: in RenewSurface(), if mOwnsContext, return immediately.

I mean of course, if !mOwnsContext.
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+
Keywords: checkin-needed
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.

Attachment

General

Created:
Updated:
Size: