Bug 82686 – [chromium] Add layout test for a video that changes dimensions while decoding

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Andrew Scherkus]

While we have unit tests we lack an end-to-end test for handling videos that change dimensions during playback.

[Andrew Scherkus]

Created attachment 134717 [details]
Patch

[Andrew Scherkus]

Hey Eric mind taking a look over this test?

The VP8 specification doesn't disallow against resolution changes mid-stream. I tried to get a clip generated for other codecs but failed so for now I'm checking it under LayoutTests/platform/chromium -- let me know if you want me to take another approach. For now I'm mostly interested in getting a test in so we don't regress in this area again.

As a side... from a spec perspective any thoughts on whether the element size should change to reflect the new resolution or should it remain the same? I can see arguments both ways :)

[Eric Carlson]

(In reply to comment #2)
> Hey Eric mind taking a look over this test?
> 
> The VP8 specification doesn't disallow against resolution changes mid-stream. I tried to get a clip generated for other codecs but failed so for now I'm checking it under LayoutTests/platform/chromium -- let me know if you want me to take another approach. For now I'm mostly interested in getting a test in so we don't regress in this area again.
> 
The dimensions of some streamed formats (eg. MPEG2 transport stream) can certainly change when tracks appear or disappear, but we aren't set up to stream files for layout tests yet so I think it is OK for this to be chromium-specific for now.

> As a side... from a spec perspective any thoughts on whether the element size should change to reflect the new resolution or should it remain the same? I can see arguments both ways :)

I think it is clear that videoWidth and videoHeight must change, and and in the absence of explicit sizing instructions an element's size is set from the content size, so I think it should reflect the new resolution.

[Eric Carlson]

Comment on attachment 134717 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134717&action=review

> LayoutTests/platform/chromium/media/video-frame-size-change.html:19
> +                waitForEvent("canplay", function() {
> +                    testExpected(video.clientWidth, 1920);
> +                    testExpected(video.clientHeight, 1080);
> +                    testExpected(video.videoWidth, 1920);
> +                    testExpected(video.videoHeight, 1080);
> +                    video.play();
> +                });

It is probably worth having another <video> element with "width" and "height" attributes to make sure that .clientWidth and .clientHeight do NOT change when the intrinsic size changes.

> LayoutTests/platform/chromium/media/video-frame-size-change.html:34
> +                    // Note that it's not specified whether we should scale the new frame size
> +                    // or adjust the size of the element. See the following for details:
> +                    // http://code.google.com/p/chromium/issues/detail?id=117629
> +                    //

I don't think this is necessary.

[Andrew Scherkus]

Comment on attachment 134717 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134717&action=review

>> LayoutTests/platform/chromium/media/video-frame-size-change.html:19
>> +                });
> 
> It is probably worth having another <video> element with "width" and "height" attributes to make sure that .clientWidth and .clientHeight do NOT change when the intrinsic size changes.

Good idea!

>> LayoutTests/platform/chromium/media/video-frame-size-change.html:34
>> +                    //
> 
> I don't think this is necessary.

Removed.

[Andrew Scherkus]

Committed as http://trac.webkit.org/changeset/112920