Closed Bug 1097441 Opened 5 years ago Closed 5 years ago

Async video doesn't handle video resolution changes

Categories

(Core :: Layout, defect)

29 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attachment #8521110 - Flags: review?(roc)
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.
Attachment #8521110 - Flags: review?(roc) → review+
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.
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
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.
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.
Attachment #8521717 - Flags: review?(nical.bugzilla)
Attachment #8521717 - Attachment is obsolete: true
Attachment #8521717 - Flags: review?(nical.bugzilla)
Attachment #8521929 - Flags: review?(nical.bugzilla)
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.
Attachment #8521929 - Flags: review?(nical.bugzilla) → review+
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.
Flags: needinfo?(matt.woodrow)
Yes, that seems like a good change :)
Flags: needinfo?(matt.woodrow)
See Also: → 998019
Depends on: 1100110
You need to log in before you can comment on or make changes to this bug.