Closed Bug 626979 Opened 9 years ago Closed 9 years ago

WebM video freeze picture when changing resolution

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b11
Tracking Status
blocking2.0 --- final+

People

(Reporter: sgadrat, Assigned: cpearce)

References

()

Details

(Whiteboard: [softblocker])

Attachments

(2 files, 3 obsolete files)

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.
A video generated with some switching between two resolutions to ease of reproducing
I can reproduce on 
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre ID:20110119030331
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
Blocks: 566247
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
Version: unspecified → Trunk
I forgot mention to comment #3
I tested with attachment 505032 [details] only
Tim, could this be a regression in libvpx? Blocking on the assumption that it is.
blocking2.0: ? → final+
(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.
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.
Assignee: tterribe → chris
Status: NEW → ASSIGNED
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.
Attached patch Patch v1 (obsolete) — Splinter Review
Handle changes in video frame sizes. This was greenish on TryServer.
Attachment #505672 - Flags: review?(kinetik)
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.
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.
This patch is OK, but I think we should handle dynamic changes to the video intrinsic size.
       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.
(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#
From https://groups.google.com/a/webmproject.org/group/webm-discuss/msg/e494486de530965d

On Jan 25, 4:19 am, John Koleszar <jkoles...@google.com> 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.
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #505672 - Attachment is obsolete: true
Attachment #506517 - Flags: review?(kinetik)
Attachment #505672 - Flags: review?(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.
Attached patch Patch v3 (obsolete) — Splinter Review
With increasingly paranoid checks on frame sizes during metadata loading. Still greenish on TryServer.
Attachment #506517 - Attachment is obsolete: true
Attachment #507220 - Flags: review?(kinetik)
Attachment #506517 - Flags: review?(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.
Attachment #507220 - Flags: review?(kinetik) → review+
+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.
Attached patch Patch v4Splinter Review
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.
Attachment #507220 - Attachment is obsolete: true
Attachment #507307 - Flags: review+
Keywords: checkin-needed
Whiteboard: [softblocker] → [softblocker][needs-landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/408332b0a834
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: [softblocker][needs-landing] → [softblocker]
Target Milestone: --- → mozilla2.0b11
Status: RESOLVED → VERIFIED
Depends on: 656040
You need to log in before you can comment on or make changes to this bug.