Closed Bug 567626 Opened 14 years ago Closed 14 years ago

fix up opengl layers backend

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

References

Details

Attachments

(1 file, 2 obsolete files)

The OpenGL layers backend needed a little bit of love.  This provides that love, and should make us compatible with desktop OpenGL and OpenGL ES 2.0.  A bunch of rendering issues are also fixed with this patch; in particular, I can go to any web page and use the MakeGoFast extension to view a layers version of it, and it renders correctly; there were some artifacts previously, and with the EGL/GLES work in bug 562409 (on which this is based) a lot of stuff was broken.

Some changes include:

- new shader/program API, including generation of source header file from easier to edit file

- use of gfx3DMatrix throughout, instead of float arrays

- caching of uniform values to avoid unnecessary state changes

- correct VBO usage, including binding/unbinding as necessary; most common rendering data is stored in a single VBO.

- lots of helper functions, reducing boilerplate code needed to write a layer's rendering function

- correct switching between TEXTURE_2D and TEXTURE_RECTANGLE FBOs (needed shader changes)

- BGR is handled via a fragment shader (removing the need for GL_BGRA format support)

- RGBX and BGRX shaders are introduced, to handle rendering with uploaded texture data from cairo where the alpha channel should be ignored (GL's GL_RGB format is tightly packed with UNSIGNED_BYTES, and the other forms -- UNSIGNED_INT_8_8_8_8_REV etc. -- aren't supported on GLES)

- .. and possibly other stuff that I've forgotten
Attached patch patch, v1 (obsolete) — Splinter Review
Here's the patch.  There are still some problems in the TEXTURE_RECTANGLE FBO path, but they might be nvidia-related.
Assignee: nobody → vladimir
Attachment #446982 - Flags: review?
Attachment #446982 - Flags: review? → review?(bas.schouten)
Attached patch patch, v2 (obsolete) — Splinter Review
Fixed TEXTURE_RECTANGLE path -- the main bug was that I was using the wrong fragment shader (was using the 2d one instead of the 2drect one, so it was reading some random 2d texture that happened to be bound).  Also needed to explicitly enable ARB_texture_rectangle in the fragment shader -- the GLSL compiler complained unless I did that.

I also changed the program wrappers to print the shader info log, if any, even if compilation succeeded in debug builds; this showed a number of warnings that helped track things down.
Attachment #446982 - Attachment is obsolete: true
Attachment #447131 - Flags: review?
Attachment #446982 - Flags: review?(bas.schouten)
Attachment #447131 - Flags: review? → review?(bas.schouten)
Attached patch patch, v3Splinter Review
Merged to trunk; also fixed:
- canvas webgl y-flip issue;
- attempt to use EXT_bgra for DrawToTarget to optimize it a little bit.
Attachment #447131 - Attachment is obsolete: true
Attachment #447249 - Flags: review?(bas.schouten)
Attachment #447131 - Flags: review?(bas.schouten)
Comment on attachment 447249 [details] [diff] [review]
patch, v3

In general I'm really happy with this patch!

There's a bunch of pure style changes here (particularly single line if de-bracing) which change the style to go against the Style guide. And the style in the rest of the module, personally I'm not a big fan if this, consistency is important, if we think single line ifs should generally not have braces, we should change the guide. This is confusing.

Then some small comments.

>-  gfxRGBA color = mColor;
>-  // color is premultiplied, so we need to adjust all channels
>-  color.r *= GetOpacity();
>-  color.g *= GetOpacity();
>-  color.b *= GetOpacity();
>-  color.a *= GetOpacity();
>-  program->SetLayerColor(color);
>-  program->SetLayerTransform(&mTransform._11);
>-  program->Apply();
>-
>-  gl()->fDrawArrays(LOCAL_GL_TRIANGLE_STRIP, 0, 4);

There's a reason we pre-multiply here, using this method we spend a 4 component vector multiplication on each pixel that we don't need to (i.e. the result is constant). There's no way for the shaders to optimize this. This 4 component vector multiplication isn't that bad for good graphics hardware, but for budget hardware with less stream processors or mobile devices it's quite a burden. (on the N900 a single float4 multiplication for 1 single fullscreen quad significantly knocked performance). Some extra instructions in the vertex shaders should be fine since we hardly have to process any vertices, but with fragment shaders we should be careful. I'm planning to add a specific shader to the D3D9 layers implementation for opaque ThebesLayers as well.

>+    program->Activate();
>+    program->SetLayerQuadTransform(nsIntRect(0.0f, 0.0f,
>+                                             yuvImage->mSize.width,
>+                                             yuvImage->mSize.height));
>+    program->SetLayerTransform(&mTransform._11);

Maybe we can use gfxMatrix3D here too? I quite like the usage, if only for consistency.

>+#ifdef DEBUG_vladimir
>+      fprintf (stderr, "---- FBO: using 0x%04x\n", target);
>+#endif

Do we want to leave this in there?

>+#if 0
>+  // XXX for whatever reason, scissor is not working -- even with no
>+  // cliprect set, so we go through the 0,0,w,h path, any updates
>+  // after the initial render end up failing the scissor rectangle.  I
>+  // have no idea why.  We disable it for now, because it's not actually
>+  // helping us with anything -- we draw to a specific location in the
>+  // front buffer as it is.
How about with container layers? Which may expect the clipping areas of their children to be respected...

>+  if (!mHasBGRA) {
>+    // need to swap B and R bytes
>+    for (int j = 0; j < height; ++j) {
>+      PRUint32 *row = (PRUint32*) (imageSurface->Data() + imageSurface->Stride() * j);
>+      for (int i = 0; i < width; ++i) {
>+        *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
>+        row++;
>+      }
>+    }
>+  }

Ugh, we could probably really optimize this with some SIMD love, not too important for now though!

>+      nsCAutoString log;
>+      log.SetCapacity(len);
>+      mGL->fGetShaderInfoLog(sh, len, (GLint*) &len, (char*) log.BeginWriting());
>+      log.SetLength(len);
>+
>+      if (!success) {
>+        fprintf (stderr, "=== SHADER COMPILATION FAILED ===\n");
>+      } else {
>+        fprintf (stderr, "=== SHADER COMPILATION WARNINGS ===\n");
>+      }
>+
>+        fprintf (stderr, "=== Source:\n%s\n", aShaderSource);
>+        fprintf (stderr, "=== Log:\n%s\n", nsPromiseFlatCString(log).get());
>+        fprintf (stderr, "============\n");

We should probably log this somewhere else than stderr. Also on my ATI this is currently -very- spammy. It always outputs big blocks of text with SHADER COMPILATION WARNINGS, with the source and something like 'Fragement shader was succesfully compiled to run on hardware. (which isn't even a warning!' :)
>+      nsCAutoString log;
>+      log.SetCapacity(len);
>+      mGL->fGetProgramInfoLog(mProgram, len, (GLint*) &len, (char*) log.BeginWriting());
>+      log.SetLength(len);
>+
>+      if (!success) {
>+        fprintf (stderr, "=== PROGRAM LINKING FAILED ===\n");
>+      } else {
>+        fprintf (stderr, "=== PROGRAM LINKING WARNINGS ===\n");
>+      }
>+      fprintf (stderr, "=== Log:\n%s\n", nsPromiseFlatCString(log).get());
>+      fprintf (stderr, "============\n");

Spammy too! This time the log says 'Fragment shader(s) linked, vertex shader(s) linked.
>+  void SetLayerQuadTransform(const float *aMatrix) {
>+    SetMatrixUniform(mUniformLocations[QuadTransformUniform], aMatrix);
>+  }
>+
>+  void SetLayerQuadTransform(const nsIntRect& aRect) {

I'm not completely happy with this function signature, I think we should decorate it some more to make it more verbose about the magic it's doing, or at least document it. Considering a rect isn't really a transform by itself :)

>+/* This should not be used on GL ES */
>+#ifndef GL_ES
>+uniform sampler2DRect uTexture;
>+uniform vec2 uTexCoordMultiplier;
>+void main()
>+{
>+  gl_FragColor = texture2DRect(uTexture, vec2(vTexCoord * uTexCoordMultiplier)) * uLayerOpacity;

I wonder if we should maybe premultiply the texcoords, again with devices with poor fragment shader capabilities in mind.
Attachment #447249 - Flags: review?(bas.schouten) → review+
http://hg.mozilla.org/mozilla-central/rev/683e22968074 \o/
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
I think this change is what caused bug 567095.
Pushed a followup to fix some firstline comments that were accidentally broken by this bug's patch: http://hg.mozilla.org/mozilla-central/rev/f70a48308703
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: