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.

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).

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.

Use libyuv for color conversion.

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.

Hi Ralph: Please help review this patch. In this patch, I use libyuv to convert the image format from 444 and 422 to I420.

bug fix. +static bool isYUV420(const PlanarYCbCrImage::Data *aData) +{ + if (aData->mYSize == aData->mCbCrSize * 4) { ^^ This should be 2.

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.

r=rillian try server: https://tbpl.mozilla.org/?tree=Try&rev=bafde066102f

fix nits. try server: https://tbpl.mozilla.org/?tree=Try&rev=a69e699673f3

Modify moz.build: Move |LOCAL_INCLUDES| to condition |MOZ_WEBM_ENCODER| . try server: https://tbpl.mozilla.org/?tree=Try&rev=8e1e975e7341

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