Bug 34933 - media/video-display-aspect-ratio.html fails
: media/video-display-aspect-ratio.html fails
Status: RESOLVED FIXED
: WebKit
Media Elements
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 35297
  Show dependency treegraph
 
Reported: 2010-02-15 01:35 PST by
Modified: 2010-02-23 08:15 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


------- Comment #1 From 2010-02-15 01:37:09 PST -------
The commit that introduced the new test is http://trac.webkit.org/changeset/54766
------- Comment #2 From 2010-02-15 01:52:23 PST -------
Test skipped as of r54768
------- Comment #3 From 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 From 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 From 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 From 2010-02-17 16:35:48 PST -------
Created an attachment (id=48945) [details]
Proposed patch
------- Comment #7 From 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 From 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 From 2010-02-18 00:42:15 PST -------
Created an attachment (id=48982) [details]
updated patch
------- Comment #10 From 2010-02-18 00:44:45 PST -------
(In reply to comment #9)
> Created an attachment (id=48982) [details] [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 From 2010-02-22 13:47:58 PST -------
(From update of attachment 48982 [details])
OK.
------- Comment #12 From 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 From 2010-02-23 00:21:36 PST -------
(From update of attachment 48982 [details])
clearing flags, patch was commited
------- Comment #14 From 2010-02-23 08:15:20 PST -------
Closing as the patch was commited. See Bug 35297 from now on ;)