596784 - optimize scrolling in GL thebes layers

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

We need to implement faster updates for GL Thebes layers, especially when scrolling.

This code does so, by implementing a similar "rotated buffer" scheme as is done with basic layers.  One major change is that this only handles scrolling in one direction, and only handles invalidations of regions that don't wrap around.  This should be the common case though, and we can do further work to optimize this.

Note that this patch also reverts a current optimization that's present for X11 & EGL only, which uses GLX Pixmaps and the texture_from_pixmap extension so that we can render Thebes content to a Pixmap and bind it as a texture; we'll want to bring this back, but in a different form than the current methods.

Comment 1

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

Attachment: SuperBufferOGL

Comment 2

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

whoops, a few commented out things remained in there; clearing them out.

Comment 3

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

Hmm, disabling the GLX pixmap optimization isn't good, what's it going to take to bring it back?

Comment 4

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

Need to rework it so that we don't wrap an entire GLContext around it, but instead implement TextureImage that uses it.  I'll work on that next.

Comment 5

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

I don't much like the name SuperBufferOGL, and I'd quite like to have both directions working. I'll try to have a bash at this tomorrow to sort that out, and then I'll be in a pretty good position to review the final patch :-)

Comment 6

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

SuperBufferOGL is a placeholder, we can change it; as for both directions, come talk to me first -- we need to figure out what restrictions we need to have on it and what we can relax.

Comment 7

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

Seems to me we can keep SurfaceBufferOGL as-is for now, and just make BasicBufferOGL do the smart GL-specific stuff.

Comment 8

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

Attachment: WIP

This is basically a copy from ThebesLayerBuffer into BasicBufferOGL, with most gfxASurface/gfxContext bits replaced with GL stuff. It compiles, but it's untested. The big hole is at "// XXX need GL magic here"; we need to call BindAndDrawQuadWithTextureRect or RenderTo to copy from mTexImage into destBuffer.

ThebesLayerOGL::CreateSurface was creating a surface and then passing it into "new Basic/SurfaceBufferOGL". That didn't seem like a great idea since the BufferOGL classes know how to (re)create their own surfaces already in BeginPaint, and if we pass in a texture the BufferOGL classes need to assume it's the size of the visible region bounds, which is a bit fragile. So I took that out, so mTexImage starts out null, and replaced the decision about which of Basic or Surface to use with a new method in GLContext.

This patch leaves SurfaceBufferOGL completely untouched except for the above refactoring, so we still get the optimizations there.

Comment 9

[Oleg Romashin (:romaxa)]

Comment on attachment 475685 [details] [diff] [review]
SuperBufferOGL

EGL context texture not rendered correctly.
I guess some changes needed in BeginUpdate EGL.

Also we need add 
XSync(static_cast<gfxXlibSurface*>(mImpl->mThebesSurface.get())->XDisplay(), False);

into EndUpdate function

Comment 10

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

Attachment: updated work in progress

Here's an updated work in progress.  Scrolling seems to work fine, but there are now some other bugs that I'm seeing (perhaps due to viewport etc., not sure).  Will do more debugging tomorrow.

Comment 11

[Oleg Romashin (:romaxa)]

Comment on attachment 476604 [details] [diff] [review]
updated work in progress

With this patch EGL stuff does not work at all... just black screen

Comment 12

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

Attachment: updated

Seems to work fine.  This ended up mixing in a fix for bug 596907 as well, since I needed a scissor/viewport stack and had to change the same callsites that that bug was changing.  Matt, can you verify on Mac?

Comment 14

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

+  // that is bigger than the window dimensions.  This is fine for now,
+  // but might be a problem if we ever start doing GL drawing to
+  // retained layer FBOs that happen to retain more than is visible.

This can already happen with 2D transforms.

+BasicTextureImage::Resize(const nsIntSize& aSize)
+{
+    NS_ASSERTION(!mUpdateContext, "Resize() while in update?");
+
+    mGLContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture);
+
+    mGLContext->fTexImage2D(LOCAL_GL_TEXTURE_2D,

Wouldn't it be helpful to exit early if the size is already correct? I bet this code causes memory allocation on my Mac ATI driver.

I presume that we need to use programs even for a simple blit because of GLES?

+     * If PR_FALSE is returned, then this texture image cannot be resized
+     * in this way, and BeginUpdate/EndUpdate must be used.
+     */
+    virtual PRBool Resize(const nsIntSize& aSize) {
+        return PR_FALSE;

Why not just have Resize always work and call BeginUpdate/EndUpdate itself (or do something equivalent) if necessary?

Comment 15

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

(In reply to comment #14)> +  // that is bigger than the window dimensions.  This is fine for now,> +  // but might be a problem if we ever start doing GL drawing to> +  // retained layer FBOs that happen to retain more than is visible.> > This can already happen with 2D transforms.> > +BasicTextureImage::Resize(const nsIntSize& aSize)> +{> +    NS_ASSERTION(!mUpdateContext, "Resize() while in update?");> +> +    mGLContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture);> +> +    mGLContext->fTexImage2D(LOCAL_GL_TEXTURE_2D,> > Wouldn't it be helpful to exit early if the size is already correct? I bet this code causes memory allocation on my Mac ATI driver.Yeah, I'll do that.> I presume that we need to use programs even for a simple blit because of GLES?Yeah; we should file a separate bug to use FramebufferBlit.  However one issue is that FframebufferBlit can't deal with a GL_REPEAT source, whereas the current code can.. so we should use it if we can, but we'll still need this path.> +     * If PR_FALSE is returned, then this texture image cannot be resized> +     * in this way, and BeginUpdate/EndUpdate must be used.> +     */> +    virtual PRBool Resize(const nsIntSize& aSize) {> +        return PR_FALSE;> > Why not just have Resize always work and call BeginUpdate/EndUpdate itself (or> do something equivalent) if necessary?Oh, that's not a bad idea.  I really just want Resize to do the right thing without needing to BeginUpdate/EndUpdate, but I didn't want to have to untangle the EGL TextureImage implementation with X11/GLX (because I want to rip that stuff out anyway and use texture from pixmap a different way).  I'll make the change here though.

Comment 16

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

Wow -- "Submit only my new comment" is very badly broken.

tl;dr: "yep, good ideas, I'll do that; framebuffer blits can't handle GL_REPEAT source, so we'll need the shader path regardless, but should optimize using framebuffer blit when we can in a future patch"

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Wow -- "Submit only my new comment" is very badly broken.

It's actually a Firefox bug; fix landed earlier today.  Update!

Comment 18

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

Attachment: updated

updated patch, with rotation bug fix.  no idea how I didn't catch that, was due to a last-minute change I did... oops.

This doesn't include the exit-early-on-resize code because it would require a few logic changes (need to track the current size), and I was doing this late last night and didn't trust myself enough to do that quickly without errors :-)  Will do in a followup patch.

For the issue of bigger bounds, well, I think we need to get a testcase that does that so we can look at it.  It wouldn't be super hard to fix.

Comment 19

[Joe Drew (not getting mail)]

This blocks us turning on OpenGL on Mac, which blocks beta 7.

Comment 20

[Joe Drew (not getting mail)]

This does seem to work nicely on Mac.

Comment 21

[Joe Drew (not getting mail)]

Hm, however, I get lots and lots of

WARNING: Overflowed nscoord_MAX in conversion to nscoord: file ../../dist/include/nsCoord.h, line 359

Comment 22

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

Comment on attachment 477163 [details] [diff] [review]
updated

Nice.

Comment 23

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 477163 [details] [diff] [review]
updated


>+        gfxContext *dummy = BeginUpdate(r);

Can we get rid of the variable? Gives us a lot of compile time warnings.

The scissor stuff doesn't seem to take into account that a Y-flip transform is applied to all drawing when DoubleBuffering is enabled (except inside LayerManagerOGL::Render). I was under the impression that our coordinates (from layout) were always with 0 at the top, and OpenGL has 0 at the bottom. Without double buffering we render to a FBO using our coordinates (drawing everything upside down) and then flip on the final blit to the screen.

Does appear to work fine on mac though.

I get garbage once the microsoft psychedelic benchmark loads, fixed by switching tabs.

http://ie.microsoft.com/testdrive/Performance/PsychedelicBrowsing/Default.htm

I also noticed a number of new (I think) reftest failures (mainly in selection/), we should test this in birch/try before landing it.

Comment 24

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 477163 [details] [diff] [review]
updated

Since the scissor is applied after the transformation, we need to flip the scissor when the Y-flip transform is applied (IsDoubleBuffering() == true).

Rendering to the window or a FBO shouldn't affect the clipping at all.

Checking aPreviousFrameBuffer == 0 is essentially the same as IsDoubleBuffering for simple cases, which is why it's working currently, but could break on double buffered systems that use a FBO for complex containers.

I think the best solution would be to always apply the Y-flip transform, and always invert the clip rect, but changing the above to IsDoubleBuffering() should be fine to.

r=me with this and tests passing.

Comment 25

[Oleg Romashin (:romaxa)]

Attachment: crash on N900 Qt port EGL

Running this:
MOZ_ACCELERATED=1 MOZ_QT_GRAPHICSSYSTEM=native ./fennec -graphicssystem native  -url about:license

make qt egl crashes

Running qt build N900:
./fennec -url about:license

Only black screen visible

Comment 26

[Joe Drew (not getting mail)]

Attachment: draw only the rects inside the region

This patch makes us draw only the rects inside the visible region (instead of the bounds of the visible region), which makes sure we don't draw garbage when we've got a non-rectangular region.

Comment 27

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

Comment on attachment 477571 [details] [diff] [review]
draw only the rects inside the region

Looks great to me, ship it

Comment 28

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/51bf8308d297
http://hg.mozilla.org/mozilla-central/rev/3d4df00e4c90

Vlad has promised to do the followup work to address Matt's concerns.

Comment 29

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

Yep, I also filed bug 598703 to use BlitFramebuffer when possible.

Comment 30

[Daniel Holbert [:dholbert]]

Landed a followup to remove an unused variable:
 http://hg.mozilla.org/mozilla-central/rev/ee9d604a8ed5