611315 - ThebesOGL buffer Rotation not handled correctly for shadow layers

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: artifacts

run fennec on desktop with MOZ_ACCELERATED=1

1) open URL
2) scroll page down a bit.

ACTUAL:
Parts of thebes buffers scrolled incorrectly, and quadrants mispositioned
See screenshot

EXPECTED: no artifacts


I  found that if I  set this condition as false when aResolution != 1.0
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ThebesLayerBuffer.cpp#249

Then page rendered correctly


>static PRBool isAcceleratedParent = getenv("MOZ_ACCELERATED") != 0;
>PRBool dropBuffer = isAcceleratedParent && (aXResolution != 1.0 || aYResolution != 1.0);
>if (mBufferRotation == nsIntPoint(0,0) && !dropBuffer) {


I  think Basic Chrome ThebesLayer does return and handle rotation somehow, but OGL thebesBuffer does not... and I  think it happens because of OGL and Basic Thebes shadow layers is working differently (swap vs upload)

cjones: do you have any guess what else need to be fixed in order to make rotation handling correctly?

Comment 1

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

It's not immediately obvious to me what the problem is there.  There appears to be a y-axis rotation being rendered incorrectly, but there's also some garbage on the right side that looks just plain wrong.  Last I was familiar with the code, GL ThebesLayer could draw buffers with arbitrary rotations just fine (I don't think that's going to change with bug 609195 either).  You might check that the buffer rotation and so forth are being synchronized properly from the content process.  No other guesses than that off the top of my head.

Comment 2

[Oleg Romashin (:romaxa)]

This is not about GL rotation, we create ThebesBufferOGL with texture CLAM_TO_EDGE... and do just simple upload copy->src->dest image

I  think some fake rotation happen in software ThebesShadowLayer, and in ThebesLayer, but not in ShadowThebesLayerOGL.

so I  guess content shadowableThebesLayer rotating content in backSurface (sharedImage) but don't know that Chrome shadow layer don't rotate that... et.c

Comment 3

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

(In reply to comment #2)
> This is not about GL rotation, we create ThebesBufferOGL with texture
> CLAM_TO_EDGE... and do just simple upload copy->src->dest image
> 

That's not what my source tree says!  http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#578

> I  think some fake rotation happen in software ThebesShadowLayer, and in
> ThebesLayer, but not in ShadowThebesLayerOGL.
> 
> so I  guess content shadowableThebesLayer rotating content in backSurface
> (sharedImage) but don't know that Chrome shadow layer don't rotate that... et.c

I don't quite follow what you're saying here, but the thebes buffer rect and rotation are synchronized from content-->chrome, so ShadowThebesLayerOGL is very much aware of buffer rotations made in the content process.

Comment 4

[Oleg Romashin (:romaxa)]

> That's not what my source tree says! 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#578

oh, yep, because I'm using patch from 609195 (otherwise weird artifacts)
sorry for confusion

Comment 5

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

Probably want to talk to vlad/jeff then.

Comment 6

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

And if this is indeed a problem with the patch in 609195 this needs to be closed INVALID.  You can't file bugs on things that haven't landed yet ;).

Comment 7

[Oleg Romashin (:romaxa)]

This is reproducible with latest m-c
just take
http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-central-linux/1289505805/

open some long page, make double tap (zoom) and scroll page down
and you will see problems

Comment 8

[Oleg Romashin (:romaxa)]

this seems already fixed.

Comment 9

[Chris Lord [:cwiiis]]

I don't think this is fixed - we've disabled rotation with shadow layers, but if you re-enable it, either the new tiled textures have broken it, or it was never fixed - I'll take a look at this.

Comment 10

[Chris Lord [:cwiiis]]

I have a patch to fix this, will tidy up and attach soon.

Comment 11

[Chris Lord [:cwiiis]]

Sorry for the delay on attaching the patch, seems it works running fennec on desktop, but not on android (possibly due to tiled textures/some inconsistency in the EGL vs. GL code?)

I'm looking into that and if it's taking too long to figure out, I'll just attach the patch anyway.

Comment 12

[Chris Lord [:cwiiis]]

hah, and wrong again - it's broken on both, but for whatever reason, much harder to reproduce on PC. Back to the drawing board...

Comment 13

[Oleg Romashin (:romaxa)]

is it broken the same way as in attached screenshot? wondering why I don't see it on N9 with GLES layers enabled...

Comment 14

[Chris Lord [:cwiiis]]

(In reply to comment #13)
> is it broken the same way as in attached screenshot? wondering why I don't
> see it on N9 with GLES layers enabled...

It's broken in a very similar way, I can't tell if it's exactly the same as I don't know what was done prior to that screenshot being taken. You don't see it currently, as buffer rotation is disabled for shadow layers on GL now (though this wasn't always the case I don't think).

I'm currently working to see if it can be re-enabled, for performance reasons, but I've run into this bug.

Comment 15

[Chris Lord [:cwiiis]]

Attachment: Fix drawing of rotated buffers with tiled textures

This fixes drawing of rotated layers when tiled textures are being used. The drawing relied on texture wrapping, which is incorrect when the texture is only part of the entire image.

This patch restructures the drawing somewhat so that the tiled case can be handled separately and tiling is done manually in that case.

Comment 16

[Chris Lord [:cwiiis]]

Attachment: Fix upload for GL shadow thebes layers

This fixes updating when using GL for shadow thebes layers with rotation. The rotation wasn't taken into account during upload, so whenever there was rotation, the wrong region of the buffer was uploaded, resulting in artifacts similar to the screenshot.

This patch modifies the update region with the rotation coordinates, and if that region then crosses texture boundaries, it splits the update into multiple parts.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 550679 [details] [diff] [review]
Fix drawing of rotated buffers with tiled textures

Review of attachment 550679 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +199,5 @@
> +    nsIntSize texSize = mTexImage->GetSize();
> +    nsIntRect textureRect = nsIntRect(0, 0, texSize.width, texSize.height);
> +    textureRect.MoveBy(region.GetBounds().TopLeft());
> +    nsIntRegion subregion;
> +    subregion.And(region, textureRect);

Under what circumstances would the textureRect not cover the entirety of 'region'?

Given that this is the visible region of the layer (or the bounds of it), we should have every pixel of it available to draw.

@@ +230,5 @@
>        }
> +
> +      nsIntRect tileRect = mTexImage->GetTileRect();
> +      if (i == 0 && tileRect.Size() != textureRect.Size())
> +        usingTiles = true;

How about adding a TiledTextureImage::GetTileCount() function to determine this? Seems cleaner than trying to work it out during iteration.

@@ +236,5 @@
>        // Bind textures.
>        TextureImage::ScopedBindTexture texBind(mTexImage, LOCAL_GL_TEXTURE0);
>        TextureImage::ScopedBindTexture texOnWhiteBind(mTexImageOnWhite, LOCAL_GL_TEXTURE1);
>  
> +      // Draw tile, with manual tiling if we're using tiles

Comment should note that the buffer can be rotated and will have (up to) 4 distinct sections, and we are drawing each of these separately.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 550680 [details] [diff] [review]
Fix upload for GL shadow thebes layers

Review of attachment 550680 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +859,5 @@
> +      destRegion.MoveBy(0, bounds.height);
> +    } else if (top >= bounds.height) {
> +      top -= bounds.height;
> +      destRegion.MoveBy(0, -bounds.height);
> +    }

What would cause these cases to happen? It seems like the updated region should never start at a negative value, and the rotation amount should never be move than the height.

Comment 19

[Chris Lord [:cwiiis]]

(In reply to comment #17)
> Comment on attachment 550679 [details] [diff] [review] [diff] [details] [review]
> Fix drawing of rotated buffers with tiled textures
> 
> Review of attachment 550679 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/ThebesLayerOGL.cpp
> @@ +199,5 @@
> > +    nsIntSize texSize = mTexImage->GetSize();
> > +    nsIntRect textureRect = nsIntRect(0, 0, texSize.width, texSize.height);
> > +    textureRect.MoveBy(region.GetBounds().TopLeft());
> > +    nsIntRegion subregion;
> > +    subregion.And(region, textureRect);
> 
> Under what circumstances would the textureRect not cover the entirety of
> 'region'?
> 
> Given that this is the visible region of the layer (or the bounds of it), we
> should have every pixel of it available to draw.

I agree, but this logic is the same logic that was there before and I didn't want to break anything :)

> @@ +230,5 @@
> >        }
> > +
> > +      nsIntRect tileRect = mTexImage->GetTileRect();
> > +      if (i == 0 && tileRect.Size() != textureRect.Size())
> > +        usingTiles = true;
> 
> How about adding a TiledTextureImage::GetTileCount() function to determine
> this? Seems cleaner than trying to work it out during iteration.

This sounds like a good idea, will do.

> @@ +236,5 @@
> >        // Bind textures.
> >        TextureImage::ScopedBindTexture texBind(mTexImage, LOCAL_GL_TEXTURE0);
> >        TextureImage::ScopedBindTexture texOnWhiteBind(mTexImageOnWhite, LOCAL_GL_TEXTURE1);
> >  
> > +      // Draw tile, with manual tiling if we're using tiles
> 
> Comment should note that the buffer can be rotated and will have (up to) 4
> distinct sections, and we are drawing each of these separately.

Sure, will add.

Comment 20

[Chris Lord [:cwiiis]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> Comment on attachment 550680 [details] [diff] [review] [diff] [details] [review]
> Fix upload for GL shadow thebes layers
> 
> Review of attachment 550680 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/ThebesLayerOGL.cpp
> @@ +859,5 @@
> > +      destRegion.MoveBy(0, bounds.height);
> > +    } else if (top >= bounds.height) {
> > +      top -= bounds.height;
> > +      destRegion.MoveBy(0, -bounds.height);
> > +    }
> 
> What would cause these cases to happen? It seems like the updated region
> should never start at a negative value, and the rotation amount should never
> be move than the height.

I'm not really sure, I don't know the code well enough and haven't dug deep enough to know if this is checked for or not - My naive answer would be that I don't see why it wouldn't happen though.

For example, if you had a large rotation (say height - 10) and an animated gif that was near the top of the rotation (say at the zeroth pixel) that was larger than 10 pixels tall and changed frame, would we really re-render the entire buffer without rotation?

The update region doesn't take into account rotation, I'd have thought that the rotation would be totally transparent to this, and therefore it could happen? Need someone more knowledgeable about that to comment.

Comment 21

[Chris Lord [:cwiiis]]

Attachment: Fix drawing of rotated buffers with tiled textures (revised)

This addresses the issues commented on, with regards to my previous comments.

Comment 22

[Chris Lord [:cwiiis]]

Comment on attachment 550680 [details] [diff] [review]
Fix upload for GL shadow thebes layers

Adding feedback flag for roc. If we could get a comment as to whether update regions are allowed to cross rotation borders, that'd be good.

Comment 23

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

We avoid updating an area that crosses rotation boundaries, basically by undoing the rotation first if necessary. See the comment in ThebesLayerBuffer.cpp:
        // The stuff we need to redraw will wrap around an edge of the
        // buffer, so move the pixels we can keep into a position that
        // lets us redraw in just one quadrant.

BasicBufferOGL does something similar:
        // The stuff we need to redraw will wrap around an edge of the
        // buffer, so we will need to do a self-copy

I expect that this means Upload would never see an update region that crosses a rotation boundary, but you should check, or at least add an assertion in Upload to that effect.

Comment 24

[Chris Lord [:cwiiis]]

Attachment: Fix upload for GL shadow thebes layers

Thanks roc for the comment, I've revised the patch accordingly (and it's now, of course, a lot smaller :))

Comment 25

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #24)
> Created attachment 553156 [details] [diff] [review]
> Fix upload for GL shadow thebes layers
> 
> Thanks roc for the comment, I've revised the patch accordingly (and it's
> now, of course, a lot smaller :))

I have to take this back, after a clobber build, it seems the assertion is being hit - so either I've done it wrong, or boundaries are being crossed - I'll investigate and come back with a revised patch.

Comment 26

[Chris Lord [:cwiiis]]

Attachment: Fix upload for GL shadow thebes layers

Sorry for the noise - my previous update was just obviously wrong. I've corrected the code and the assertion and it appears to work fine now.

Comment 27

[Chris Lord [:cwiiis]]

Comment on attachment 553196 [details] [diff] [review]
Fix upload for GL shadow thebes layers

Argh, realised my testing setup was broken so I wasn't testing on Android correctly. Will re-attach and update flags when I'm certain things are working correctly.

Comment 28

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 552324 [details] [diff] [review]
Fix drawing of rotated buffers with tiled textures (revised)

Review of attachment 552324 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +224,5 @@
> +                   "Tile count mismatch on component alpha texture");
> +      mTexImageOnWhite->BeginTileIteration();
> +    }
> +
> +    int i = 0;

Unused?

Comment 29

[Chris Lord [:cwiiis]]

Attachment: Fix upload for GL shadow thebes layers

Actually tested, seems to work correctly on desktop and android.

Comment 30

[Chris Lord [:cwiiis]]

Attachment: Fix drawing of rotated buffers with tiled textures (revised)

Removed unused variable, seems to work fine on desktop and android.

Comment 31

[Florian Hänel [:heeen]]

looks fine on meego!

Comment 32

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 553718 [details] [diff] [review]
Fix drawing of rotated buffers with tiled textures (revised)

No need to rerequest review for a simple change like this :)

Comment 33

[Oleg Romashin (:romaxa)]

Shouldn't we just push it?

Comment 34

[Chris Lord [:cwiiis]]

I've pushed both these patches to try, and assuming there are no issues, I'll push to inbound.

Comment 35

[Chris Lord [:cwiiis]]

Successful try: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=76b99c8e966b

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b879d413ce2d
http://hg.mozilla.org/integration/mozilla-inbound/rev/07d34812bdee

Comment 36

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/b879d413ce2d
http://hg.mozilla.org/mozilla-central/rev/07d34812bdee

Comment 37

[Benoit Girard (:BenWa)]

Backed out since these patches relied on bug 675908 which had to be backed out because of regression because of opengl layers regression on android in chrome pages (about:config+about:support):
http://hg.mozilla.org/integration/mozilla-inbound/rev/0432558816ac

Comment 38

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/012258603265
http://hg.mozilla.org/mozilla-central/rev/50575fae9b1d