573829 - Retain ThebesLayer contents in OpenGL layers backend

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Refactor ThebesLayerBuffer in preparation for its use in the OGL layers backend

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Retain ThebesLayer contents in OGL layers backend

Vlad, you seem like the best reviewer of this, but please pass off if appropriate.

Note that this patch draws the buffer contents only in a "seeing The Matrix" kind of way; it doesn't apply the rotation to the ThebesLayerBuffer.  (Kinda neat, but useless.)  Next patch fixes that.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I should add that these patches always re-upload the entire retained buffer with glTexImage2D() after invalidations.  This should be faster than the old code, but isn't optimal on X11+GLX/EGL at least.  Optimizations in followups, please.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> This should be faster than the old code

For scrolling, I meant to qualify.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #4)
Yes, on Mac it's definitely faster to use glTexSubimage2D to update textures after the initial glTexImage2D.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 453177 [details] [diff] [review]
part 1: Refactor ThebesLayerBuffer in preparation for its use in the OGL layers backend

+  ThebesLayerBuffer(PRBool aExactlySized=PR_FALSE)

use an enum parameter, and no default value

+  bool isOpaqueContent =
+    (targetSurface->AreSimilarSurfacesSensitiveToContentType() &&

PRBool

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I started to file a followup for optimizing this code on platforms where we'd want to treat the texture as the backing storage for ThebesLayerBuffer (and update using temporary surfaces and TexSubImage2d()), but that's not high priority at the moment.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Retain ThebesLayer contents in OGL layers backend

The new patches forthcoming fix a rendering glitch caused by not binding the ThebesLayerOGL's texture for rendering when it wasn't updated with new content.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly

Comment 11

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

Comment on attachment 453313 [details] [diff] [review]
part 3: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly

So this is generally fine, except don't add the stuff to LayerManagerOGL; just put it in ThebesLayer directly.  That is:


>+  void UnbindQuadVBO() {
>+    mGLContext->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, 0);
>+  }

Just call BindBuffer(, ..0) directly where you need it.  The call doesn't unbind the quad vbo specifically, it unbinds any VBO, so the naming would be weird.


>+  void BindAndDrawQuadWithTextureRect(LayerProgram *aProg,
>+                                      const nsIntRect& aTexCoordRect);

And this, just put the code in ThebesLayer; if we end up needing to pull it out later let's do it, but there's no point in having lots of specialized helpers that only have consumer.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly, v2

Moved single-user code out of LayerManagerOGL and into ThebesLayerOGl.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly, v2

(Oops, uploaded the wrong patch.)

Comment 14

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

Comment on attachment 453540 [details] [diff] [review]
 part 3: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly, v2 

Great, thanks!

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Use ThebesLayerBuffer to retain contents for ASurface-backed TextureImages

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1.5: Add TextureImage API for getting backing ASurface (if exists) and determining whether it's in an update

Oops, almost forgot this one.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Add TextureImage API for getting backing ASurface (if exists) and determining whether it's in an update

part 1 will land independently of >= 2 because it made the cross-process layers implementation cleaner.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Use ThebesLayerBuffer to retain contents for ASurface-backed TextureImages

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 4: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I incorporated the fixes from bug 579696, but there's still a problem with google.com's fade-in animation that's also present with m-c tip, so is apparently a bug elsewhere.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 453177 [details] [diff] [review]
part 1: Refactor ThebesLayerBuffer in preparation for its use in the OGL layers backend

Requesting approval (just on this patch, on purpose) because it refactors ThebesLayerBuffer so that it can be used more easily by both the cross-process layers code and the GL layers work here.  Not taking it will cause major merge headaches later between ThebesLayerBuffer fixes/cross-process layers/GL layers.  It's a safe refactoring.

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Tentatively blocking because we may suffer a perf regression from software rendering without retaining thebes content.

Comment 24

[Joe Drew (not getting mail)]

Comment on attachment 453177 [details] [diff] [review]
part 1: Refactor ThebesLayerBuffer in preparation for its use in the OGL layers backend

Doesn't need approval - this is a blocker.

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

part 1: http://hg.mozilla.org/mozilla-central/rev/432bca3dd7ba

Comment 26

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

Comment on attachment 458591 [details] [diff] [review]
part 4: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly

looks good!

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/8139e2e2d216
http://hg.mozilla.org/mozilla-central/rev/f4d3421c4702
http://hg.mozilla.org/mozilla-central/rev/da66f4dd97cf

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

In

  TextureImage::ContentType contentType =
+    CanUseOpaqueSurface() ? gfxASurface::CONTENT_COLOR :
                            gfxASurface::CONTENT_COLOR_ALPHA;

(<http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#394>), was the + added intentionally?

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Clean up a chunk of .rej vomit that just so happened to be syntactically correct and not change semantics

Thanks for the catch.

Comment 30

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

Comment on attachment 459862 [details] [diff] [review]
Clean up a chunk of .rej vomit that just so happened to be syntactically correct and not change semantics

ha, cute.

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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