Bug 34790 – [Gtk] wrong video aspect ratio

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

In the linked page the video is displayed stretched. In naturalSize() after applying the PAR we should rescale the width and height.

[Philippe Normand]

Created attachment 48502 [details]
proposed patch

[Sebastian Dröge (slomo)]

(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

[Sebastian Dröge (slomo)]

(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.

[Philippe Normand]

Created attachment 48506 [details]
proposed patch

and updated as per slomo suggestion

[WebKit Review Bot]

Attachment 48506 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/257303

[Philippe Normand]

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.

[Gustavo Noronha (kov)]

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 =)

[Philippe Normand]

Created attachment 48633 [details]
updated patch (with test)

[Gustavo Noronha (kov)]

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!

[Philippe Normand]

Thanks ;) Landed as http://trac.webkit.org/changeset/54766 (with git-svn, Gustavo! :))