Implement EGL GLContextProvider

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: fred23, Assigned: fred23)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 4 obsolete attachments)

(Assignee)

Description

8 years ago
Created attachment 442137 [details] [diff] [review]
w.i.p. patch

We'd like an GLContextProviderEGL implementation for layers­.
attached is a w.i.p patch for that.
For now, it implies GTK2 toolkit, but really, for maemo's sake, we outta add QT support ASAP.
(Assignee)

Comment 1

8 years ago
oh, by the way guys, don't try to build with my patch applied, it doesn't. I'm fixing build problems as we speak.
(Assignee)

Updated

8 years ago
Assignee: nobody → bugzillaFred
(Assignee)

Comment 2

8 years ago
Created attachment 444278 [details] [diff] [review]
patch v.1

Here's a working patch for Layers port to EGL with GLES2.0
Notice however that : 

1) It's still including only one renderShader (I'm not swizzling BGRA into RGBA yet). I'll post another patch with two shaders... BGRARenderShader and RGBARenderShader

2) TexEnv:GL_MODULATE is not yet included in the render shader.

I'd be cool if you could test it and give me some feedback. On my N900, fennec seemed to render correctly (chrome and contents), though I have not tested extensively with a lot of URIs.
Attachment #442137 - Attachment is obsolete: true
(Assignee)

Comment 3

8 years ago
Created attachment 444279 [details] [diff] [review]
patch v.1

Please consider this one instead
Attachment #444278 - Attachment is obsolete: true
Comment on attachment 444279 [details] [diff] [review]
patch v.1

I had a quick look at the patch, looks good! Here's some quick comments so far on the LayerManagerOGL modifications.

>@@ -390,7 +488,10 @@
>    * Setup the texture used as the backbuffer.
>    */
>   mGLContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mBackBuffer);
>+#ifndef USE_GLES2
>+  // TODO : apply GL_MODULATE from inside the shader
>   mGLContext->fTexEnvf(LOCAL_GL_TEXTURE_ENV, LOCAL_GL_TEXTURE_ENV_MODE, LOCAL_GL_MODULATE);
>+#endif

We should just be able to remove this, our shaders already do this correctly I think.

>+#ifdef USE_GLES2
>+  // GLES2 promises that binding to any custom FBO will attach 
>+  // to GL_COLOR_ATTACHMENT0 attachment point.
>+  mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, mFrameBuffer);
>+
>+  GLenum err = mGLContext->fCheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER);
>+  if (err != LOCAL_GL_FRAMEBUFFER_COMPLETE) {
>+    return;
>+  }
Wouldn't this require GL_READ_FRAMEBUFFER? Maybe it doesn't, just thinking.

>@@ -518,6 +634,8 @@
>   mGLContext->fAttachShader(mProgram, aFragmentShader);
> 
>   mGLContext->fBindAttribLocation(mProgram, VERTEX_ATTRIB_LOCATION, "aVertex");
>+  if (bindTexCoords)
>+      mGLContext->fBindAttribLocation(mProgram, TEXCOORDS_ATTRIB_LOCATION, "aTexCoords");

I'd say we make LayerProgram::Initialize virtual and bind this in the RenderProgram initialize function.
>+static const GLchar *sRenderVertexShader = RENDER_SHADER_GLOBAL_VARS "\
>+attribute highp vec4 aVertex; \
>+attribute highp vec2 aTexCoords; \
>+void main() \
> { \
>-gl_FragColor = texture2D(uLayerTexture, vTextureCoordinate) * uLayerOpacity; \
>+    gl_Position = uMatrixProj * aVertex; \
>+    vTextureCoordinate = aTexCoords; \
> }";

Strange, why do we have to multiply by uMatrixProj here? You seem to pass in:

+      mGLContext->fVertexAttribPointer(VERTEX_ATTRIB_LOCATION, 2, 
+          LOCAL_GL_FLOAT, LOCAL_GL_FALSE, 0, vertices);

Which should contain the viewport coordinates already, and not require an orthogonal projection.
Comment on attachment 444279 [details] [diff] [review]
patch v.1


>+
>+#if defined(MOZ_X11) && defined(MOZ_PLATFORM_MAEMO)
I guess this should be inside MOZ_WIDGET_GTK2...
>+    GdkWindow *win = (GdkWindow *) aWidget->GetNativeData(NS_NATIVE_WINDOW);
>+
>+    display = sEGLLibrary.fGetDisplay((EGLNativeDisplayType)GDK_WINDOW_XDISPLAY(win));

I think you can use NS_NATIVE_DISPLAY here to get widget specific display... and then you don't need any defines at all.



>-		nsUnicodeRange.cpp \
>+		nsUnicodeRange.cpp \DEFINES += -DUSE_GLES2
> 		$(NULL)
something wrong with defines here...
(Assignee)

Comment 6

8 years ago
Created attachment 445186 [details] [diff] [review]
patch v.2

I've reworked the patch per Bas and Romaxa's comments :

(In reply to comment #4)
> (From update of attachment 444279 [details] [diff] [review])
> >   mGLContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mBackBuffer);
> >+#ifndef USE_GLES2
> >+  // TODO : apply GL_MODULATE from inside the shader
> >   mGLContext->fTexEnvf(LOCAL_GL_TEXTURE_ENV, LOCAL_GL_TEXTURE_ENV_MODE, LOCAL_GL_MODULATE);
> >+#endif
> 
> We should just be able to remove this, our shaders already do this correctly I
> think.

Done.

> 
> >+#ifdef USE_GLES2
> >+  // GLES2 promises that binding to any custom FBO will attach 
> >+  // to GL_COLOR_ATTACHMENT0 attachment point.
> >+  mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, mFrameBuffer);
> >+
> >+  GLenum err = mGLContext->fCheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER);
> >+  if (err != LOCAL_GL_FRAMEBUFFER_COMPLETE) {
> >+    return;
> >+  }
> Wouldn't this require GL_READ_FRAMEBUFFER? Maybe it doesn't, just thinking.

Really, I went through GLES2.0 spec and since fReadBuffer doesn't exist, 
they really state that :

      "glReadPixels :

            If the currently bound framebuffer is not the default framebuffer object, color
            components are read from the color image attached to the
            GL_COLOR_ATTACHMENT0 attachment point."

So I think I'm going to leave that there for now.
        
> >@@ -518,6 +634,8 @@
> >   mGLContext->fAttachShader(mProgram, aFragmentShader);
> > 
> >   mGLContext->fBindAttribLocation(mProgram, VERTEX_ATTRIB_LOCATION, "aVertex");
> >+  if (bindTexCoords)
> >+      mGLContext->fBindAttribLocation(mProgram, TEXCOORDS_ATTRIB_LOCATION, "aTexCoords");
> 
> I'd say we make LayerProgram::Initialize virtual and bind this in the
> RenderProgram initialize function.

Yeah, better OOP ;-)
Done.

> >+static const GLchar *sRenderVertexShader = RENDER_SHADER_GLOBAL_VARS "\
> >+attribute highp vec4 aVertex; \
> >+attribute highp vec2 aTexCoords; \
> >+void main() \
> > { \
> >-gl_FragColor = texture2D(uLayerTexture, vTextureCoordinate) * uLayerOpacity; \
> >+    gl_Position = uMatrixProj * aVertex; \
> >+    vTextureCoordinate = aTexCoords; \
> > }";
> 
> Strange, why do we have to multiply by uMatrixProj here? You seem to pass in:
> 
> +      mGLContext->fVertexAttribPointer(VERTEX_ATTRIB_LOCATION, 2, 
> +          LOCAL_GL_FLOAT, LOCAL_GL_FALSE, 0, vertices);
> 
> Which should contain the viewport coordinates already, and not require an
> orthogonal projection.

yeah, I think you're right. But since I'm stuck with on-device fennec and no real draw-to-screen because of mTarget, I can't test it.
So here it is, Done for now.

I've also addressed both Oleg's comments.
Now in ContextProviderEGL.cpp, the use of 
GET_NATIVE_DISPLAY()  and
GET_NATIVE_WINDOW() drops our need for #defines 
in main function body. You can take a look and tell me what you think Oleg ?
thanks


The swizzling shaders are there, but I'm not using them yet, since I'm not yet sure of which surfaces *do* use BGRA (I must admit I'm less familiar with that Thebes part of the code). I'll rework on Monday with your comments and will probably pass in on to someone else for gfxSurface-specific stuff.

thanks
Attachment #444279 - Attachment is obsolete: true
Attachment #445186 - Flags: review?
Attachment #445186 - Flags: feedback?(romaxa)
(Assignee)

Comment 7

8 years ago
Vlad: from our quick chat from yesterday on #gfx, I understand you're blocked on me. I guess you can start working on your stuff from what I just posted (patch v2) and I'll sync myself on you as soon as I come back from the weekend. How does that sound ?
No worries; I'm not significantly blocked on your stuff, just something that I wanted to do would end up depending on some of the cleanup work you've done.  It can wait until the EGL stuff lands.
Created attachment 445288 [details] [diff] [review]
Fixed Qt port

Mostly the same version of patch, but fixed QT/GTK CreateForWindow... and removed bunch of white spaces ;)
In particular case it seems to work.
But I've found one problem...
We are trying to initialize GL context (CreateForWindow) multiple times for the different aWidget but with the same XWindow..

Qt-port is organized such way that QGraphicsView (with XWindow) hosting multiple QGraphicsWidgets (each wrapped by nsIWidget)...

Is it possible to detect existing GL wrapper for XWindow and just return existing glContext?
Comment on attachment 445186 [details] [diff] [review]
patch v.2

Seems to work for me, except problem with multiple createForWindow...

But I think it should be fixed somewhere else...
Attachment #445186 - Flags: feedback?(romaxa) → feedback+

Updated

8 years ago
Attachment #445186 - Flags: review? → review?(bas.schouten)
Oleg, can you attach your fixes/changes as a patch on top of fred's?  That way it can be reviewed/integated separately
Also the main patch needs a lot of merging for trunk -- a good part of the patch fails to apply atm.
Created attachment 446449 [details] [diff] [review]
Interdiff Qt compile and work initially
Attachment #446449 - Attachment description: Interdiff → Interdiff Qt compile and work initially
Attachment #446449 - Attachment filename: interdiff.diff → interdiff.diff
Created attachment 446450 [details] [diff] [review]
White spaces, indent against first patch v.2

some strange nsUnicodeRange.cpp \DEFINES += -DUSE_GLES2 line removed
Created attachment 446454 [details] [diff] [review]
updated to trunk

Ok, this is the original patch + Oleg's Qt patches, updated to the trunk.  It builds and runs, at least on windows.  It causes flipped RGB colors on windows, though.  I need to do some stuff to the OGL layers backend, and I'll use this as a base -- Fred, if possible, can you use this patch as a base for further tweaks of the EGL backend?  If you've done work beyond what was posted here, you should be able to generate an interdiff and continue from there.  Will make it easier to manage merging down the line.
(Assignee)

Comment 17

8 years ago
Yeah, no problem Vlad, I'll start from this patch too. And thanks for the update!

For the flipped RGB issue, this is completely expected for now, because even if the patch I submitted had the "swizzled" shaders in it, it was *not* making use of them... The reason for that is simple : I couldn't test on device because of the "fennec-is-not-drawing-on-screen" issue, so I left it out for now.

If you ever happen to have free cycles to play with that, correcting this should be pretty simple, you'd only have to write: 

  mSwRGBLayerProgram->Activate() instead of mRGBLayerProgram->Activate()
  mSwYCbCrLayerProgram->Activate() instead of mYCbCrLayerProgram->Activate()

... to all the relevant places.
(Assignee)

Comment 18

8 years ago
Just to be sure you guys don't think I'm letting you down on this.
As of today (2010-05-20), my wife is 5 days late and the probability that we have our child within 3-4 days is really, really high. 

So since the EGL port basis is mainly done, building and running ok on both windows and maemo 5, and since I've got other stuff to finish for fennecko and e10s, that's just as important as this, I decided to put EGL on ice for now.

So if you've got any urgent need for an updated EGL backend in the following days, please, do assign this bug to yourself or someone else and get this piece of work moving forward :-)

thanks.
You actually don't need SwYCbCr stuff at all -- the swizzle happens due to the -incoming- data, and YUV data is never going to be in the wrong byte order.  Also, you should really make sure that everything works on the desktop first, ignoring Fennec entirely; as is, the patch currently breaks fullscreen video on the desktop.  I'll take a look into why.
(Assignee)

Comment 20

8 years ago
(In reply to comment #19)
> You actually don't need SwYCbCr stuff at all -- the swizzle happens due to the
> -incoming- data, and YUV data is never going to be in the wrong byte order. 
ah, got it...wasn't sure about this one. thanks.

> Also, you should really make sure that everything works on the desktop first,
> ignoring Fennec entirely;
ok, I hadn't developed/tested on desktop initially because I had honestly no idea of a good way to drive EGL/openGL ES2.0 under my current Windows platform. What's the best way, actually, on desktop ?  I've got Windows and a Virtual Ubuntu Karmic under VirtualBox. I've read there's an openGLES2.0 emulator under Windows... is that any good ?

> as is, the patch currently breaks fullscreen video on
> the desktop.  I'll take a look into why.
I hate giving other people extra work... And I thank you for that; I'll try to take a look too, but as I told #gfx people yesterday, my wife is going to deliver pretty soon, so it's possible that this work gets done a little bit later than expected, which is annoying I admit, but I really have limited control over that, we'll see. Thanks for your understanding.
I've split out the layers work and folded it in to bug 567626; I'll attach just the EGL context provider here.
Depends on: 567626
Created attachment 446979 [details] [diff] [review]
EGL context provider

Here's the EGL context provider patch

Updated

8 years ago
Attachment #445186 - Flags: review?(bas.schouten)
Created attachment 449243 [details] [diff] [review]
Fixed bad EGL_XXX defines, it should be LOCAL_EGL_XXX

It seems to work fine. even with Qt.
Attachment #446979 - Attachment is obsolete: true
Created attachment 449670 [details] [diff] [review]
EGL context provider

Ok, here's a slightly updated patch from oleg's earlier one; adds some support for pbuffers.  This works for WebGL at least, but I think we should get this checked in and iterate off of it.
Attachment #449670 - Flags: review?
Attachment #449670 - Flags: review? → review?(bas.schouten)
Attachment #449670 - Flags: review?(bas.schouten) → review?(joe)
Comment on attachment 449670 [details] [diff] [review]
EGL context provider

Whoops, forgot bas is out this week
Summary: Port Layers to EGL → Implement EGL GLContextProvider
Comment on attachment 449670 [details] [diff] [review]
EGL context provider

Bas is back!
Attachment #449670 - Flags: review?(joe) → review?(bas.schouten)
Attachment #449670 - Flags: review?(bas.schouten) → review+
Checked in.

http://hg.mozilla.org/mozilla-central/rev/c0b7a044a05a
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.