675908 - TiledTextureImage BeginUpdate/EndUpdate are broken

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Lord [:cwiiis]]

Attachment: Fix TiledTextureImage updates

TiledTextureImage breaks BeginUpdate/EndUpdate in these cases:

- The update is encompassed by more than one tile
- The update is encompassed by a single tile that isn't the first tile
- The update is a non-rectangular region that covers more than one tile

The first two are partially caused by an incorrect usage of unsigned integers instead of signed integers. The second is also caused by an incorrect device offset. The third is caused by the region given to BeginUpdate not being altered by the texture tiles, which can only upload rectangular regions.

I've fixed the bugs for the first two, and to fix the third problem, I've introduced a new function 'QueryUpdate' on TextureImage, that lets you query the update region without actually beginning an update. TiledTextureImage then uses this to build up the correct region when BeginUpdate is called.

The attached patch fixes all three cases.

Comment 1

[Chris Lord [:cwiiis]]

Just a note, it's easier to test this patch by artificially restricting the maximum tile size (in the creator of TiledTextureImage) - restricting it to a size less than the width of the window makes the problems especially obvious.

Comment 2

[Joe Drew (not getting mail)]

Comment on attachment 550050 [details] [diff] [review]
Fix TiledTextureImage updates

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

::: gfx/thebes/GLContext.cpp
@@ +583,5 @@
>      }
>  
> +    mUpdateSurface = gfxPlatform::GetPlatform()->
> +        CreateOffscreenSurface(gfxIntSize(rgnSize.width, rgnSize.height),
> +                               GetContentType());

I think this will implicitly disable our FBO use, which we probably don't want. What's the origin of this change? (Note that GetSurfaceForUpdate is also implemented in TextureImageCGL.)

@@ +725,1 @@
>          region = nsIntRegion(bounds);

Can this whole block be replaced with QueryUpdate?

@@ +756,5 @@
> +        aRegion = nsIntRect(nsIntPoint(0, 0), mSize);
> +        return;
> +    }
> +
> +    nsIntRegion newRegion = nsIntRegion();

You can just say newRegion; here.

@@ +762,5 @@
> +    // We need to query each texture with the region it will be drawing and
> +    // set aRegion to be the combination of all of these regions
> +    for (unsigned i = 0; i < mImages.Length(); i++) {
> +        int xPos = (i % mColumns) * mTileSize;
> +        int yPos = (i / mColumns) * mTileSize;

These should presumably be unsigned ints, for the same reason that you changed them in DirectUpdate?

@@ +795,4 @@
>          // if the texture hasn't been initialized yet, or something important
>          // changed, we need to recreate our backing surface and force the
>          // client to paint everything
> +        aRegion = nsIntRect(nsIntPoint(0, 0), mSize);

Should this not be using QueryUpdate?

@@ +803,3 @@
>      for (unsigned i = 0; i < mImages.Length(); i++) {
> +        int xPos = (i % mColumns) * mTileSize;
> +        int yPos = (i / mColumns) * mTileSize;

OK, I'm going to need some explanation of why we need unsigned in some places and not others, I think.

::: gfx/thebes/GLContext.h
@@ +199,5 @@
> +     * making decisions about updating before calling BeginUpdate().
> +     *
> +     * |aRegion| is an inout param.
> +     */
> +    virtual void QueryUpdate(nsIntRegion& aRegion) {

I'm not sure about this name - Query implies that we're asking a question, but really what we're doing is giving the class a chance to override or otherwise change the update region.

I'm not sure what a better name would be, though. :(

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Fix TiledTextureImage updates (Fix rot)

Comment 4

[Chris Lord [:cwiiis]]

(In reply to Joe Drew (:JOEDREW!) from comment #2)
> Comment on attachment 550050 [details] [diff] [review] [diff] [details] [review]
> Fix TiledTextureImage updates
> 
> Review of attachment 550050 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/GLContext.cpp
> @@ +583,5 @@
> >      }
> >  
> > +    mUpdateSurface = gfxPlatform::GetPlatform()->
> > +        CreateOffscreenSurface(gfxIntSize(rgnSize.width, rgnSize.height),
> > +                               GetContentType());
> 
> I think this will implicitly disable our FBO use, which we probably don't
> want. What's the origin of this change? (Note that GetSurfaceForUpdate is
> also implemented in TextureImageCGL.)

I haven't changed any logic here, the previous code did

ImageFormat format = (GetContentType() == gfxASurface::COTNENT_COLOR) ?
  gfxASurface::ImageFormatRGB24 : gfxASurface::ImageFormatARGB32;

then didn't use the format, then called GetSurfaceForUpdate (which isn't a virtual function) with the region width/height and this format, which would then just do

return gfxPlatform::GetPlatform()->
  CreateOffscreenSurface(aSize, gfxASurface::ContentFromFormat(aFmt));

So this was just unecessary code? I figured removing it would make things more readable.

> @@ +725,1 @@
> >          region = nsIntRegion(bounds);
> 
> Can this whole block be replaced with QueryUpdate?

DirectUpdate and Begin/EndUpdate aren't the same, so while QueryUpdate could be used in this case, I think semantically it'd be better not to (Begin/End may at some point have different constraints to DirectUpdate or do different things perhaps?)

> @@ +756,5 @@
> > +        aRegion = nsIntRect(nsIntPoint(0, 0), mSize);
> > +        return;
> > +    }
> > +
> > +    nsIntRegion newRegion = nsIntRegion();
> 
> You can just say newRegion; here.

Right, not sure why I didn't :) Will fix.

> @@ +762,5 @@
> > +    // We need to query each texture with the region it will be drawing and
> > +    // set aRegion to be the combination of all of these regions
> > +    for (unsigned i = 0; i < mImages.Length(); i++) {
> > +        int xPos = (i % mColumns) * mTileSize;
> > +        int yPos = (i / mColumns) * mTileSize;
> 
> These should presumably be unsigned ints, for the same reason that you
> changed them in DirectUpdate?

I changed them all to int from unsigned int - I did this, as in both cases, the negation of the values is used (and can end up being incorrectly handled - as it happens, it's fine in the DirectUpdate case, but best to use signed in all cases to be consistent and to stop this mistake from happening again)

> @@ +795,4 @@
> >          // if the texture hasn't been initialized yet, or something important
> >          // changed, we need to recreate our backing surface and force the
> >          // client to paint everything
> > +        aRegion = nsIntRect(nsIntPoint(0, 0), mSize);
> 
> Should this not be using QueryUpdate?

Yes it should, I missed this... Will fix.

> @@ +803,3 @@
> >      for (unsigned i = 0; i < mImages.Length(); i++) {
> > +        int xPos = (i % mColumns) * mTileSize;
> > +        int yPos = (i / mColumns) * mTileSize;
> 
> OK, I'm going to need some explanation of why we need unsigned in some
> places and not others, I think.

It should be signed everywhere - I'm pretty sure this patch makes it that way?

> ::: gfx/thebes/GLContext.h
> @@ +199,5 @@
> > +     * making decisions about updating before calling BeginUpdate().
> > +     *
> > +     * |aRegion| is an inout param.
> > +     */
> > +    virtual void QueryUpdate(nsIntRegion& aRegion) {
> 
> I'm not sure about this name - Query implies that we're asking a question,
> but really what we're doing is giving the class a chance to override or
> otherwise change the update region.
> 
> I'm not sure what a better name would be, though. :(

Perhaps QueryUpdateRegion or GetUpdateRegion might be better? You're asking the question "What region will I have to redraw if I modify the given region?"

Comment 5

[Joe Drew (not getting mail)]

(In reply to Chris Lord [:cwiiis] from comment #4)
> (In reply to Joe Drew (:JOEDREW!) from comment #2)
> > Comment on attachment 550050 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Fix TiledTextureImage updates
> > 
> > Review of attachment 550050 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/thebes/GLContext.cpp
> > @@ +583,5 @@
> > >      }
> > >  
> > > +    mUpdateSurface = gfxPlatform::GetPlatform()->
> > > +        CreateOffscreenSurface(gfxIntSize(rgnSize.width, rgnSize.height),
> > > +                               GetContentType());
> > 
> > I think this will implicitly disable our FBO use, which we probably don't
> > want. What's the origin of this change? (Note that GetSurfaceForUpdate is
> > also implemented in TextureImageCGL.)
> 
> I haven't changed any logic here, the previous code did
> 
> ImageFormat format = (GetContentType() == gfxASurface::COTNENT_COLOR) ?
>   gfxASurface::ImageFormatRGB24 : gfxASurface::ImageFormatARGB32;
> 
> then didn't use the format, then called GetSurfaceForUpdate (which isn't a
> virtual function)

    // Returns a surface to draw into
    virtual already_AddRefed<gfxASurface>
      GetSurfaceForUpdate(const gfxIntSize& aSize, ImageFormat aFmt);

> > OK, I'm going to need some explanation of why we need unsigned in some
> > places and not others, I think.
> 
> It should be signed everywhere - I'm pretty sure this patch makes it that
> way?

I misread, I guess! On second reading everything looks ok.

> > ::: gfx/thebes/GLContext.h
> > @@ +199,5 @@
> > > +     * making decisions about updating before calling BeginUpdate().
> > > +     *
> > > +     * |aRegion| is an inout param.
> > > +     */
> > > +    virtual void QueryUpdate(nsIntRegion& aRegion) {
> > 
> > I'm not sure about this name - Query implies that we're asking a question,
> > but really what we're doing is giving the class a chance to override or
> > otherwise change the update region.
> > 
> > I'm not sure what a better name would be, though. :(
> 
> Perhaps QueryUpdateRegion or GetUpdateRegion might be better? You're asking
> the question "What region will I have to redraw if I modify the given
> region?"

UpdateUpdateRegion() ;)

Arguably, Objective-C-style GetUpdateRegionFor:aRegion would be clearest. (Jeff's head may explode from me praising Objective-C for something.) So I guess if you had nsIntRegion GetUpdateRegion(nsIntRegion aForRegion) that'd be ok.

Comment 6

[Chris Lord [:cwiiis]]

(In reply to Joe Drew (:JOEDREW!) from comment #5)
> (In reply to Chris Lord [:cwiiis] from comment #4)
> > (In reply to Joe Drew (:JOEDREW!) from comment #2)
> > > Comment on attachment 550050 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] [diff] [details] [review]
> > > Fix TiledTextureImage updates
> > > 
> > > Review of attachment 550050 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] [diff] [details] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: gfx/thebes/GLContext.cpp
> > > @@ +583,5 @@
> > > >      }
> > > >  
> > > > +    mUpdateSurface = gfxPlatform::GetPlatform()->
> > > > +        CreateOffscreenSurface(gfxIntSize(rgnSize.width, rgnSize.height),
> > > > +                               GetContentType());
> > > 
> > > I think this will implicitly disable our FBO use, which we probably don't
> > > want. What's the origin of this change? (Note that GetSurfaceForUpdate is
> > > also implemented in TextureImageCGL.)
> > 
> > I haven't changed any logic here, the previous code did
> > 
> > ImageFormat format = (GetContentType() == gfxASurface::COTNENT_COLOR) ?
> >   gfxASurface::ImageFormatRGB24 : gfxASurface::ImageFormatARGB32;
> > 
> > then didn't use the format, then called GetSurfaceForUpdate (which isn't a
> > virtual function)
> 
>     // Returns a surface to draw into
>     virtual already_AddRefed<gfxASurface>
>       GetSurfaceForUpdate(const gfxIntSize& aSize, ImageFormat aFmt);

Whoops, and I misread this... Will revert :)

> > > OK, I'm going to need some explanation of why we need unsigned in some
> > > places and not others, I think.
> > 
> > It should be signed everywhere - I'm pretty sure this patch makes it that
> > way?
> 
> I misread, I guess! On second reading everything looks ok.
> 
> > > ::: gfx/thebes/GLContext.h
> > > @@ +199,5 @@
> > > > +     * making decisions about updating before calling BeginUpdate().
> > > > +     *
> > > > +     * |aRegion| is an inout param.
> > > > +     */
> > > > +    virtual void QueryUpdate(nsIntRegion& aRegion) {
> > > 
> > > I'm not sure about this name - Query implies that we're asking a question,
> > > but really what we're doing is giving the class a chance to override or
> > > otherwise change the update region.
> > > 
> > > I'm not sure what a better name would be, though. :(
> > 
> > Perhaps QueryUpdateRegion or GetUpdateRegion might be better? You're asking
> > the question "What region will I have to redraw if I modify the given
> > region?"
> 
> UpdateUpdateRegion() ;)
> 
> Arguably, Objective-C-style GetUpdateRegionFor:aRegion would be clearest.
> (Jeff's head may explode from me praising Objective-C for something.) So I
> guess if you had nsIntRegion GetUpdateRegion(nsIntRegion aForRegion) that'd
> be ok.

Rename sounds good, will do!

Will revise and re-attach as soon as time/connectivity allow me to.

Comment 7

[Chris Lord [:cwiiis]]

Attachment: Fix TiledTextureImage updates (revised, rebased)

This patch hopefully addresses all of the above points and is rebased on central as of earlier today.

Comment 8

[Oleg Romashin (:romaxa)]

Comment on attachment 551319 [details] [diff] [review]
Fix TiledTextureImage updates (revised, rebased)

Tested this patch, and at least it does not break anything on maemo6

Comment 9

[Oleg Romashin (:romaxa)]

Comment on attachment 551319 [details] [diff] [review]
Fix TiledTextureImage updates (revised, rebased)

Oh, it seems breaking some rendering offset...
with this patch I'm getting Fennec UI loading progress indicator rendering in 0,0 screen instead of proper position in toolbar.

Comment 10

[Chris Lord [:cwiiis]]

(In reply to Oleg Romashin (:romaxa) from comment #9)
> Comment on attachment 551319 [details] [diff] [review]
> Fix TiledTextureImage updates (revised, rebased)
> 
> Oh, it seems breaking some rendering offset...
> with this patch I'm getting Fennec UI loading progress indicator rendering
> in 0,0 screen instead of proper position in toolbar.

Is this only on Maemo? How do you reproduce? I don't see this on desktop or android, but then the Maemo code-path is a bit different again... Does this still happen with the fixes in bug #677699 and bug #669602 ?

Comment 11

[Oleg Romashin (:romaxa)]

Comment on attachment 551319 [details] [diff] [review]
Fix TiledTextureImage updates (revised, rebased)

>+            // Correct the device offset
>+            surface->SetDeviceOffset(gfxPoint(-bounds.x, -bounds.y));
>             // we don't have a temp surface
Patch works ok on maemo without this change.

Comment 12

[Oleg Romashin (:romaxa)]

What I see is that in backing surface path BeginUpdate does not set 
mUpdateSurface->SetDeviceOffset(gfxPoint(-mUpdateRect.x, -mUpdateRect.y));

Comment 13

[Chris Lord [:cwiiis]]

Attachment: Fix TiledTextureImage updates (revised, rebased)

Ok, updated patch that makes sure that when a backing surface is available, the device offset still gets set. Hopefully this should be good to go.

Comment 14

[Oleg Romashin (:romaxa)]

Comment on attachment 553189 [details] [diff] [review]
Fix TiledTextureImage updates (revised, rebased)

Still broken, the same way

Comment 15

[Oleg Romashin (:romaxa)]

Checked this version
http://pastebin.mozilla.org/1301330
and it works good on maemo.

Comment 16

[Chris Lord [:cwiiis]]

Attachment: Fix TiledTextureImage updates (revised, rebased)

Here's the patch that romaxa tested for me yesterday, hopefully this one is good to go - works fine on desktop, android and Maemo.

Comment 17

[Chris Lord [:cwiiis]]

Comment on attachment 553400 [details] [diff] [review]
Fix TiledTextureImage updates (revised, rebased)

argh, just realised I wasn't testing this correctly, so this is terribly broken on Android. Will fix properly soon...

Comment 18

[Chris Lord [:cwiiis]]

Attachment: Fix TiledTextureImage updates (revised, rebased)

I'm not sure if there's any difference between this and the last patch, but I've gone over it more finely and actually tested it this time and it seems to work on both desktop and android (and ought to work on Maemo).

Perhaps there are still corner-cases, but the original bug is probably more severe than anything this doesn't fix/introduces, assuming that I haven't totally broken Maemo again... Feedback appreciated.

Comment 19

[Oleg Romashin (:romaxa)]

Comment on attachment 553716 [details] [diff] [review]
Fix TiledTextureImage updates (revised, rebased)

Maemo6 works good with this patch.

Comment 20

[Joe Drew (not getting mail)]

Comment on attachment 553716 [details] [diff] [review]
Fix TiledTextureImage updates (revised, rebased)

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

Device offsets being left set worries me a tiny bit, but since we re-set it on every BeginUpdate I don't think it'll be an issue.

Comment 21

[Chris Lord [:cwiiis]]

I've pushed this to try, and assuming no issues I'll push it to inbound.

Comment 22

[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/0c59eeb18700

Comment 23

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/0c59eeb18700

Comment 24

[Benoit Girard (:BenWa)]

Backed out because of opengl layers regression on android in chrome pages (about:config+about:support):
http://hg.mozilla.org/integration/mozilla-inbound/rev/0432558816ac

Comment 25

[Chris Lord [:cwiiis]]

Attachment: Fix typo that caused black areas on android

The patch that I ended up pushing broke on Android due to a small typo/mistake in GetUpdateRegion in GLContextProviderEGL's TextureImage::GetUpdateRegion method:

            aForRegion = nsIntRegion(mUpdateRect);

should have been

            aForRegion = nsIntRegion(aForRegion.GetBounds());

This happened due to bad copy/pasting when adding GetUpdateRegion (and I guess an unfortunate set of circumstances meant it didn't come up in my testing).

Carrying forward review and pushing to inbound again.

Comment 26

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/0dffbdf36868