Aspect ratio incorrect in new Ogg decoder

RESOLVED FIXED in mozilla1.9.3a5

Status

()

defect
P2
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

({regression})

Trunk
mozilla1.9.3a5
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 beta1+)

Details

()

Attachments

(1 attachment, 4 obsolete attachments)

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
Blocks: 531340
Duplicate of this bug: 557093
blocking2.0: --- → ?
Keywords: regression
Duplicate of this bug: 557367
Posted 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.
Posted 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).

+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 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.
Posted 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().

http://hg.mozilla.org/mozilla-central/rev/6f250c9b680b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Reftest also failed on Linux debug, but passed on Mac debug reftests. Nice.
Posted 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+
http://hg.mozilla.org/mozilla-central/rev/3742af50864f
Status: REOPENED → RESOLVED
Closed: 10 years ago9 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.