1097441 - Async video doesn't handle video resolution changes

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: video-frame-scaling

If the resolution of a video changes while we're playing asynchronously (which can happen with MSE) then we get the scaling wrong and draw it the wrong size.

Instead of specifying the transform from image->dest as part of the layer transform, we should instead use ImageLayer::SetScaleToSize so that it gets recomputed for each video frame.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8521110 [details] [diff] [review]
video-frame-scaling

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

Looks good but really needs a test, since I suspect STRETCH isn't well tested anywhere.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Note also that this assumes all frames have the same aspect ratio. We shouldn't really assume that, in which case we need a new ScaleMode, say PRESERVE_ASPECT_RATIO_CONTAIN, that does an aspect-ratio-preserving scale to fit the image into mScaleToSize.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But that can be another bug...

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also note that CSS object-fit and object-position are about to land, which will require additional layers support to handle in their full generality with async video frames.
http://dev.w3.org/csswg/css-images-3/#the-object-fit
http://dev.w3.org/csswg/css-images-3/#the-object-position

Comment 5

[Matt Woodrow (:mattwoodrow)]

The existing video tests should cover this, all video frames will get drawn this way so we should have lots of coverage.

How likely are we to get videos that change aspect ratio mid-stream? That seems a little scary. Not that hard to fix though.

Comment 6

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f96fe425680e

Comment 7

[Matt Woodrow (:mattwoodrow)]

And backed out for failing reftests - https://hg.mozilla.org/integration/mozilla-inbound/rev/f2afdff90b2e

Looks like we never set mSize on BufferTextureHost, so we can't compute the scale. Will figure something out tomorrow.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Initialize mSize on BufferTextureHost

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Initialize mSize on BufferTextureHost

Comment 10

[Nicolas Silva [:nical]]

Comment on attachment 8521929 [details] [diff] [review]
Initialize mSize on BufferTextureHost

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

::: gfx/layers/composite/CompositableHost.h
@@ +151,5 @@
>    {
>      MOZ_ASSERT(false, "Should have been overridden");
>    }
>  
> +  virtual gfx::IntSize GetImageSize() const

This isn't great. I suppose we could have ImageLayerComposite hold a reference to an ImageHost rather than a CompositableHost to avoid leaking this api into CompositableHost. Anyway it's not pretty but not the end of the world I guess.

Comment 11

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a98abd02074f
https://hg.mozilla.org/integration/mozilla-inbound/rev/11c5034cb10b

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a98abd02074f
https://hg.mozilla.org/mozilla-central/rev/11c5034cb10b
https://hg.mozilla.org/mozilla-central/rev/cc92e864c679

Comment 13

[Daniel Holbert [:dholbert]]

Question on https://hg.mozilla.org/mozilla-central/rev/11c5034cb10b:
>+++ b/layout/generic/nsVideoFrame.cpp
>@@ -218,19 +218,18 @@ nsVideoFrame::BuildLayer(nsDisplayListBu
>-  transform.PreScale(r.Width() / frameSize.width,
>-                     r.Height() / frameSize.height);
>   layer->SetBaseTransform(gfx::Matrix4x4::From2D(transform));
>+  layer->SetScaleToSize(IntSize(r.width, r.height), ScaleMode::STRETCH);

Sanity-check: It would've been OK to just directly pass "scaleHint" here, too, right? scaleHint is created a few lines up, like so:
    IntSize scaleHint(static_cast<int32_t>(r.Width()),
                      static_cast<int32_t>(r.Height()));
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsVideoFrame.cpp#209

I'm asking because my patch in bug 624647 ("fix, part 2") rewrites this chunk of code to honor "object-fit"/"object-position", and I'm unbitrotting it on top of this bug's patch, and I'd like to unbitrot it such that I use "scaleHint" to reduce the number of times we cast from float to int.  I'm pretty sure it's OK, but just wanted to make sure there wasn't a reason you avoided 'scaleHint' here.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Yes, that seems like a good change :)