The default bug view has changed. See this FAQ.

ThebesOGL buffer Rotation not handled correctly for shadow layers

RESOLVED FIXED in mozilla9

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: romaxa, Assigned: cwiiis)

Tracking

Trunk
mozilla9
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 489807 [details]
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?
(Reporter)

Updated

7 years ago
Blocks: 609838
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.
(Reporter)

Comment 2

7 years ago
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
(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.
(Reporter)

Comment 4

7 years ago
> 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
Probably want to talk to vlad/jeff then.
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 ;).
(Reporter)

Comment 7

7 years ago
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
(Reporter)

Comment 8

6 years ago
this seems already fixed.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 9

6 years ago
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.
(Assignee)

Comment 10

6 years ago
I have a patch to fix this, will tidy up and attach soon.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Updated

6 years ago
Summary: ThebesOGL buffer Rotation not handled correctly → ThebesOGL buffer Rotation not handled correctly for shadow layers
(Assignee)

Comment 11

6 years ago
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.
(Assignee)

Comment 12

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

Comment 13

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

Comment 14

6 years ago
(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.
(Assignee)

Comment 15

6 years ago
Created attachment 550679 [details] [diff] [review]
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.
Attachment #550679 - Flags: review?(matt.woodrow)
(Assignee)

Comment 16

6 years ago
Created attachment 550680 [details] [diff] [review]
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.
(Assignee)

Updated

6 years ago
Attachment #550680 - Flags: review?(matt.woodrow)
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.
Attachment #550679 - Flags: review?(matt.woodrow) → review+
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.
Attachment #550680 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 19

6 years ago
(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.
(Assignee)

Comment 20

6 years ago
(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.
(Assignee)

Comment 21

6 years ago
Created attachment 552324 [details] [diff] [review]
Fix drawing of rotated buffers with tiled textures (revised)

This addresses the issues commented on, with regards to my previous comments.
Assignee: nobody → chrislord.net
Attachment #550679 - Attachment is obsolete: true
Attachment #552324 - Flags: review?(matt.woodrow)
(Assignee)

Comment 22

6 years ago
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.
Attachment #550680 - Flags: feedback?(roc)
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.
(Assignee)

Comment 24

6 years ago
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 :))
Attachment #550680 - Attachment is obsolete: true
Attachment #553156 - Flags: review?(matt.woodrow)
Attachment #550680 - Flags: feedback?(roc)
(Assignee)

Comment 25

6 years ago
(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.
(Assignee)

Updated

6 years ago
Attachment #553156 - Flags: review?(matt.woodrow)
(Assignee)

Comment 26

6 years ago
Created attachment 553196 [details] [diff] [review]
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.
Attachment #553156 - Attachment is obsolete: true
Attachment #553196 - Flags: review?(matt.woodrow)
(Assignee)

Comment 27

6 years ago
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.
Attachment #553196 - Flags: review?(matt.woodrow)
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?
Attachment #552324 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 29

6 years ago
Created attachment 553717 [details] [diff] [review]
Fix upload for GL shadow thebes layers

Actually tested, seems to work correctly on desktop and android.
Attachment #553196 - Attachment is obsolete: true
Attachment #553717 - Flags: review?(matt.woodrow)
(Assignee)

Comment 30

6 years ago
Created attachment 553718 [details] [diff] [review]
Fix drawing of rotated buffers with tiled textures (revised)

Removed unused variable, seems to work fine on desktop and android.
Attachment #552324 - Attachment is obsolete: true
Attachment #553718 - Flags: review?(matt.woodrow)
looks fine on meego!
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 :)
Attachment #553718 - Flags: review?(matt.woodrow) → review+
Attachment #553717 - Flags: review?(matt.woodrow) → review+
(Reporter)

Comment 33

6 years ago
Shouldn't we just push it?
Keywords: checkin-needed
(Assignee)

Comment 34

6 years ago
I've pushed both these patches to try, and assuming there are no issues, I'll push to inbound.
Target Milestone: --- → mozilla8
Version: Trunk → Other Branch
(Assignee)

Comment 35

6 years ago
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
Keywords: checkin-needed
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/b879d413ce2d
http://hg.mozilla.org/mozilla-central/rev/07d34812bdee
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: mozilla8 → mozilla9
Version: Other Branch → Trunk
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

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
http://hg.mozilla.org/mozilla-central/rev/012258603265
http://hg.mozilla.org/mozilla-central/rev/50575fae9b1d
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.