Bug 34790 - [Gtk] wrong video aspect ratio
Summary: [Gtk] wrong video aspect ratio
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://people.ubuntu.com/~elle.uca/us...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-10 02:52 PST by Philippe Normand
Modified: 2010-02-15 00:49 PST (History)
4 users (show)

See Also:


Attachments
proposed patch (4.53 KB, patch)
2010-02-10 07:11 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (4.75 KB, patch)
2010-02-10 08:39 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (5.93 KB, patch)
2010-02-10 09:35 PST, Philippe Normand
gustavo: review-
Details | Formatted Diff | Diff
updated patch (with test) (169.15 KB, patch)
2010-02-12 01:21 PST, Philippe Normand
gustavo: review+
gustavo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2010-02-10 02:52:44 PST
In the linked page the video is displayed stretched. In naturalSize() after applying the PAR we should rescale the width and height.
Comment 1 Philippe Normand 2010-02-10 07:11:34 PST
Created attachment 48502 [details]
proposed patch
Comment 2 Sebastian Dröge (slomo) 2010-02-10 07:33:03 PST
(In reply to comment #1)
> Created an attachment (id=48502) [details]
> proposed patch

You should divide the display width/height by their greatest common divisor before the great if-else block, otherwise you'll get very interesting results :)

Other than that this looks good
Comment 3 Sebastian Dröge (slomo) 2010-02-10 07:33:47 PST
(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=48502) [details] [details]
> > proposed patch
> 
> You should divide the display width/height by their greatest common divisor
> before the great if-else block, otherwise you'll get very interesting results
> :)

To be more specific, you could get overflows, it doesn't change the results for the non-overflow cases.
Comment 4 Philippe Normand 2010-02-10 08:39:47 PST
Created attachment 48506 [details]
proposed patch

and updated as per slomo suggestion
Comment 5 WebKit Review Bot 2010-02-10 08:42:52 PST
Attachment 48506 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/257303
Comment 6 Philippe Normand 2010-02-10 09:35:04 PST
Created attachment 48508 [details]
proposed patch

Imported the gcd function from GStreamer git and applied the WebKit
coding style to it. So I had to rename it.
Comment 7 Gustavo Noronha (kov) 2010-02-10 09:56:07 PST
Comment on attachment 48508 [details]
proposed patch

 62 gint greatestCommonDivisor(gint a, gint b)

No need to use gint for these. I think xan will agree =D

 491     int width = 0, height = 0;
 492     GstCaps* caps = GST_PAD_CAPS(pad);
 493     gint pixelAspectRatioNumerator, pixelAspectRatioDenominator;
 494     gint displayWidth, displayHeight, darGcd;

Same here. Also GCD is an acronym, so it should go all upper-cased. I would also prefer to see displayAspectRatioGCD.

I wonder if we can include a layout test with this one. We used to have this code added to fix a layout test, if I remember correctly, so we should be able to use it as a base, provided that we can add a video that triggers it to the tree. r- so we can have that test, as discussed on IRC =)
Comment 8 Philippe Normand 2010-02-12 01:21:58 PST
Created attachment 48633 [details]
updated patch (with test)
Comment 9 Gustavo Noronha (kov) 2010-02-14 08:31:46 PST
Comment on attachment 48633 [details]
updated patch (with test)

 47 gint greatestCommonDivisor(gint a, gint b);

You don't need to expose this function in the header. Just make it static, and remove it from the .h (it's only used inside that file). Pay attention while landing, you may need to use an svn checkout =(, I am not sure git svn will handle the new binary file well. Thanks!
Comment 10 Philippe Normand 2010-02-15 00:49:17 PST
Thanks ;) Landed as http://trac.webkit.org/changeset/54766 (with git-svn, Gustavo! :))