1290149 - regression: Full invalidations of tab bar when scrolling

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

STR: scroll the tab bar with paint flashing. It does all of the flashing.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

This was caused by bug 1263083

Comment 2

[Markus Stange [:mstange]]

This is another case where having a optimization for moving the visible region of a single tiled layer would be useful.

Comment 3

[Jamie Nicol [:jnicol]]

And the reason I couldn't reproduce was because I was on Linux so not using tiling at all. D'oh.

The layer's visible region is roughly the same as the width of the toolbar rather than the actual width of all the tabs. As the tab bar is scrolled the layer's visible region moves (.x changes but .width remains the same).

When using RotatedBuffer, most of the content remains valid on scroll. When using MultiTiledContentClient, most of the tiles remain valid. But this means when using SingleTiledContentClient then entire buffer becomes invalid on scroll.

Switching back to using MultiTiledContentClient is a huge waste of memory. What Markus suggests about being able to move the visible region of a single tiled layer would be good. But is there any downsides to just using RotatedBuffer (no tiles) here?

Comment 4

[Markus Stange [:mstange]]

(In reply to Jamie Nicol [:jnicol] from comment #3)
> But is there any downsides to just using RotatedBuffer (no tiles) here?

Mostly the fact that we don't like the RotatedBuffer code and would like to have a migration path off of it.
Also, we'd have to make SingleTiledContentClient use RotatedBuffer, because as far as I remember, all the other ways of using RotatedBuffer on Mac require synchronous layer transactions.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

I think if we scrolled this using a memcpy style technique that would be sufficiently performant and sufficiently simple.

Comment 6

[Jamie Nicol [:jnicol]]

Attachment: Bug 1290149 - Copy intersecting region from old frontbuffer to new backbuffer when single tile layer visible region changes;

Review commit: https://reviewboard.mozilla.org/r/72918/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72918/

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8782886 [details]
Bug 1290149 - Copy intersecting region from old frontbuffer to new backbuffer when single tile layer visible region changes;

https://reviewboard.mozilla.org/r/72918/#review70676

::: gfx/layers/client/SingleTiledContentClient.cpp:193
(Diff revision 1)
> +      const gfx::IntRect rect = copyableRegion.
> +        MovedBy(-discardedValidRegion.GetBounds().TopLeft()).GetBounds();

I suggest calling GetBounds first and then moving.

const gfx::IntRect copyableRect = copyableRegion.GetBounds();
const gfx::IntRect rect = copyableRect - discardedValidRegion.GetBounds().TopLeft();
const gfx::IntPoint dest = copyableRect.TopLeft() - mTilingOrigin;

::: gfx/layers/client/SingleTiledContentClient.cpp:198
(Diff revision 1)
> +      const gfx::IntRect rect = copyableRegion.
> +        MovedBy(-discardedValidRegion.GetBounds().TopLeft()).GetBounds();
> +      const gfx::IntPoint dest =
> +        copyableRegion.GetBounds().TopLeft() - mTilingOrigin;
> +
> +      RefPtr<gfx::DataSourceSurface> surf = discardedFrontBuffer->GetAsSurface();

The documentation for TextureClient::GetAsSurface says:

> This method is strictly for debugging. It causes locking and needless copies.

I don't know this code well enough to suggest alternatives. nical or mattwoodrow might know what to do.

::: gfx/layers/client/SingleTiledContentClient.cpp:199
(Diff revision 1)
> +        MovedBy(-discardedValidRegion.GetBounds().TopLeft()).GetBounds();
> +      const gfx::IntPoint dest =
> +        copyableRegion.GetBounds().TopLeft() - mTilingOrigin;
> +
> +      RefPtr<gfx::DataSourceSurface> surf = discardedFrontBuffer->GetAsSurface();
> +      dt->CopySurface(surf, rect, dest);

Maybe add a comment
// This can copy invalid content because we're copying the bounds of the region,
// but the invalid parts will be redrawn with valid content because of the way
// we modify the paintRegion.
or something like that.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8782886 [details]
Bug 1290149 - Copy intersecting region from old frontbuffer to new backbuffer when single tile layer visible region changes;

mozreview what are you doing

Comment 9

[Jamie Nicol [:jnicol]]

Hmm this appears to still be not quite right. If adding/removing tabs quickly, upon the change from the tabs fitting within the width and them needing to be scrollable we can end up with some invalid content being shown. Not sure why.

Comment 10

[Jamie Nicol [:jnicol]]

Attachment: Invalid content when closing tabs

Comment 11

[Jamie Nicol [:jnicol]]

Attachment: Invalid content when adding tabs

Comment 12

[Jamie Nicol [:jnicol]]

This also caused a huge regression on the tabpaint talos test: https://treeherder.mozilla.org/perf.html#/compare?originalProject=autoland&originalRevision=3da4d64410c0&newProject=try&newRevision=9a8916934113&framework=1&showOnlyImportant=0

Could well be because of the use of TextureClient::GetAsSurface()

Comment 13

[Matt Woodrow (:mattwoodrow)]

It's not obvious that GetAsSurface() would actually be slow on OSX.

As far as I can tell we wouldn't trip the copy-on-write behaviour, so the returned DataSourceSurface should just be a wrapper around the existing pixels.

TextureClient::CopyToTextureClient is the recommended way to copy though.

Comment 14

[Jamie Nicol [:jnicol]]

Comment on attachment 8782886 [details]
Bug 1290149 - Copy intersecting region from old frontbuffer to new backbuffer when single tile layer visible region changes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72918/diff/1-2/

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8782886 [details]
Bug 1290149 - Copy intersecting region from old frontbuffer to new backbuffer when single tile layer visible region changes;

https://reviewboard.mozilla.org/r/72918/#review71104

Comment 16

[Markus Stange [:mstange]]

(In reply to Jamie Nicol [:jnicol] from comment #9)
> Hmm this appears to still be not quite right. If adding/removing tabs
> quickly, upon the change from the tabs fitting within the width and them
> needing to be scrollable we can end up with some invalid content being
> shown. Not sure why.

Was this due to the missing locks?

Comment 17

[Jamie Nicol [:jnicol]]

(In reply to Markus Stange [:mstange] from comment #16)
> (In reply to Jamie Nicol [:jnicol] from comment #9)
> > Hmm this appears to still be not quite right. If adding/removing tabs
> > quickly, upon the change from the tabs fitting within the width and them
> > needing to be scrollable we can end up with some invalid content being
> > shown. Not sure why.
> 
> Was this due to the missing locks?

No, the previous path had locking in the code it called.

It was fixed by subbing aDirtyRegion from copyableRegion. I was assuming the entire intersection between the old valid region and the new valid region is reusable. But this is of course not the case: some content will have been invalidated and needs repainted, even if the buffer hadn't been discarded. A very reproducible test for this is reduce the window's width and see if the toolbar is repainted.

Comment 18

[Markus Stange [:mstange]]

Ah, I hadn't caught that change. Thanks.

Comment 19

[Jamie Nicol [:jnicol]]

Comment on attachment 8782886 [details]
Bug 1290149 - Copy intersecting region from old frontbuffer to new backbuffer when single tile layer visible region changes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72918/diff/2-3/

Comment 20

[Jamie Nicol [:jnicol]]

I found the cause of the remaining problem I was seeing: It was due to copying the bounds of the copyable region rather than the individual rects. This may have been okay for opaque layers, but not for component alpha layers.

Comment 21

[Jamie Nicol [:jnicol]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e7aeb9c66dd7449a0e6534676b831765239a0b8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b5db8733d655cbc1f9eb63940676d648b21c98f

r+ carries from comment 15

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1cefde52cd8
Copy intersecting region from old frontbuffer to new backbuffer when single tile layer visible region changes; r=mstange

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c1cefde52cd8