639415 - Video offsets are ignored when pre-scaling

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Timothy B. Terriberry (:derf)]

If a video has a crop rectangle with non-zero offsets, these are ignored when the image is pre-scaled in BasicPlanarYCbCrImage::SetData().

Steps To Reproduce:
1. Set layers.acceleration.disabled to true (if necessary)
2. Open http://v2v.cc/~j/theora_testsuite/offset_test.ogv
3. Hit Ctrl++ to zoom in.
4. Let the video play through one time.
5. Play the video a second time.

On the second play through, the video is scaled using ScaleYCbCrToRGB32() with aData.mYChannel, aData.mCbChannel, andaData.mCrChannel passed directly. aData.mPicX and aData.mPicY are ignored.

I don't actually know why this doesn't get triggered on the first play through, that may be a separate issue. It may be because we're buffering all the frames before the first one gets played back: only the first two contain any actual data, the rest have no coded blocks, and there are only 10 total. However, the prescale path is definitely not taken, even if the zoom setting is already not 1:1 when the video starts playing for the first time.

Comment 1

[Matthew Gregan [:kinetik]]

As Tim guessed, we don't hit this bug on the first play through because all of the video frames have been converted to BasicPlanarYCbCrImages before BasicImageContainer::SetScaleHint has been called from nsVideoFrame::BuildLayer.  If the video had enough frames we'd eventually switch over to the prescale path and the same bug would be revealed.  On a second play through the scale hint is already set, so the prescale path is taken from the beginning.

ScaleYCbCrToRGB32 doesn't support picture offsets.  I think Chris Double added picture offset support to ConvertYCbCrToRGB32 when it was imported.  Someone would need to add support to ScaleYCbCrToRGB32 to fix this nicely.

Short term, we could bail from the prescale path if the frame has a picture offset.

Comment 2

[Timothy B. Terriberry (:derf)]

Conceptually fixing this in the Chromium code is not that hard... it just involves starting each row with a non-zero x coordinate. But this requires updates to the asm to take an extra parameter.

(In reply to comment #1)
> Short term, we could bail from the prescale path if the frame has a picture
> offset.

This is probably fine for the short term. Better to be a little slow than totally wrong.

But, this begs the question, is it a good idea to be taking the slow path at startup when we know that a) we've decoded a keyframe (already slow) and b) there's little to no buffer of decoded frames to fall back on if we're too slow?

Should I file a separate bug on getting prescale set earlier, or is that just not possible?

Comment 3

[Matthew Gregan [:kinetik]]

I think the scale hint is set from BuildLayer (and not the decoder) because we need box size information from layout to calculate the final size... I cced roc and doublec since they worked on the original implementation and might have a better answer.

Comment 4

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

We could reorganize nsVideoFrame::BuildLayer so that SetScaleHint gets computed and called after
  if (videoSize.width <= 0 || videoSize.height <= 0 || area.IsEmpty())
    return nsnull;
and before we check for an actual image. That should make scale hints available to the decoder much earlier.

We should also fix the prescale path or bail out early if there is a nontrivial crop rectangle.

Comment 5

[Timothy B. Terriberry (:derf)]

Okay, I broke out the "prescale gets used too late" issue into bug 640328.

Comment 6

[cajbir (:cajbir)]

Attachment: Fix

Implemented the short term fix suggested by Matthew in comment 1. I'll raise a new bug to add offset support to gfx/ycbcr scaling routines.

Comment 7

[Timothy B. Terriberry (:derf)]

Okay. I had a slightly more general fix in my patch for bug 634557, since the NEON scaling code there _does_ support picture offsets. I'm still working on breaking that up into separate pieces for all of the issues it addresses, though.

Comment 8

[cajbir (:cajbir)]

So your patch fixes this bug? Would you prefer we wait for that rather than landing this? If so, I'll just make this bug dependent on yours and wait for it.

Comment 9

[Timothy B. Terriberry (:derf)]

I don't want to land that patch as-is, so I have no problem with you landing this in the mean time.

Comment 10

[Matthew Gregan [:kinetik]]

Comment on attachment 519078 [details] [diff] [review]
Fix

Looks fine.  It'd be good to add a reftest that plays through a short video with an offset twice before checking that it's rendered correctly.

Comment 11

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/ce528cff2f8f