556889 - Aspect ratio incorrect in new Ogg decoder

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Comment 1

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

We regressed bug 480058 when we landed the new Ogg decoder. We're not handling aspect ration correctly. See example URL.

Comment 4

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

Attachment: Patch v1

Turns our the layers changes broke aspect ratio handling, and we didn't notice because our aspect ratio reftests actually only test scaling, they don't test when a video has a pixel aspect ratio encoded into the video.

Patch: Changes nsVideoFrame::BuildLayer() so that it will apply a scale transform in proportion to final_video_frame_size / decoded_video_frame_size. Includes reftest.

Comment 5

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

+    nsRefPtr<gfxASurface> imageSurface = container->GetCurrentAsSurface(&sz.mSize);

We really don't want to do this. With GL this will cause a very expensive readback of the pixel data.

We probably should just add a separate API to ImageContainer to get the image size. Return a gfxIntSize; you can make frameSize be a gfxIntSize.

Comment 6

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

Attachment: Patch v2

Add and use ImageContainer::GetSize() instead of ImageContainer::GetCurrentAsSurface(gfxIntSize*).

Comment 7

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

+  /**
+   * Returns the size of the image in pixels.
+   */
+  virtual gfxIntSize GetSize() = 0;

Call this GetCurrentSize(). Should probably document this as safe to call on any thread (or else explain that it's main-thread only).

+gfxIntSize
+BasicImageContainer::GetSize()
+{
+  return ToImageData(mImage)->GetSize();
+}

This should take the monitor.

ImageContainerOGL should take a monitor too. But currently it has no synchronization whatsoever. Bas! Ignore this for now.

Otherwise looks good.

Comment 8

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

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

Oops, this patch causes an impressive number of assertions to fail while running the content/media mochitests... Will revise...

Comment 9

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

(In reply to comment #8)
> Oops, this patch causes an impressive number of assertions to fail while
> running the content/media mochitests... Will revise...

Gah! This was caused by BasicImageContainer::GetCurrentSize() throwing some kind of exception when mImage was nsnull. GetCurrentSize() calls ToImageData() which does static_cast<BasicImageImplData*>(aImage->GetImplData()) which was throwing.

I'll change BasicImageContainer::GetCurrentSize() to return (0,0) when mImage is null, and change nsVideoFrame::BuildLayer() to not create a layer when the image is zero sized.

Comment 10

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

Attachment: Patch v3

As v2, but with null checks to avoid assertions.

Comment 11

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

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

+
+  } else if (mActiveImage->GetFormat() == Image::CAIRO_SURFACE) {

Unnecessary blank line

If Bas lands bug 557671 before you, you'll need to update this patch. Otherwise he'll need to update.

There's a potential race condition here where the current frame can change during nsVideoFrame::BuildLayer. But that's OK because that race condition already exists --- the current frame might also change between BuildLayer and actually compositing the frame tree.

Comment 12

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

Pushed to mozilla-central, rebased to take into account Bas's checkin. Included adding lock call to ImageContainerOGL::GetCurrentSize().

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

Comment 13

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

Reftest failure on Linux Opt of the new test I added:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270716636.1270717570.5251.gz

Backed out:
http://hg.mozilla.org/mozilla-central/rev/26dd5773a08a
http://hg.mozilla.org/mozilla-central/rev/af2fa9a53db5

Comment 14

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

Reftest also failed on Linux debug, but passed on Mac debug reftests. Nice.

Comment 15

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

Attachment: Patch v3 - with test fixed

I spoke to Roc about this. We think the reason the test was failing on Linux is that the scaling to preserve the aspect ratio can sample outside of the visible picture region, resulting on a thin grey line down the edges of the scaled picture. I've updated the test so that it overwrites the edges of the video picture with red boxes, so any such scaling artifacts are overwritten.

Roc: I couldn't figure out how to make the aspect ratio and the letter-box scaling cancel each other out as we discussed, I don't think it's possible as the letter-box scaling is always done such that the aspect ratio is preserved.

Carrying forward r=roc.

Comment 16

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

Yes, I guess that's true.

Comment 17

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

Attachment: Patch v3 - with test fixed & locking in OGL layer

D'oh, I forgot to re-add taking the monitor in ImageContainerOGL::GetCurrentSize(). I had added that when I un-bitrotted before checking in last time, but forgot to re-add it again to the previous patch. Added it in this patch.

Comment 18

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

http://hg.mozilla.org/mozilla-central/rev/3742af50864f

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The test added in this patch fails on Mac OS X 10.6 -- that's filed as bug 562000.