661456 - WebM video rendered with incorrect aspect ratio

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

Attachment: copy of linked testcase

Linked video has pixel dimensions of 320x240 and display dimensions of 426x240.  Firefox renders the video as 567x240.  This is caused by calculating a pixel aspect ratio of 1.33125 in nsWebMReader::ReadMetadata and then applying it to the horizontal display dimension when rendering (floor(426 * 1.33125) = 567).

This is somewhat related to bug 656040, but that's about mishandling of  DisplayUnit=3 whereas the testcase linked here is using DisplayUnit=0.

Comment 1

[Alice0775 White]

Regression window(cached m-c hourly):
Works, 426x240:
http://hg.mozilla.org/mozilla-central/rev/733c4bc77ab5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre ID:20110127221516
Fails, 567x240:
http://hg.mozilla.org/mozilla-central/rev/0e50480c408f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre ID:20110127235726
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=733c4bc77ab5&tochange=0e50480c408f
Suspected Bug:
408332b0a834	Chris Pearce — Bug 626979 - Handle WebM frame size changes. r=kinetik a=blocking2.0

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: Scale WebM to display dimensions

WebM video should just be displayed at the dimensions specified in Display{Width,Height}. We don't need to do fancy/complicated aspect ratio calculations, just render at the size specified in the container.

So factor out the aspect ratio calculation up into the various backends, and perform it specific to each media type. Ogg and raw calculate their display size using the existing scaling WRT aspect ratio code, and WebM just uses the display region from its metadata as the display dimensions.

The following testcase render with the correct aspect ratio with this patch applied:
1. http://gazooze.com/productions/?prod=42 (bug 656040, we should actually reject or warn about the DisplayUnit=3 in this file, but it renders at what appears to be the correct aspect ratio now)
2. https://bugzilla.mozilla.org/attachment.cgi?id=505032 (bug 626979, regression causing bug)
3. https://bugzilla.mozilla.org/attachment.cgi?id=536811 (this bug's testcase).

Green on Try: http://tbpl.mozilla.org/?tree=Try&rev=09f863550eca

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: Scale WebM to display dimensions

Oops, forgot to `hg qref` after fixing comments. ;|

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 540911 [details] [diff] [review]
Patch: Scale WebM to display dimensions

Review of attachment 540911 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/VideoUtils.cpp
@@ +199,5 @@
> +
> +static PRInt32 ConditionDimension(float aValue, PRInt32 aDefault)
> +{
> +  // This will exclude NaNs and infinities
> +  if (aValue >= 1.0 && aValue <= 10000.0)

I know this is just moving from another file, but it seems like we should use MAX_VIDEO_{WIDTH,HEIGHT} (or something) rather than a magic number for the upper bound.

::: content/media/VideoUtils.h
@@ +40,5 @@
>  #define VideoUtils_h
>  
>  #include "mozilla/ReentrantMonitor.h"
>  
> +#include "nsMathUtils.h"

nsMathUtils can be moved into the cpp file.

::: content/media/nsBuiltinDecoderReader.h
@@ +64,5 @@
>  
>    // Returns PR_TRUE if it's safe to use aPicture as the picture to be
>    // extracted inside a frame of size aFrame, and scaled up to and displayed
>    // at a size of aDisplay. You should validate the frame, picture, and
> +  // display regions before using them to display ovideo frames.

Typo.

::: content/media/raw/nsRawReader.cpp
@@ +237,5 @@
>                                     currentFrameTime + (USECS_PER_S / mFrameRate),
>                                     b,
>                                     1, // In raw video every frame is a keyframe
> +                                   -1,
> +                                   nsIntRect(0, 0, mMetadata.frameWidth, mMetadata.frameHeight));

Shouldn't this be passing mDisplay?

Also, rename mDisplay to mPicture for consistency with the other two readers.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

Review comments addressed. 

> ::: content/media/VideoUtils.cpp
> > +static PRInt32 ConditionDimension(float aValue, PRInt32 aDefault)
> [...]
> I know this is just moving from another file, but it seems like we should
> use MAX_VIDEO_{WIDTH,HEIGHT} (or something) rather than a magic number for
> the upper bound.

I opted to rely on nsVideoInfo::ValidateVideoRegion() to check the size of the frame after being scaled by the aspect ratio, instead of verifying the aspect ratio directly with some arbitrary number.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Landed on mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6d87b94b1b12

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/6d87b94b1b12

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Test?

Comment 9

[Mathieu Pellerin]

I haven't checked whether Firefox's Aurora branch (upcoming F6.0) is affected by this ratio issue. If it is, devs should think about applying this patch to that branch before it reaches beta. 

This fix has a very positive impact on online webm experience.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 541200 [details] [diff] [review]
Patch v2

Yes, landing this patch on aurora is a good idea. This patch fixes a regression which makes some WebM videos display at an incorrect size/aspect ratio. Can I have approval to land this on aurora please?

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 541200 [details] [diff] [review]
Patch v2

We're not going to rush this in in the last week of Aurora. Please let it go through the regular uplift process. Thanks.

Comment 12

[Vlad [QA]]

Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2

The video from the test case is displayed in 426x240.