Retain ThebesLayer contents in OpenGL layers backend

RESOLVED FIXED

Status

()

Core
Graphics
--
enhancement
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(5 attachments, 9 obsolete attachments)

25.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.85 KB, patch
Details | Diff | Splinter Review
15.72 KB, patch
vlad
: review+
Details | Diff | Splinter Review
7.04 KB, patch
vlad
: review+
Details | Diff | Splinter Review
1.21 KB, patch
vlad
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Created attachment 453177 [details] [diff] [review]
part 1: Refactor ThebesLayerBuffer in preparation for its use in the OGL layers backend
Assignee: nobody → jones.chris.g
Attachment #453177 - Flags: review?(roc)
Created attachment 453178 [details] [diff] [review]
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.
Attachment #453178 - Flags: review?(vladimir)
Created attachment 453180 [details] [diff] [review]
part 3: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly
Attachment #453180 - Flags: review?(vladimir)
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.
(In reply to comment #4)
> This should be faster than the old code

For scrolling, I meant to qualify.
(In reply to comment #4)
Yes, on Mac it's definitely faster to use glTexSubimage2D to update textures after the initial glTexImage2D.
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
Attachment #453177 - Flags: review?(roc) → review+
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.
Created attachment 453312 [details] [diff] [review]
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.
Attachment #453178 - Attachment is obsolete: true
Attachment #453180 - Attachment is obsolete: true
Attachment #453312 - Flags: review?(vladimir)
Attachment #453178 - Flags: review?(vladimir)
Attachment #453180 - Flags: review?(vladimir)
Created attachment 453313 [details] [diff] [review]
part 3: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly
Attachment #453313 - Flags: review?(vladimir)
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.
Attachment #453313 - Flags: review?(vladimir) → review-
Created attachment 453530 [details] [diff] [review]
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.
Attachment #453313 - Attachment is obsolete: true
Attachment #453530 - Flags: review?(vladimir)
Created attachment 453540 [details] [diff] [review]
 part 3: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly, v2 

(Oops, uploaded the wrong patch.)
Attachment #453530 - Attachment is obsolete: true
Attachment #453540 - Flags: review?(vladimir)
Attachment #453530 - Flags: review?(vladimir)
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!
Attachment #453540 - Flags: review?(vladimir) → review+
No longer blocks: 573889
Depends on: 573889
Created attachment 455774 [details] [diff] [review]
part 2: Use ThebesLayerBuffer to retain contents for ASurface-backed TextureImages
Attachment #453312 - Attachment is obsolete: true
Attachment #453540 - Attachment is obsolete: true
Attachment #455774 - Flags: review?(vladimir)
Created attachment 455775 [details] [diff] [review]
part 3: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly
Attachment #455775 - Flags: review?(vladimir)
Created attachment 455786 [details] [diff] [review]
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.
Attachment #455786 - Flags: superreview?(vladimir)
Created attachment 458589 [details] [diff] [review]
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.
Attachment #455774 - Attachment is obsolete: true
Attachment #455775 - Attachment is obsolete: true
Attachment #455786 - Attachment is obsolete: true
Attachment #458589 - Flags: superreview?(vladimir)
Attachment #455774 - Flags: review?(vladimir)
Attachment #455775 - Flags: review?(vladimir)
Attachment #455786 - Flags: superreview?(vladimir)
Created attachment 458590 [details] [diff] [review]
part 3: Use ThebesLayerBuffer to retain contents for ASurface-backed TextureImages
Attachment #458590 - Flags: review?(vladimir)
Created attachment 458591 [details] [diff] [review]
part 4: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly
Attachment #458591 - Flags: review?(vladimir)
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 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.
Attachment #453177 - Flags: approval2.0?
Tentatively blocking because we may suffer a perf regression from software rendering without retaining thebes content.
Blocks: 580405
No longer blocks: 556063

Updated

8 years ago
blocking2.0: --- → betaN+
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.
Attachment #453177 - Flags: approval2.0?
Attachment #458589 - Flags: superreview?(vladimir) → superreview+
Comment on attachment 458591 [details] [diff] [review]
part 4: Use GL_REPEAT and appropriate texcoords to render ThebesLayerOGL's pixels rotated correctly

looks good!
Attachment #458591 - Flags: review?(vladimir) → review+
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?
Created attachment 459862 [details] [diff] [review]
Clean up a chunk of .rej vomit that just so happened to be syntactically correct and not change semantics

Thanks for the catch.
Attachment #459862 - Flags: review?(vladimir)
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.
Attachment #459862 - Flags: review?(vladimir) → review+
You need to log in before you can comment on or make changes to this bug.