Bug 34933 - media/video-display-aspect-ratio.html fails
Summary: media/video-display-aspect-ratio.html fails
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 35297
  Show dependency treegraph
Reported: 2010-02-15 01:35 PST by Philippe Normand
Modified: 2010-02-23 08:15 PST (History)
2 users (show)

See Also:

Proposed patch (256.42 KB, patch)
2010-02-17 16:35 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
updated patch (1.93 KB, patch)
2010-02-18 00:42 PST, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Philippe Normand 2010-02-15 01:37:09 PST
The commit that introduced the new test is http://trac.webkit.org/changeset/54766
Comment 2 Philippe Normand 2010-02-15 01:52:23 PST
Test skipped as of r54768
Comment 3 Nikolas Zimmermann 2010-02-17 07:01:42 PST
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.
Comment 4 Eric Carlson 2010-02-17 07:44:31 PST
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?
Comment 5 Philippe Normand 2010-02-17 08:52:34 PST
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.
Comment 6 Eric Carlson 2010-02-17 16:35:48 PST
Created attachment 48945 [details]
Proposed patch
Comment 7 Nikolas Zimmermann 2010-02-17 16:38:39 PST
Patch looks fine, but don't you also want to modify the skipped lists? Most platforms skip the test because of inconsitent results..
Comment 8 Eric Carlson 2010-02-17 17:38:30 PST
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.
Comment 9 Philippe Normand 2010-02-18 00:42:15 PST
Created attachment 48982 [details]
updated patch
Comment 10 Philippe Normand 2010-02-18 00:44:45 PST
(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.
Comment 11 Eric Seidel (no email) 2010-02-22 13:47:58 PST
Comment on attachment 48982 [details]
updated patch

Comment 12 Philippe Normand 2010-02-23 00:20:52 PST
Landed in r55125. Should we keep the bug opened nevertheless? Test is still skipped on SnowLeopard and Windows.
Comment 13 Philippe Normand 2010-02-23 00:21:36 PST
Comment on attachment 48982 [details]
updated patch

clearing flags, patch was commited
Comment 14 Philippe Normand 2010-02-23 08:15:20 PST
Closing as the patch was commited. See Bug 35297 from now on ;)