626979 - WebM video freeze picture when changing resolution

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Sylvain Gadrat]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b10pre) Gecko/20110118 Firefox-4.0/4.0b10pre
Build Identifier: Minefield 4b10pre (2011-01-18)

Some adaptive bitrate WebM streams modify the picture size of the video on the fly. When it happens Firefox freeze the picture and continue to play the sound.

Tested on Windows 32bits with Firefox 4b9 recently released and on Ubuntu/Linux with Minefield 4b10pre (2011-01-18) from nightly build ppa.

Reproducible: Always

Steps to Reproduce:
1. Open a page with a <video> sourcing from a file with dimension changing
2. Play the video
3. Wait for freeze
Actual Results:  
The video freeze and the sound continue to play

Expected Results:  
play the video and the sound

Tested on :
 - Windows 32bits with Firefox 4b9 recently released
 - Ubuntu/Linux with Minefield 4b10pre (2011-01-18) from nightly build ppa.

Comment 1

[Sylvain Gadrat]

Attachment: WebM file with dimension changing video

A video generated with some switching between two resolutions to ease of reproducing

Comment 2

[Alice0775 White]

I can reproduce on 
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre ID:20110119030331

Comment 3

[Alice0775 White]

This webm build works
http://hg.mozilla.org/mozilla-central/rev/fbb9eff859cb
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a4webm) Gecko/20100518 MozillaDeveloperPreview/3.7a4webm ID:20100518233040

Broken since:
http://hg.mozilla.org/mozilla-central/rev/d2b01fbc5480
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100608 Minefield/3.7a5pre ID:20100608174111
Bug 566247 - Merge libvpx to mozilla-central

Comment 4

[Alice0775 White]

I forgot mention to comment #3
I tested with attachment 505032 [details] only

Comment 5

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

Tim, could this be a regression in libvpx? Blocking on the assumption that it is.

Comment 6

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

(In reply to comment #3)
> This webm build works
> http://hg.mozilla.org/mozilla-central/rev/fbb9eff859cb
> Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a4webm)
> Gecko/20100518 MozillaDeveloperPreview/3.7a4webm ID:20100518233040

I managed to track down the developer preview 3.7awebm build which Alice tested on [ftp://ftp.mozilla.org/pub/firefox/releases/devpreview/1.9.3a4webm/] and I can confirm that we do indeed play the resolution change in the video attached to this bug. We don't change the frame size during playback though, so we assume libpvx is scaling the image down to match the initial frame size.

libvpx apparently had a feature which performed this scaling and resolution change, but it must have been removed or disabled.

The videos at the QuavStreams page in the bug URL never WFM with the dev preview.


> Broken since:
> http://hg.mozilla.org/mozilla-central/rev/d2b01fbc5480
> Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre)
> Gecko/20100608 Minefield/3.7a5pre ID:20100608174111
> Bug 566247 - Merge libvpx to mozilla-central

This is the day after our initial landing of WebM support on mozilla-central. libvpx was updated at least once in between 1.9.3a4webm and our landing on m-c, which is probably when the feature was lost from libvpx. So this never worked on trunk.

Comment 7

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

It looks like this wasn't actually caused by libvpx removing some feature. We landed http://hg.mozilla.org/mozilla-central/rev/e530c2b50c0a after the dev preview was spun, but before we landed libvpx on m-c, and this installed a check in VideoData::Create() which causes us to refuse to create a video frame if we detect that the frame size has changed. If I disable this check [http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoderReader.cpp#103] then we return to the behaviour we had in the dev preview. This behaviour is actually incorrect; we're clipping the new larger frame at the size of the original smaller frame. Chrome is also doing this FWIW. That just requires a fix in the video frame creation to ensure we're using the frame dimensions rather than the container specified dimensions.

So the behviour we'll aim for is to keep the video at the dimensions specified by the container (which can be overruled by setting @width and @height on the video element) and scaling the frame to that size.

I'll take this bug off Tim, since he's more of a decoder library guy than a playback engine guy.

Comment 8

[Sylvain Gadrat]

Just to say thank you.

 1. For finding the problem so fast.
 2. For working on the problem.
 3. For solving more than just what reported.

The cropping bug is present in every browser I have tested. If it will be solved, it will make Firefox the first browser handling it correctly. Good news for adaptive streaming.

So, thank you.

Comment 9

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

Attachment: Patch v1

Handle changes in video frame sizes. This was greenish on TryServer.

Comment 10

[Sylvain Gadrat]

I tested your Patch v1 on a today mercurial checkout. It works very well.

The only thing, in nsBuiltinDecoderReader.cpp, the line comparing aInfo's width/height to aBuffer's ones :

"
+  if (aInfo.mPicture.width == aBuffer.mPlanes[0].mWidth &&
+      aInfo.mPicture.height == aBuffer.mPlanes[0].mHeight)
"

Generate a compilation warning due to the comparison between a PRInt32 and a PRUInt32. Don't know if it matter to you.

Comment 11

[Sylvain Gadrat]

I do not seriously tested, but what happen if the aspect ratio change ? I am thinking about live television streams which sometimes change their aspect ratio.

Currently the picture seems to be stretched to the original aspect ratio.

Comment 12

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

This patch is OK, but I think we should handle dynamic changes to the video intrinsic size.

Comment 13

[Matthew Gregan [:kinetik]]

       mInfo.mPixelAspectRatio = (float(params.display_width) / params.width) /
                                 (float(params.display_height) / params.height);

Not that you touched this, but since it's in the same area, we should fix it.  It looks like this should be computed from the picture region, not the frame size: http://matroska.org/technical/specs/notes.html#Cropping

+  if (aInfo.mPicture.width == aBuffer.mPlanes[0].mWidth &&
+      aInfo.mPicture.height == aBuffer.mPlanes[0].mHeight)
+  {
+    picX = aInfo.mPicture.x;
+    picY = aInfo.mPicture.y;
+    picSize = gfxIntSize(aInfo.mPicture.width, aInfo.mPicture.height);

The picture region is computed from the frame size minus the crop, so you wouldn't expect this to ever match the decoded frame size if a crop is specified.  Based on the comment, you want to be comparing mFrame width/height against the buffer width/height.

+  } else {
+    // Frame size is different from what the container reports. This is legal, and we
+    // will preserve the ratio of the crop rectangle as it was reported relative to the
+    // frame size reported by the container.
+    picX = (aInfo.mPicture.x * aBuffer.mPlanes[0].mWidth) / aInfo.mFrame.width;
+    picY = (aInfo.mPicture.y * aBuffer.mPlanes[0].mHeight) / aInfo.mFrame.height;
+    PRUint32 w = (aBuffer.mPlanes[0].mWidth * aInfo.mPicture.width) / aInfo.mFrame.width;
+    PRUint32 h = (aBuffer.mPlanes[0].mHeight * aInfo.mPicture.height) / aInfo.mFrame.height;
+    picSize = gfxIntSize(w,h);
+  }

It's not clear that scaling the crop size is the right thing to do, but that'll need to be clarified on the WebM mailing list.

Comment 14

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

(In reply to comment #13)
> It's not clear that scaling the crop size is the right thing to do, but that'll
> need to be clarified on the WebM mailing list.

Just posted...

https://groups.google.com/a/webmproject.org/group/webm-discuss/browse_thread/thread/cc3221910efd4770#

Comment 15

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

From https://groups.google.com/a/webmproject.org/group/webm-discuss/msg/e494486de530965d

On Jan 25, 4:19 am, John Koleszar <[e-mail]> wrote:
[...]
> So the operation you should perform should be equivalent to: scale up
> to PixelWxH, crop according to PixelCrop, scale to DisplayWxH.

I think what we're doing is equivalent to this.

Comment 16

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

Attachment: Patch v2

With review comments addressed. I realised that I need to add another size to nsVideoInfo so that we can properly scale the cropped picture region out of the frame to the container specified display size.

Still greenish on TryServer.

Comment 17

[Matthew Gregan [:kinetik]]

+  PRUint32 picX, picY;
+  gfxIntSize picSize;
+  if (aInfo.mFrame.width == aBuffer.mPlanes[0].mWidth &&
+      aInfo.mFrame.height == aBuffer.mPlanes[0].mHeight)
+  {
+    picX = aInfo.mPicture.x;
+    picY = aInfo.mPicture.y;
+    picSize = gfxIntSize(aInfo.mPicture.width, aInfo.mPicture.height);
+  } else {
+    // Frame size is different from what the container reports. This is legal
+    // in WebM, and we will preserve the ratio of the crop rectangle as it
+    // was reported relative to the picture size reported by the container.
+    picX = (aInfo.mPicture.x * aBuffer.mPlanes[0].mWidth) / aInfo.mFrame.width;
+    picY = (aInfo.mPicture.y * aBuffer.mPlanes[0].mHeight) / aInfo.mFrame.height;
+    PRUint32 w = (aBuffer.mPlanes[0].mWidth * aInfo.mPicture.width) / aInfo.mFrame.width;
+    PRUint32 h = (aBuffer.mPlanes[0].mHeight * aInfo.mPicture.height) / aInfo.mFrame.height;
+    picSize = gfxIntSize(w,h);
+  }

Maybe simpler as:

PRUint32 picX = aInfo.mPicture.x;
PRUint32 picY = aInfo.mPicture.y;
gfxIntSize picSize = gfxIntSize(aInfo.mPicture.width, aInfo.mPicture.height);

if (aInfo.mFrame.width != aBuffer.mPlanes[0].mWidth ||
    aInfo.mFrame.height != aBuffer.mPlanes[0].mHeight)
{
  picX = (aInfo.mPicture.x * aBuffer.mPlanes[0].mWidth) / aInfo.mFrame.width;
  picY = (aInfo.mPicture.y * aBuffer.mPlanes[0].mHeight) / aInfo.mFrame.height;
  picSize = gfxIntSize(aBuffer.mPlanes[0].mWidth * aInfo.mPicture.width / aInfo.mFrame.width,
                       aBuffer.mPlanes[0].mHeight * aInfo.mPicture.height / aInfo.mFrame.height);
}

Do we need to worry about |aBuffer.mPlanes[0].mWidth * aInfo.mPicture.width| (and height) overflowing?

-      mFrame(0,0),

nsIntSize's default constructor doesn't initialize to 0.  So it might be better to keep this, and add another for mDisplay.

Comment 18

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

Attachment: Patch v3

With increasingly paranoid checks on frame sizes during metadata loading. Still greenish on TryServer.

Comment 19

[Matthew Gregan [:kinetik]]

Comment on attachment 507220 [details] [diff] [review]
Patch v3

If there's a way to make the two mega-checks in the Ogg and WebM reader simpler/smaller by adding a new Validate function, that'd be great... other than that, everything looks great.

Comment 20

[cajbir (:cajbir)]

+PR_STATIC_ASSERT(PlanarYCbCrImage::MAX_DIMENSION * PlanarYCbCrImage::MAX_DIMENSION < PR_UINT32_MAX);

The multiplication could overflow at compile time. This should be:

PR_STATIC_ASSERT(MAX_DIMENSION < PR_UINT32_MAX / MAX_DIMENSION)

Also some lines in the patch have windows line endings.

Comment 21

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

Attachment: Patch v4

Patch with comments from Matthew Gregan and Chris Double addressed. I added a static ValidateVideoRegion() method to nsVideoInfo to perform the video sanity checking, and also added the same checks for the display rect as for the frame rect.

Carrying forward r=kinetik.

Comment 22

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/408332b0a834