803013 - Progressive tile painting can cause bad visual artifacts when scrolling on pages that invalidate while scrolling

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Lord [:cwiiis]]

Progressive tile painting makes the assumption that it's always ok to use progressive painting when scrolling. Unfortunately, this shows unacceptable artifacing if scrolling causes previously valid, visible areas to invalidate - while this is usually the side-effect of invalidation bugs, there are also legitimate causes for this.

I suggest to change the heuristic so that for non-repeated transactions (i.e. for the first update in a transaction) that any tiles that intersect with the viewport should be updated in a single update. This also lets us just remove the assumption about scrolling altogether (as that was to prevent seams in the visible area when not scrolling).

We can disable this heuristic when zooming to allow for faster zoom updates, but I'm not entirely sure we want to...

Comment 1

[Chris Lord [:cwiiis]]

hmm, revising this, I think this should only apply to tiles that were on-screen and are still on-screen at the time of the transaction - this should still have the desired effect without regressing sites like taskjs.org (which the method described in comment #0 unfortunately does :()

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Maintain visual coherency when progressively updating

This patch does a much better job than the quick hack I had in before. Could still do with some testing, but I think it's reasonably safe.

Comment 3

[Benoit Girard (:BenWa)]

I think we should never draw more then one tile at a time because this will give inconsistent abort behavior. What we can do is we can conditionally call 'PaintedTiledLayerBuffer'. The changes we've done will not appear until this is called.

Comment 4

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #3)
> I think we should never draw more then one tile at a time because this will
> give inconsistent abort behavior. What we can do is we can conditionally
> call 'PaintedTiledLayerBuffer'. The changes we've done will not appear until
> this is called.

That sounds fine to me, I'll rework the patch to do this.

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Maintain visual coherency when progressively updating v2

This is much simplified vs. the last one - the new rule is that visible updates to stale content and the first visible update will be grouped together. This also groups by delaying the Painted callback rather than painting multiple tiles at once.

Comment 6

[Chris Lord [:cwiiis]]

Comment on attachment 672823 [details] [diff] [review]
Maintain visual coherency when progressively updating v2

Removing r? as that way of grouping updates appears not to work - I've likely misunderstood how it should work (it results in massive flickering instead).

Comment 7

[Chris Lord [:cwiiis]]

Reading the code, I realise why this happens now. Will factor it in.

Comment 8

[Chris Lord [:cwiiis]]

Attachment: Maintain visual coherency when progressively updating v3

Right, this finally does what I intended and I've checked to make sure;

In the situation that we're drawing visible, stale content, we loop around the tile-drawing logic until we're done with this content. The UI update callback is still called, so cancelling is still possible. Otherwise, the logic is unchanged.

This replaces the check to see if scrolling has happened, so progressive updates are always enabled now, except in the case of resolution changes (which I think this will help enable - I couldn't immediately get it going, so I'll save it for follow-up).

This will also improve seams appearing in animated content.

Comment 9

[Benoit Girard (:BenWa)]

Comment on attachment 673208 [details] [diff] [review]
Maintain visual coherency when progressively updating v3

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

Good work overall. I think it's hard to avoid the growing complexity here :(. Here's are my though on this patch, feel free to disagree. I'd also like to discuss tomorrow about how we handle coherency with the tile store.

::: gfx/layers/TiledLayerBuffer.h
@@ +402,5 @@
>    }
>  
>    mRetainedTiles = newRetainedTiles;
>    mValidRegion = aNewValidRegion;
> +  mLastPaintRegion.Or(mLastPaintRegion, aPaintRegion);

This is confusing. Perhaps we should rename the variable to something like mPaintedRegion. And I think we should remove the SetPaintRegion since it doesn't make sense for something outside this object to set it directly. We should instead have a ClearPaintRegion then the semantic become more simpler. TiledLayerBuffer accumulates the painted region until it is cleared.

Also for a TiledLayerBuffer implementation that isn't going to clear mLastPaintRegion (like the OGL tiled layer) it's just going to keep growing. Perhaps we should move this code in the basic tiled layer or have the ogl tiled layer clear it after every update?

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +308,2 @@
>  
> +  bool repeatImmediately = false;

By forcing a repeat immediately you're preventing other layers from drawing progressively instead of drawing this layer progressively but not forwarding the updates into the progressive transactions. Is this intentional? If so perhaps we should document why here.

@@ +308,4 @@
>  
> +  bool repeatImmediately = false;
> +  mTiledBuffer.SetLastPaintRegion(nsIntRegion());
> +  do {

I don't so much mind the do {} but I feel like this function is starting to become a very complex beast. We need to simplify it. Perhaps we can break some of the logic inside this if into a helper function and pass the relevant objects by reference. Maybe something like regionToPaint = ComputeProgressivePaintRegion(invalidRegion, ...);

Also perhaps the do statement should be within the useProgressivePainting block since it doesn't apply to regular draws.

@@ +395,5 @@
> +          tileBounds.y += incY;
> +        }
> +      }
> +
> +      if (!BasicManager()->IsRepeatTransaction() && !repeatImmediately) {

I don't really like the IsRepeatTransaction bit that much. Is it only to avoid computing to And() on subsequent calls. I guess it's best to avoid calling on subsequent iteration but I feel like it's making this code harder so I'm a bit mixed about keeping it.

@@ +408,5 @@
> +        // therefore update what we want to paint and ask for a new paint transaction.
> +
> +        // If we're drawing stale, visible content, make sure that it happens
> +        // in one go by repeating this work without calling the painted
> +        // callback.

// Then draw the remaining empty content progressively.

Comment 10

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #9)
> Comment on attachment 673208 [details] [diff] [review]
> Maintain visual coherency when progressively updating v3
> 
> Review of attachment 673208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good work overall. I think it's hard to avoid the growing complexity here
> :(. Here's are my though on this patch, feel free to disagree. I'd also like
> to discuss tomorrow about how we handle coherency with the tile store.
> 
> ::: gfx/layers/TiledLayerBuffer.h
> @@ +402,5 @@
> >    }
> >  
> >    mRetainedTiles = newRetainedTiles;
> >    mValidRegion = aNewValidRegion;
> > +  mLastPaintRegion.Or(mLastPaintRegion, aPaintRegion);
> 
> This is confusing. Perhaps we should rename the variable to something like
> mPaintedRegion. And I think we should remove the SetPaintRegion since it
> doesn't make sense for something outside this object to set it directly. We
> should instead have a ClearPaintRegion then the semantic become more
> simpler. TiledLayerBuffer accumulates the painted region until it is cleared.

Agreed, this would be much nicer.

> Also for a TiledLayerBuffer implementation that isn't going to clear
> mLastPaintRegion (like the OGL tiled layer) it's just going to keep growing.
> Perhaps we should move this code in the basic tiled layer or have the ogl
> tiled layer clear it after every update?

I think clearing it after every update would be fine - Though it isn't going to keep growing, it'll just get replaced by the region from the basic tiled layer (right?) I'm happy to make this change for clarity.

> ::: gfx/layers/basic/BasicTiledThebesLayer.cpp
> @@ +308,2 @@
> >  
> > +  bool repeatImmediately = false;
> 
> By forcing a repeat immediately you're preventing other layers from drawing
> progressively instead of drawing this layer progressively but not forwarding
> the updates into the progressive transactions. Is this intentional? If so
> perhaps we should document why here.

This was kind of a throw-back to some other issues I was having, but there's no really strong reason for it now. I was going to change it back, but then I thought the case of one layer having updates on the screen and another layer not having updates on the screen is quite uncommon - not to mention that you probably don't want to have the off-screen layer steal time that could be used to get visible content on the screen.

I think I prefer it this way as I suspect it makes the case where we need to do this faster without sacrificing anything important. The update callback is still called, so if the user makes this case disappear, things will carry on/the draw will get cancelled. I'm open to being convinced otherwise though.

> @@ +308,4 @@
> >  
> > +  bool repeatImmediately = false;
> > +  mTiledBuffer.SetLastPaintRegion(nsIntRegion());
> > +  do {
> 
> I don't so much mind the do {} but I feel like this function is starting to
> become a very complex beast. We need to simplify it. Perhaps we can break
> some of the logic inside this if into a helper function and pass the
> relevant objects by reference. Maybe something like regionToPaint =
> ComputeProgressivePaintRegion(invalidRegion, ...);
> 
> Also perhaps the do statement should be within the useProgressivePainting
> block since it doesn't apply to regular draws.

Agreed on both accounts, will do.

> @@ +395,5 @@
> > +          tileBounds.y += incY;
> > +        }
> > +      }
> > +
> > +      if (!BasicManager()->IsRepeatTransaction() && !repeatImmediately) {
> 
> I don't really like the IsRepeatTransaction bit that much. Is it only to
> avoid computing to And() on subsequent calls. I guess it's best to avoid
> calling on subsequent iteration but I feel like it's making this code harder
> so I'm a bit mixed about keeping it.

Right, my initial implementation had more to do on only the first draw, but currently it's only this. Perhaps this would read better if I did something like;

bool isFirstTransaction = !BasicManager()->IsRepeatTransaction() && !repeatImmediately;
if (isFirstTransaction) ...

instead? It seems a shame to do more region operations than necessary, even if they end up being quite cheap.

> @@ +408,5 @@
> > +        // therefore update what we want to paint and ask for a new paint transaction.
> > +
> > +        // If we're drawing stale, visible content, make sure that it happens
> > +        // in one go by repeating this work without calling the painted
> > +        // callback.
> 
> // Then draw the remaining empty content progressively.

Will add.

Comment 11

[Chris Lord [:cwiiis]]

Attachment: Maintain visual coherency when progressively updating v4

I think this addresses all comments except for the one about immediately repeated updates blocking other layers (which I think is ok, but would be open to changing).

I've split the progressive update region computation out into a separate function and added more/better documentation.

Comment 12

[Benoit Girard (:BenWa)]

(In reply to Chris Lord [:cwiiis] from comment #10)
> not to mention
> that you probably don't want to have the off-screen layer steal time that
> could be used to get visible content on the screen.
> 

That's a good argument. I'm convinced.

Comment 13

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/19c34170c0f6

Comment 14

[Chris Lord [:cwiiis]]

hmm, discovered a situation where this isn't working... Leave open, I'll debug this tomorrow.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19c34170c0f6

Comment 16

[Chris Lord [:cwiiis]]

Think I know what the problem is and just about to test a patch (the problem occurs when there is empty content and stale, visible content - and it's because mValidRegion gets updated immediately, regardless of what areas are actually updated)

Comment 17

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #16)
> Think I know what the problem is and just about to test a patch (the problem
> occurs when there is empty content and stale, visible content - and it's
> because mValidRegion gets updated immediately, regardless of what areas are
> actually updated)

This is indeed the problem - just battling with the artifacts that fixing this (in the way I've fixed it) is causing atm, should have a patch today.

Comment 18

[Chris Lord [:cwiiis]]

Attachment: Make sure the valid region matches the updated region during progressive updates

On top of the committed patch, this fixes coherency issues for me. They are easily tested on a site like bungie.net when zoomed in - you'll see the background constantly repaint when scrolling. Because of the multiple layers, it looks terrible without this patch.

Of course, the invalidation issues on that site should be fixed, but that's a separate bug :)

Comment 19

[Chris Lord [:cwiiis]]

Attachment: Make sure the valid region matches the updated region during progressive updates (v2)

We discussed this on IRC and realised that my patch was massively over-complicated - this does the same thing in far less code :)

Comment 20

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/8b9c7bf80449

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b9c7bf80449

Comment 22

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/84cfd17d0647, though we'll see whether or not it stays out - this landed while reftests were very nearly completely broken, and when they got unbroken, reftest-1 and reftest-4 came out permaorange. The push before this had a wee bit of green, but even with massive retriggers all I could get on this push was the all-white useless results. If we wind up still permaorange without this, it can always just reland again.

Comment 23

[Phil Ringnalda (:philor)]

Dunno which of this or bug 795259 (which I also backed out in the same cset) it was, but with them backed out, reftests are back to as normal and as green as they get.

Comment 24

[Asaf Romano (gone)]

mc-backout: http://hg.mozilla.org/mozilla-central/rev/f7982ce0280a

Comment 25

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/5003d92bca8c

This alone will not cause oranges, it needs to be coupled with the enabling of progressive tiles on bug 795259. A separate bug has been filed about that issue (bug 805014).

Comment 26

[Ryan VanderMeulen [:RyanVM]]

(In reply to Phil Ringnalda (:philor) from comment #22)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/84cfd17d0647

https://hg.mozilla.org/mozilla-central/rev/84cfd17d0647

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5003d92bca8c