1419912 - Reduce image container and image key update churn

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Andrew Osmond [:aosmond] (he/him)]

ImageResource::UpdateImageContainer currently updates all of the image containers, regardless of whether or not its contents would have changed. This is a problem because it updates the generation counter, which causes us to update the image key with both ImageBridgeChild and SharedSurfacesChild. This wastes a lot of resources for no real purpose. Additionally, SharedSurfacesChild making the decision to update the image key for the *surface* based on the generation counter is flawed; now that we have support for multiple image containers, it is possible the same surface will be placed in multiple containers.

We should change UpdateImageContainer to know why it is being called -- either because the surface has had decode progress (e.g. we got more rows to display), or because we want to advance an animation, etc. Based on this, it can optimize when the generation counter really needs to be increased for a particular container. Armed with the knowledge of what surface, if any, was changed, UpdateImageContainer itself should take on the responsibility of letting SharedSurfacesChild know it should regenerate its image keys for a particular surface on the next paint instead of basing this decision on potentially many container generation counters.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1419912-Part-1.-Make-SharedSurfacesChild-delegat.patch, v1

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1419912-Part-2a.-Add-plumbing-to-pass-the-update.patch

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1419912-Part-2b.-ImageResource-UpdateImageContai.patch, v1

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0004-Bug-1419912-Part-3.-ImageResource-SetCurrentImage-sh.patch, v1

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c725153c1364e4a748691c9ece8c2842131c72a6

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8931497 [details] [diff] [review]
0003-Bug-1419912-Part-2b.-ImageResource-UpdateImageContai.patch, v1

>+    if (aSurfaceKey && prevSurface &&
>+        prevSurface->GetSize() == entry.mSize &&
>+        aSurfaceKey->Size() != entry.mSize) {
>+      // Our previously selected surface is the desired size (so it isn't a
>+      // substitute), and the surface that changed has a different size. No need
>+      // to recheck the surface cache for this container.

This comment isn't this patch/bug. Is it possible to simplify the design so that the entry.mSize always matches the surface in the entry?

>+    // Note that when we progress an animation, we call this method without a
>+    // surface key. While the animated compositing frame *can* change the
>+    // underlying buffer unlike normal frames, we don't currently place
>+    // animation frames in shared surfaces. As such, as long as we advance the
>+    // generation counter (even if the surface remains the same), the fallback
>+    // mechanism to get the updated current frame to WebRender will work.

The fact that we have to mention WebRender here is unsettling: UpdateImageContainer is a generic function that should work for any backend.

Something about this feels fragile, like it's going to be a source of bugs.

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #6)
> Comment on attachment 8931497 [details] [diff] [review]
> 0003-Bug-1419912-Part-2b.-ImageResource-UpdateImageContai.patch, v1
> 
> >+    if (aSurfaceKey && prevSurface &&
> >+        prevSurface->GetSize() == entry.mSize &&
> >+        aSurfaceKey->Size() != entry.mSize) {
> >+      // Our previously selected surface is the desired size (so it isn't a
> >+      // substitute), and the surface that changed has a different size. No need
> >+      // to recheck the surface cache for this container.
> 
> This comment isn't this patch/bug. Is it possible to simplify the design so
> that the entry.mSize always matches the surface in the entry?
> 

No. With high quality scaling, and factor of 2, we have to live with containers which ultimately want one size, but may at any time, contain a different size.

> >+    // Note that when we progress an animation, we call this method without a
> >+    // surface key. While the animated compositing frame *can* change the
> >+    // underlying buffer unlike normal frames, we don't currently place
> >+    // animation frames in shared surfaces. As such, as long as we advance the
> >+    // generation counter (even if the surface remains the same), the fallback
> >+    // mechanism to get the updated current frame to WebRender will work.
> 
> The fact that we have to mention WebRender here is unsettling:
> UpdateImageContainer is a generic function that should work for any backend.
> 
> Something about this feels fragile, like it's going to be a source of bugs.

I could flip the problem on its head. My problem is the generation counter is monotonically increasing. There are actually only two users of this counter, and they both just care if the counter changed, and have no problem if it cycles (e.g. for animated images) or goes down.

Here's the idea: I store a generation ID for the surface itself, in the surface. It is guaranteed to be unique across all surfaces, sourcing from the same static monotonically increasing counter in ImageContainer. If the surface doesn't have an ID, I generate one for it, and if we get the surface key again in UpdateImageContainer, we regenerate it. We then take this ID and make ImageContainer use it when we call SetCurrentImages.

This allows multiple containers to share the same generation ID and avoid the problem I have in SharedSurfacesChild more transparently. Actually it lets me eliminate p1 of this series entirely.

I will need to think more on the story for animations. In an ideal world, with bug 1337111 (decode full frames), then there is no intermediate compositing surface, so we cycle generation counters on each loop of the animation (e.g. 1, 5, 13, 7, 1, 5, 13, 7, ...). With the intermediate compositing surface, I will need to pass a surface key when advancing an animation too, which I don't currently do.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1419912-Part-1.-Add-use-surface-invalidation-cou.patch

Let's simplify this dramatically. In this patch I add a thread safe invalidation counter to SourceSurfaceSharedData, and update SharedSurfacesChild to use that invalidation counter as the indicator a new image key should be generated. It ignores the generation counter of the container entirely.

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1419912-Part-2.-Ensure-the-decoders-invalidate-t.patch

Here I update the invalidation counter whenever imgFrame::ImageUpdatedInternal is called on the decoding thread. This happens when we post an invalidation, or when we finish a frame. It doesn't call it for vector surfaces, or for non-first frames in an animation, because they don't need to invalidate -- when they get put in an image container, they will always be in their final form. Much harder for a bug to sneak in I hope :).

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

In addition to that, the other reason for previously reducing the churn on the container itself, was because some surfaces *aren't* shared, like when we use D2D to rasterize SVGs. I confirmed with jrmuizel that we want to use skia to rasterize even on Windows, and possibly use recordings/blob images to rasterize in the GPU process (if there is a scenario where this provides a measurable benefit).

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8933449 [details] [diff] [review]
0002-Bug-1419912-Part-2.-Ensure-the-decoders-invalidate-t.patch

Wow, that's a lot smaller.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40291f623e62
Part 1. Add/use surface invalidation counter to track changes in SourceSurfaceSharedData. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5b476bb4052
Part 2. Ensure the decoders invalidate the surface as written to. r=tnikkel

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/40291f623e62
https://hg.mozilla.org/mozilla-central/rev/c5b476bb4052