Last Comment Bug 661456 - WebM video rendered with incorrect aspect ratio
: WebM video rendered with incorrect aspect ratio
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: mozilla7
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
http://www.schleef.org/~ds/par-4-3.webm
Depends on: 669237
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-01 19:01 PDT by Matthew Gregan [:kinetik]
Modified: 2011-08-30 07:49 PDT (History)
7 users (show)
roc: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
copy of linked testcase (230.41 KB, video/webm)
2011-06-01 19:01 PDT, Matthew Gregan [:kinetik]
no flags Details
Patch: Scale WebM to display dimensions (34.36 KB, patch)
2011-06-21 15:21 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Review
Patch: Scale WebM to display dimensions (34.24 KB, patch)
2011-06-21 15:23 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Review
Patch v2 (37.98 KB, patch)
2011-06-22 15:37 PDT, Chris Pearce (:cpearce)
kinetik: review+
asa: approval‑mozilla‑aurora-
Details | Diff | Review

Description Matthew Gregan [:kinetik] 2011-06-01 19:01:01 PDT
Created attachment 536811 [details]
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 2011-06-01 22:49:42 PDT
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) 2011-06-21 15:21:33 PDT
Created attachment 540910 [details] [diff] [review]
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) 2011-06-21 15:23:50 PDT
Created attachment 540911 [details] [diff] [review]
Patch: Scale WebM to display dimensions

Oops, forgot to `hg qref` after fixing comments. ;|
Comment 4 Matthew Gregan [:kinetik] 2011-06-21 20:30:00 PDT
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) 2011-06-22 15:37:41 PDT
Created attachment 541200 [details] [diff] [review]
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) 2011-06-23 15:26:45 PDT
Landed on mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6d87b94b1b12
Comment 7 Marco Bonardo [::mak] 2011-06-24 02:54:33 PDT
http://hg.mozilla.org/mozilla-central/rev/6d87b94b1b12
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-24 03:13:25 PDT
Test?
Comment 9 Mathieu Pellerin 2011-06-24 22:45:01 PDT
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) 2011-06-24 23:14:22 PDT
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] 2011-06-28 15:22:22 PDT
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] 2011-08-30 07:49:05 PDT
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.

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