Closed Bug 1290149 Opened 3 years ago Closed 3 years ago

regression: Full invalidations of tab bar when scrolling

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jrmuizel, Assigned: jnicol)

References

Details

Attachments

(3 files)

STR: scroll the tab bar with paint flashing. It does all of the flashing.
This was caused by bug 1263083
Blocks: 1263083
This is another case where having a optimization for moving the visible region of a single tiled layer would be useful.
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?
(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.
I think if we scrolled this using a memcpy style technique that would be sufficiently performant and sufficiently simple.
Assignee: nobody → jnicol
Attachment #8782886 - Flags: review?(mstange) → review?(matt.woodrow)
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.
Attachment #8782886 - Flags: review?(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
Attachment #8782886 - Flags: review?(mstange)
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.
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()
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 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
Attachment #8782886 - Flags: review?(mstange) → review+
(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?
(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.
Ah, I hadn't caught that change. Thanks.
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.
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c1cefde52cd8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.