Bug 34933 – media/video-display-aspect-ratio.html fails

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

[Philippe Normand]

on SnowLeopard: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20%28Tests%29/r54766%20%285575%29/media/video-display-aspect-ratio-pretty-diff.html
on Leopard: http://build.webkit.org/results/Leopard%20Intel%20Release%20%28Tests%29/r54766%20%2810624%29/media/video-display-aspect-ratio-pretty-diff.html
on Windows: http://build.webkit.org/results/Windows%20Release%20%28Tests%29/r54766%20%289057%29/media/video-display-aspect-ratio-pretty-diff.html

On Leopard the width difference is only 1 pixel, maybe we could have platform-specific results for it.

I will skip the test on these platforms for now.

[Philippe Normand]

The commit that introduced the new test is http://trac.webkit.org/changeset/54766

[Philippe Normand]

Test skipped as of r54768

[Nikolas Zimmermann]

Hmm, it also fails on Tiger now. When I look at the test movie with QuickTime, it reports 427 as width. Are you sure 426 is right? Also on my leopard box, it reports 427.

[Eric Carlson]

I think the movie is the problem here, it has a width 426.666. The width field  in the video track header is 01 AA AA AA. Track width and height are 16.16 fixed point, so 0x01AA -> 426 and 0xAAAA/0x00010000 -> 0.6666564941.

I don't know what the rules are for rounding a number like this, but I wouldn't be surprised if they are ambiguous.

What is this movie supposed to test?

[Philippe Normand]

It is meant to test that the display aspect ratio (DAR) of the video is applied correctly during naturalSize() calculation. 

In the GStreamer player we come with 426 by applying the PAR (4:3) to the video resolution and rescaling against the calculated DAR. 

The GStreamer qt demuxer correctly retrieves the display resolution but doesn't advertize it downstream in the pipeline. Instead it calculates the PAR using this, as a int/int fraction, hence the loss of precision.

[Eric Carlson]

Created attachment 48945 [details]
Proposed patch

[Nikolas Zimmermann]

Patch looks fine, but don't you also want to modify the skipped lists? Most platforms skip the test because of inconsitent results..

[Eric Carlson]

Philippe, I won't be able to get back to this until tomorrow my time so please feel free to take it from here if you want.

[Philippe Normand]

Created attachment 48982 [details]
updated patch

[Philippe Normand]

(In reply to comment #9)
> Created an attachment (id=48982) [details]
> updated patch

I unskipped the test only on Leopard. On SnowLeopard and Windows it is still skipped because the test results are much more different from the expected results.

[Eric Seidel (no email)]

Comment on attachment 48982 [details]
updated patch

OK.

[Philippe Normand]

Landed in r55125. Should we keep the bug opened nevertheless? Test is still skipped on SnowLeopard and Windows.

[Philippe Normand]

Comment on attachment 48982 [details]
updated patch

clearing flags, patch was commited

[Philippe Normand]

Closing as the patch was commited. See Bug 35297 from now on ;)