562409 - Implement EGL GLContextProvider

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Frederic Plourde (:fred23)]

Attachment: 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.

Comment 1

[Frederic Plourde (:fred23)]

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.

Comment 2

[Frederic Plourde (:fred23)]

Attachment: 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.

Comment 3

[Frederic Plourde (:fred23)]

Attachment: patch v.1

Please consider this one instead

Comment 4

[Bas Schouten (:bas.schouten)]

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 5

[Oleg Romashin (:romaxa)]

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...

Comment 6

[Frederic Plourde (:fred23)]

Attachment: 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

Comment 7

[Frederic Plourde (:fred23)]

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 ?

Comment 8

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

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.

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Fixed Qt port

Mostly the same version of patch, but fixed QT/GTK CreateForWindow... and removed bunch of white spaces ;)

Comment 10

[Oleg Romashin (:romaxa)]

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 11

[Oleg Romashin (:romaxa)]

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...

Comment 12

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

Oleg, can you attach your fixes/changes as a patch on top of fred's?  That way it can be reviewed/integated separately

Comment 13

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

Also the main patch needs a lot of merging for trunk -- a good part of the patch fails to apply atm.

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Interdiff Qt compile and work initially

Comment 15

[Oleg Romashin (:romaxa)]

Attachment: White spaces, indent against first patch v.2

some strange nsUnicodeRange.cpp \DEFINES += -DUSE_GLES2 line removed

Comment 16

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

Attachment: 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.

Comment 17

[Frederic Plourde (:fred23)]

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.

Comment 18

[Frederic Plourde (:fred23)]

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.

Comment 19

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

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.

Comment 20

[Frederic Plourde (:fred23)]

(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.

Comment 21

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

I've split out the layers work and folded it in to bug 567626; I'll attach just the EGL context provider here.

Comment 22

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

Attachment: EGL context provider

Here's the EGL context provider patch

Comment 23

[Oleg Romashin (:romaxa)]

Attachment: Fixed bad EGL_XXX defines, it should be LOCAL_EGL_XXX

It seems to work fine. even with Qt.

Comment 24

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

Attachment: 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.

Comment 25

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

Comment on attachment 449670 [details] [diff] [review]
EGL context provider

Whoops, forgot bas is out this week

Comment 26

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

Comment on attachment 449670 [details] [diff] [review]
EGL context provider

Bas is back!

Comment 27

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

Checked in.

http://hg.mozilla.org/mozilla-central/rev/c0b7a044a05a