Closed Bug 970787 Opened 6 years ago Closed 6 years ago

Media Recording - Playback of recorded video "ducks_take_off_444_720p25.ogg" is glitchy

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jsmith, Assigned: bechen)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 6 obsolete files)

Build - 2/10/2014 Nightly

STR

1. Go to http://mozilla.github.io/qa-testcase-data/webapi/mediarecorder/
2. Under "Setup Stream for Media Recorder by File", select:
** File: ducks_take_off_444_720p25.ogg
** Media Type: video
** Mime Type: video/webm
3. Select play on both sets of media controls to start playback of the video
4. Select start recording
5. When the video stops playing, select the generated blob URL

Expected

The recorded video should playback the same way as the original video.

Actual

The recorded video experiences black glitches during playback, where as the original video does not.
Attached video Original Video
Attached video Recorded Video
Look like the quality issue.
(In reply to Jason Smith [:jsmith] from comment #0)
> The recorded video experiences black glitches during playback, where as the
> original video does not.

Less obvious in a mostly-blue video, but this also gets the color channels wrong, for the same reason as in bug 970776 (treating the data as 4:2:0 even when it is 4:2:2, as in that bug, or 4:4:4, as in this one).
Assignee: nobody → bechen
Attached patch bug-970787.patch (obsolete) — Splinter Review
1. add |ConvertPlanarYCbCrToI420| functino to convert the 4:4:4 and 4:2:2 into 4:2:0
2. add |isYUV420| to check the YUV data.
Comment on attachment 8378781 [details] [diff] [review]
bug-970787.patch

Review of attachment 8378781 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +297,5 @@
> +      vSrc += vPixStride;
> +    }
> +    // Pick next source line.
> +    v += lineStride;
> +  }

// U plane
// V plane
These two code chunk are duplicate. Move to an inline function

For exp:
CopyUVPlan(aSource->mCbChannel, aDestination, uPixStride, ySize);
CopyUVPlan(aSource->mCrChannel, aDestination, vPixStride, ySize);

void CopyUVPlan(uint8_t* aSrc, uint8_t*& aDestination, size_t aPixStride, const IntSize &aYPlanSize) 
{
  size_t uvWidth = aYPlanSize.width / 2;
  size_t uvHeight = aYPlanSize.height / 2;

  for (int i = 0; i < uvHeight; i++) {
    // 1st pixel per line.
    uint8_t* src = aSrc;
    for (int j = 0; j < uvWidth; j++) {
      *aDestination++ = *src;
      // Pick next source pixel.
      src += aPixStride;
    }
    
    // Pick next source line.
    aSrc += lineStride;
  }
}

@@ +342,5 @@
> +      uint32_t halfWidth = (mFrameWidth + 1) / 2;
> +      uint32_t halfHeight = (mFrameHeight + 1) / 2;
> +      uint32_t uvPlanSize = halfWidth * halfHeight;
> +
> +      if (mI420Frame.IsEmpty()) {

if (mI420Frame.Length() < (yPlanSize + uvPlanSize * 2)) {
  mI420Frame.SetLength(yPlanSize + uvPlanSize * 2);
}

@@ +353,5 @@
> +      uint8_t *cr = mI420Frame.Elements() + yPlanSize + uvPlanSize;
> +
> +      mVPXImageWrapper->planes[PLANE_Y] = y;
> +      mVPXImageWrapper->planes[PLANE_U] = cb;
> +      mVPXImageWrapper->planes[PLANE_V] = cr;

mVPXImageWrapper->planes[PLANE_Y] = mI420Frame.Elements();
mVPXImageWrapper->planes[PLANE_U] = mI420Frame.Elements() + yPlanSize;
mVPXImageWrapper->planes[PLANE_V] = mI420Frame.Elements() + yPlanSize + uvPlanSize;

@@ +357,5 @@
> +      mVPXImageWrapper->planes[PLANE_V] = cr;
> +      mVPXImageWrapper->stride[VPX_PLANE_Y] = mFrameWidth;
> +      mVPXImageWrapper->stride[VPX_PLANE_U] = halfWidth;
> +      mVPXImageWrapper->stride[VPX_PLANE_V] = halfWidth;
> +

Empty line

::: content/media/encoder/VP8TrackEncoder.h
@@ +14,1 @@
>  

Why need to using namespace in header?
Benjamin, 
is there a utility in libyuv can do this conversion for you?
Please check this header.
media/libyuv/include/libyuv/convert.h
(In reply to C.J. Ku[:CJKu] from comment #7)
> Benjamin, 
> is there a utility in libyuv can do this conversion for you?
> Please check this header.
> media/libyuv/include/libyuv/convert.h

Yes, I'll use libyuv on the next patch.
Attached patch bug-970787.patch (obsolete) — Splinter Review
Use libyuv for color conversion.
Attachment #8378781 - Attachment is obsolete: true
Attached file YUVFormatChecker.cpp
Hi Benjamin,
Both here and OMXVideoTrack encoder path need to check YUV format of source image, I suggest to create a utility and look deeper into PlanarYCbCrImage::Data field, and progressively detect more YUV format, such as NV24/NV42.

Here I attach an prototype of this utility, please discuss with John and come out more robust solution
Look reasonable, we should have a helper class to do this.
(In reply to Randy Lin [:rlin] from comment #11)
> Look reasonable, we should have a helper class to do this.

 I would suggest making them PlanarYCbCrImage::Data methods instead.
Attached patch bug-970787.patch (obsolete) — Splinter Review
Hi Ralph:
Please help review this patch.
In this patch, I use libyuv to convert the image format from 444 and 422 to I420.
Attachment #8379600 - Attachment is obsolete: true
Attachment #8380528 - Flags: review?(giles)
Attached patch bug-970787.patch (obsolete) — Splinter Review
bug fix.

+static bool isYUV420(const PlanarYCbCrImage::Data *aData)
+{
+  if (aData->mYSize == aData->mCbCrSize * 4) {
                                           ^^ 
This should be 2.
Attachment #8380528 - Attachment is obsolete: true
Attachment #8380528 - Flags: review?(giles)
Attachment #8380547 - Flags: review?(giles)
Component: Video/Audio → Video/Audio: Recording
Comment on attachment 8380547 [details] [diff] [review]
bug-970787.patch

Review of attachment 8380547 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits.

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +24,5 @@
>  
>  #define DEFAULT_BITRATE 2500 // in kbit/s
>  #define DEFAULT_ENCODE_FRAMERATE 30
>  
> +using namespace mozilla::layers;

How about:

using layers::Image;
using layers::PlanarYCbCrImage;

to avoid polluting the namespace?

@@ +248,5 @@
>  }
>  
> +static bool isYUV420(const PlanarYCbCrImage::Data *aData)
> +{
> +  if (aData->mYSize == aData->mCbCrSize * 2) {

Yes. Reasoning:

mCbCrSize is a gfx::IntSize, which ultimately inherits from BaseSize in gfx/2d/BaseSize.h. As such, it's a two-element vector. Comparison compares width and height separately. Multiplication by a scalar is normal for vectors, multiplying width and height separately. Thus for 4:2:0 they represent subsampling in each direction (factor of two in coordinate size) not subsampling in the product of both directions (factor of four in buffer size).

@@ +302,5 @@
> +      mVPXImageWrapper->stride[VPX_PLANE_Y] = data->mYStride;
> +      mVPXImageWrapper->stride[VPX_PLANE_U] = data->mCbCrStride;
> +      mVPXImageWrapper->stride[VPX_PLANE_V] = data->mCbCrStride;
> +    } else {
> +      uint32_t yPlanSize = mFrameWidth * mFrameHeight;

yPlaneSize

@@ +305,5 @@
> +    } else {
> +      uint32_t yPlanSize = mFrameWidth * mFrameHeight;
> +      uint32_t halfWidth = (mFrameWidth + 1) / 2;
> +      uint32_t halfHeight = (mFrameHeight + 1) / 2;
> +      uint32_t uvPlanSize = halfWidth * halfHeight;

uvPlaneSize

@@ +308,5 @@
> +      uint32_t halfHeight = (mFrameHeight + 1) / 2;
> +      uint32_t uvPlanSize = halfWidth * halfHeight;
> +      if (mI420Frame.IsEmpty()) {
> +        mI420Frame.SetLength(yPlanSize + uvPlanSize * 2);
> +      }

If you're assuming the image data is always the same dimensions, please assert that here.

@@ +346,5 @@
> +                           y, mFrameWidth,
> +                           cb, halfWidth,
> +                           cr, halfWidth,
> +                           mFrameWidth, mFrameHeight);
> +      } else {

Please V8Log("Unsupported planar format\n") here so we get a warning from the failure site.
Attachment #8380547 - Flags: review?(giles) → review+
Attached patch bug-970787.patch (obsolete) — Splinter Review
r=rillian
try server: https://tbpl.mozilla.org/?tree=Try&rev=bafde066102f
Attachment #8380547 - Attachment is obsolete: true
Attachment #8386581 - Flags: review+
Attached patch bug-970787.patch (obsolete) — Splinter Review
fix nits.
try server: https://tbpl.mozilla.org/?tree=Try&rev=a69e699673f3
Attachment #8386581 - Attachment is obsolete: true
Attachment #8387349 - Flags: review+
Attached patch bug-970787.patchSplinter Review
Modify moz.build:
Move |LOCAL_INCLUDES| to condition |MOZ_WEBM_ENCODER| .

try server: https://tbpl.mozilla.org/?tree=Try&rev=8e1e975e7341
Attachment #8387349 - Attachment is obsolete: true
Attachment #8387400 - Flags: review+
Keywords: checkin-needed
Blocks: 951044
https://hg.mozilla.org/mozilla-central/rev/0c2dd5c132fb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
This breaks -flto build (both with gcc and llvm):

/tmp/ccdyBHie.ltrans13.ltrans.o:ccdyBHie.ltrans13.o:function mozilla::VP8TrackEncoder::GetEncodedTrack(mozilla::EncodedFrameContainer&): error: undefined reference to 'I444ToI420'
/tmp/ccdyBHie.ltrans13.ltrans.o:ccdyBHie.ltrans13.o:function mozilla::VP8TrackEncoder::GetEncodedTrack(mozilla::EncodedFrameContainer&): error: undefined reference to 'I422ToI420'
/tmp/ccdyBHie.ltrans13.ltrans.o:ccdyBHie.ltrans13.o:function mozilla::VP8TrackEncoder::GetEncodedTrack(mozilla::EncodedFrameContainer&): error: undefined reference to 'NV12ToI420'
/tmp/ccdyBHie.ltrans13.ltrans.o:ccdyBHie.ltrans13.o:function mozilla::VP8TrackEncoder::GetEncodedTrack(mozilla::EncodedFrameContainer&): error: undefined reference to 'NV21ToI420'
collect2: error: ld returned 1 exit status
Depends on: 981780
Duplicate of this bug: 970776
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.