Bug 82686 - [chromium] Add layout test for a video that changes dimensions while decoding
Summary: [chromium] Add layout test for a video that changes dimensions while decoding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andrew Scherkus
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-29 20:30 PDT by Andrew Scherkus
Modified: 2012-04-02 14:08 PDT (History)
1 user (show)

See Also:


Attachments
Patch (1.36 MB, patch)
2012-03-29 20:38 PDT, Andrew Scherkus
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Scherkus 2012-03-29 20:30:52 PDT
While we have unit tests we lack an end-to-end test for handling videos that change dimensions during playback.
Comment 1 Andrew Scherkus 2012-03-29 20:38:15 PDT
Created attachment 134717 [details]
Patch
Comment 2 Andrew Scherkus 2012-04-02 10:39:48 PDT
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 :)
Comment 3 Eric Carlson 2012-04-02 11:21:31 PDT
(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.
Comment 4 Eric Carlson 2012-04-02 11:23:26 PDT
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.
Comment 5 Andrew Scherkus 2012-04-02 12:33:49 PDT
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.
Comment 6 Andrew Scherkus 2012-04-02 14:08:54 PDT
Committed as http://trac.webkit.org/changeset/112920