TiledTextureImage BeginUpdate/EndUpdate are broken

RESOLVED FIXED in mozilla9

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

Trunk
mozilla9
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

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

Updated

6 years ago
Attachment #550050 - Flags: review?(joe)
(Assignee)

Comment 1

6 years ago
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.
Attachment #550050 - Attachment is patch: true
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. :(
Attachment #550050 - Flags: review?(joe) → review-

Updated

6 years ago
Blocks: 676656
Created attachment 550834 [details] [diff] [review]
Fix TiledTextureImage updates (Fix rot)
Attachment #550050 - Attachment is obsolete: true
Attachment #550834 - Flags: review?(joe)
(Assignee)

Comment 4

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

Comment 6

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

Comment 7

6 years ago
Created attachment 551319 [details] [diff] [review]
Fix TiledTextureImage updates (revised, rebased)

This patch hopefully addresses all of the above points and is rebased on central as of earlier today.
Attachment #550834 - Attachment is obsolete: true
Attachment #550834 - Flags: review?(joe)
Attachment #551319 - Flags: review?(joe)
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
Attachment #551319 - Flags: feedback+
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.
Attachment #551319 - Flags: feedback+ → feedback-

Updated

6 years ago
Depends on: 677699
(Assignee)

Comment 10

6 years ago
(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 ?
Assignee: nobody → chrislord.net
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.
What I see is that in backing surface path BeginUpdate does not set 
mUpdateSurface->SetDeviceOffset(gfxPoint(-mUpdateRect.x, -mUpdateRect.y));
(Assignee)

Comment 13

6 years ago
Created attachment 553189 [details] [diff] [review]
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.
Attachment #551319 - Attachment is obsolete: true
Attachment #551319 - Flags: review?(joe)
Attachment #553189 - Flags: review?(joe)
Attachment #553189 - Flags: feedback?(romaxa)
Comment on attachment 553189 [details] [diff] [review]
Fix TiledTextureImage updates (revised, rebased)

Still broken, the same way
Attachment #553189 - Flags: feedback?(romaxa) → feedback-
Checked this version
http://pastebin.mozilla.org/1301330
and it works good on maemo.
(Assignee)

Comment 16

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

Comment 17

6 years ago
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...
Attachment #553400 - Flags: review?(joe)
(Assignee)

Comment 18

6 years ago
Created attachment 553716 [details] [diff] [review]
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.
Attachment #553400 - Attachment is obsolete: true
Attachment #553716 - Flags: review?(romaxa)
Attachment #553716 - Flags: review?(joe)
Comment on attachment 553716 [details] [diff] [review]
Fix TiledTextureImage updates (revised, rebased)

Maemo6 works good with this patch.
Attachment #553716 - Flags: review?(romaxa) → review+
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.
Attachment #553716 - Flags: review?(joe) → review+
Keywords: checkin-needed
(Assignee)

Comment 21

6 years ago
I've pushed this to try, and assuming no issues I'll push it to inbound.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla8
Version: Trunk → Other Branch
(Assignee)

Comment 22

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

Updated

6 years ago
Depends on: 681424
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

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

6 years ago
Created attachment 555488 [details] [diff] [review]
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.
Attachment #553716 - Attachment is obsolete: true
Attachment #555488 - Flags: review+
(Assignee)

Updated

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