Closed Bug 556889 Opened 14 years ago Closed 14 years ago

Aspect ratio incorrect in new Ogg decoder


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




Tracking Status
blocking2.0 --- beta1+


(Reporter: cpearce, Assigned: cpearce)




(Keywords: regression)


(1 file, 4 obsolete files)

      No description provided.
We regressed bug 480058 when we landed the new Ogg decoder. We're not handling aspect ration correctly. See example URL.
Summary: Aspect ratio incorrect in new → Aspect ratio incorrect in new Ogg decoder
blocking2.0: --- → ?
Keywords: regression
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → chris
Attachment #437182 - Flags: review?(roc)
+    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.
Attached patch Patch v2 (obsolete) — Splinter Review
Add and use ImageContainer::GetSize() instead of ImageContainer::GetCurrentAsSurface(gfxIntSize*).
Attachment #437182 - Attachment is obsolete: true
Attachment #437217 - Flags: review?(roc)
Attachment #437182 - Flags: review?(roc)
+  /**
+   * 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).

+  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 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...
Attachment #437217 - Attachment is obsolete: true
Attachment #437217 - Flags: review?(roc)
blocking2.0: ? → beta1+
Priority: -- → P2
(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.
Attached patch Patch v3 (obsolete) — Splinter Review
As v2, but with null checks to avoid assertions.
Attachment #437500 - Flags: review?(roc)
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.
Pushed to mozilla-central, rebased to take into account Bas's checkin. Included adding lock call to ImageContainerOGL::GetCurrentSize().
Closed: 14 years ago
Resolution: --- → FIXED
Reftest also failed on Linux debug, but passed on Mac debug reftests. Nice.
Attached patch Patch v3 - with test fixed (obsolete) — Splinter Review
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.
Attachment #437500 - Attachment is obsolete: true
Attachment #440120 - Flags: review+
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.
Attachment #440120 - Attachment is obsolete: true
Attachment #440122 - Flags: review+
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.3a5
The test added in this patch fails on Mac OS X 10.6 -- that's filed as bug 562000.
You need to log in before you can comment on or make changes to this bug.