The default bug view has changed. See this FAQ.

WebM video rendered with incorrect aspect ratio

VERIFIED FIXED in mozilla7

Status

()

Core
Audio/Video
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: kinetik, Assigned: cpearce)

Tracking

({regression})

Trunk
mozilla7
x86_64
All
regression
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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
Keywords: regression
OS: Linux → All
Assignee: nobody → chris
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
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
Attachment #540910 - Flags: review?(kinetik)
(Assignee)

Comment 3

6 years ago
Created attachment 540911 [details] [diff] [review]
Patch: Scale WebM to display dimensions

Oops, forgot to `hg qref` after fixing comments. ;|
Attachment #540910 - Attachment is obsolete: true
Attachment #540910 - Flags: review?(kinetik)
Attachment #540911 - Flags: review?(kinetik)
(Reporter)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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.
Attachment #540911 - Attachment is obsolete: true
Attachment #540911 - Flags: review?(kinetik)
Attachment #541200 - Flags: review?(kinetik)
(Reporter)

Updated

6 years ago
Attachment #541200 - Flags: review?(kinetik) → review+
(Assignee)

Comment 6

6 years ago
Landed on mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6d87b94b1b12
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/6d87b94b1b12
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Test?
Flags: in-testsuite?

Comment 9

6 years ago
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.
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
(Assignee)

Comment 10

6 years ago
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?
Attachment #541200 - Flags: approval-mozilla-aurora?

Comment 11

6 years ago
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.
Attachment #541200 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Depends on: 669237

Comment 12

6 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.