1057222 - Do partial tile uploads on desktop

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: partial-upload

This seems to win us a lot on desktop.

The current 'painted region' that TiledLayerBuffer sends on includes new things that we painted, but doesn't include areas that we had to copy back from the front buffer when double buffering. 

This fixes that, although in a pretty ugly way.

I think the right thing to do would be to stop using TiledLayerBuffer on the compositor side, and just pass a per-tile dirty rect/region. That would reduce a lot of the complexity. I'm not all that keen to do this right now though.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Avoid uploading whole tiles

This fixes the case where we scroll a new tile into the visible region and we end up uploading the entire tile even though only a small section of it contains valid content.

This doesn't sound like much, but the extra upload time can cause the buffer to still be locked when we try paint the following frame. Then we have to allocate a new tile as backbuffer, do copy-back etc and it takes us multiple frames to recover.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Avoid uploading whole tiles

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 8477229 [details] [diff] [review]
Avoid uploading whole tiles

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

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +301,5 @@
> +        !aSrcOffset &&
> +        !aDestRegion->IsEqual(nsIntRect(0, 0, size.width, size.height))) {
> +      // If we're only wanting to upload part of the texture, then initialize
> +      // it here so that TextureImage doesn't upload the whole surface
> +      mTexImage->Resize(size);

I don't understand the conditional logic here? Why do just calculate the size we need vs the size we have and resize if it doesn't match?

It feels like we will resize it down in some cases but that we might not hit this branch under some cases and leave it the wrong size. But then again I don't really understand why the conditional here is so complex.

Comment 4

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #0)
> Created attachment 8477220 [details] [diff] [review]
> partial-upload
> 
> This seems to win us a lot on desktop.
> 
> The current 'painted region' that TiledLayerBuffer sends on includes new
> things that we painted, but doesn't include areas that we had to copy back
> from the front buffer when double buffering. 
> 
> This fixes that, although in a pretty ugly way.
> 
> I think the right thing to do would be to stop using TiledLayerBuffer on the
> compositor side, and just pass a per-tile dirty rect/region. That would
> reduce a lot of the complexity. I'm not all that keen to do this right now
> though.

I don't understand why things are so complicated? The code was designed to make patching this trivial. The 'painted region' on the compositor side should match exactly what changes since the last frame. All the pixels should be valid in the surface that we received from content. We should just be able to upload that region directly on desktop where we know that it will be faster then a full upload. Why does it matter if content rasterized or copied the pixels into its surface. From the point of view of the compositor it's irrelevant how the pixels got there,

Comment 5

[Matt Woodrow (:mattwoodrow)]

(In reply to Benoit Girard (:BenWa) from comment #4)

> I don't understand why things are so complicated? The code was designed to
> make patching this trivial. The 'painted region' on the compositor side
> should match exactly what changes since the last frame. All the pixels
> should be valid in the surface that we received from content. We should just
> be able to upload that region directly on desktop where we know that it will
> be faster then a full upload. Why does it matter if content rasterized or
> copied the pixels into its surface. From the point of view of the compositor
> it's irrelevant how the pixels got there,

The problem is that area that changed since last frame, and the area that needs to be uploaded aren't the same thing.

Imagine a simple case where the dirty region is a single pixel, but instead of using the existing front buffer tile (because it's still locked) we have to allocate a new one. We copy from the front buffer to the back buffer to get all the content from the previous frame into the new tile, and then draw the one new pixel.

When I send this tile to the compositor, mPaintedRegion contains only 1 pixel, but I need to upload the entire 256x256 tile.

The actual area we need to upload is the combination of the dirty area, plus the area that we copied between tiles if they weren't the same thing.

My patch computes the latter and includes it in the mPaintedRegion we send to the compositor.

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Benoit Girard (:BenWa) from comment #3)
> 
> I don't understand the conditional logic here? Why do just calculate the
> size we need vs the size we have and resize if it doesn't match?
> 
> It feels like we will resize it down in some cases but that we might not hit
> this branch under some cases and leave it the wrong size. But then again I
> don't really understand why the conditional here is so complex.

'Resize' isn't a very useful name here. The point of this call is to call glTexture2D(....., nullptr) to create the texture with uninitialized contents. Then we can use glTexSubImage2D during the upload phase, and only upload the pixels that we will use.

This solves the problem where we scroll a new tile into view, and only a 5 pixel strip of the in-memory tile contains valid content but we'd upload the whole thing (including all the unitialized/blank) pixels.

The conditional is to stop this from happening in non-tiling uses cases (don't want to risk regressions), and if we were going to upload the whole tile anyway.

I can add a comment explaining this, since it's non obvious.

Comment 7

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> When I send this tile to the compositor, mPaintedRegion contains only 1
> pixel, but I need to upload the entire 256x256 tile.

Why? We only have one front buffer texture, we just need to glTexSubImage that pixel that changed. Unless that's changed now?

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Benoit Girard (:BenWa) from comment #7)
> Why? We only have one front buffer texture, we just need to glTexSubImage
> that pixel that changed. Unless that's changed now?

Yeah, we have a sort of hybrid double/single buffering scheme these days.

When trying to find a back-buffer we attempt to grab the existing front buffer (single buffering), but if it's still locked then we'll allocate a new back-buffer.

Comment 9

[Benoit Girard (:BenWa)]

Comment on attachment 8477220 [details] [diff] [review]
partial-upload

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

Looks good but I'd like to see if the revisions make the patch more understandable. It's not clear at all reading the code.

::: gfx/layers/client/TiledContentClient.h
@@ +265,5 @@
>    nsIntRegion mInvalidBack;
>    nsExpirationState mExpirationState;
>  
>  private:
>    void ValidateBackBufferFromFront(const nsIntRegion &aDirtyRegion,

Can we get a method comment? It's not clear what 'aAddPaintedRegion' means.

@@ +457,5 @@
>    void* mCallbackData;
>    CSSToParentLayerScale mFrameResolution;
>    gfxContentType mLastPaintContentType;
>    SurfaceMode mLastPaintSurfaceMode;
> +  nsIntRegion mNewValidRegion;

'new' valid region is confusing. New implies that there a different from the implied 'current' valid region. It should be identical to mValidRegion in TiledLayerBuffer if I'm tracing the code correctly. If not perhaps it could use a better name and/or description.

::: gfx/layers/composite/TiledContentHost.cpp
@@ +153,4 @@
>    // We possibly upload the entire texture contents here. This is a purposeful
>    // decision, as sub-image upload can often be slow and/or unreliable, but
>    // we may want to reevaluate this in the future.
>    // For !HasInternalBuffer() textures, this is likely a no-op.

We really should look again at this on modern android.

Comment 10

[Benoit Girard (:BenWa)]

Comment on attachment 8477229 [details] [diff] [review]
Avoid uploading whole tiles

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

Alright we discuss this on IRC.

I'm r- because as-is the patch seems to read:
Allocate a 256x256 image, resize it to 5x5, then upload it
but what we're doing is:
Allocate a 256x256 texture, resize the logical portion to 5x5, then glTexSubImage the 5x5 rect

Should be good once that's clear.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Do partial texture uploads on desktop

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Avoid uploading whole tiles v2

Comment 13

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcb6d731bb7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c81383c420f4

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fcb6d731bb7a
https://hg.mozilla.org/mozilla-central/rev/c81383c420f4