* STR: 1. upload a video file which format is webm 2. open Video app * Expected result: thumbnail is ok * Actual result: Got a broken thumbnail just like attachment shows

I found this issue when verifying bug 1120780. Michael, could you take a look?

mwu: any update/input here? thanks hema

Inder: Is this also happening on QRD?

before blocking we are waiting to confirm(likely by response in comment# 3) if this is a generic 2.2 and L issue to help understand isolate the device specific behavior that may be seen here.

Yes, we are also seeing thumbnail corruption on our reference devices. Although we have not made the determination if it's gecko or gonk issue.

Sotaro, do you have any insight about this bug? Looks like this is a generic L issue, rather than being specific to nexus 5.

Blocking Reason: broken thumbnail happening on both nexus5 and QRD. it looks like one of the webm thumbnails on the top of the screenshot looks fine. Thanks Hema

Probably another stride issue, though probably on the gecko side of things. Hermes, can you link your test video?

(In reply to Michael Wu [:mwu] from comment #8) > Probably another stride issue, though probably on the gecko side of things. > > Hermes, can you link your test video? I used this one https://github.com/mozilla-b2g/gaia/blob/master/test_media/Movies/elephants-dream.webm

steven, could you help on this?

Hi Blake, Please check this bug, thanks.

I think this should be related to color space alignment. vliu, Can you help check it?

Sure, I can reproduce this issue on Nexus 5. I will try to look into this problem.

It seems that the recent change for ColorConverter in stagefright to cause this problem.

I tried to revert some codes in ColorConverter.cpp in stagefright, it can make sure the problem we meet.

Hi Sotaro, From the Bug 1120780 Comment 59, do we expect the color converter will fall back on lock_ycbcr for the YCbCr_420_SP_VENUS case? Thanks for the comment.

(In reply to Vincent Liu[:vliu] from comment #15) > Created attachment 8593217 [details] [diff] [review] > WIP-revert-color-converter.diff > > I tried to revert some codes in ColorConverter.cpp in stagefright, it can > make sure the problem we meet. The patch worked on my master nexus-5-l. But it might be better to analyze why the problem happens. GraphicBuffer::lockYCbCr() is also supported for HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS on android v5.1 http://androidxref.com/5.1.0_r1/xref/hardware/qcom/display/msm8974/libgralloc/mapper.cpp#247

(In reply to Vincent Liu[:vliu] from comment #16) > Hi Sotaro, > > From the Bug 1120780 Comment 59, do we expect the color converter will fall > back on lock_ycbcr for the YCbCr_420_SP_VENUS case? Thanks for the comment. Color convert is done like the following sequence. If android::ColorConverter does not support YCbCr_420_SP_VENUS, ConvertVendorYUVFormatToRGB565() do it. GrallocImage::GetAsSourceSurface() ->ConvertOmxYUVFormatToRGB565() ->// Try color convert by using android::ColorConverter ->android::ColorConverter::convert() ->//If ConvertOmxYUVFormatToRGB565() failed to do color convert ->ConvertVendorYUVFormatToRGB565() ->GraphicBuffer::lockYCbCr()

moz-build nexus-5-l uses following for GraphicBuffer::lockYCbCr(). https://github.com/mozilla-b2g/hardware_qcom_display/pull/1/files It works actually same to android 5.1's the following code. It looks like different though. http://androidxref.com/5.1.0_r1/xref/hardware/qcom/display/msm8974/libgralloc/mapper.cpp#269

(In reply to Sotaro Ikeda [:sotaro] from comment #19) > It works actually same to android 5.1's the following code. It looks like > different though. > http://androidxref.com/5.1.0_r1/xref/hardware/qcom/display/msm8974/ > libgralloc/mapper.cpp#269 Android 5.1's code also caused same problem on nexus-5-l.

The video size is 560*360. From the color format stride is 640. From the investigation, when GraphicBuffer of the video is mapped by GraphicBuffer::lockYCbCr(), ystride and cstride are incorrect values and video data is strangely placed. ystride seemed like (640-4), cstride seemed like (640-8).

560 is multiple of 4, but not multiple of 8. It might be related to the problem.

Change moz-build source to same to android v5.1 and apply a stride quirk. I used the patch to check the stride problem.

In attachment 8594209 [details] [diff] [review], actually only the following is related to checking the stride problem. By applying the patch, I confirmed the thumbnail's skew was disappeared, but the video was strangely segmented. ---------------------------------------------------------- + // XXX test code to check stride problem. + if (ystride == 640) { + ycbcr->ystride = 640 - 4; + ycbcr->cstride = 640 - 8; + }

From comment 21 and comment 25, the problem seems gonk issues.

By the way, android normally use sw codec for thumbnail generation. http://androidxref.com/5.0.0_r2/xref/frameworks/av/media/libstagefright/StagefrightMetadataRetriever.cpp#387

(In reply to Sotaro Ikeda [:sotaro] from comment #27) > By the way, android normally use sw codec for thumbnail generation. > http://androidxref.com/5.0.0_r2/xref/frameworks/av/media/libstagefright/ > StagefrightMetadataRetriever.cpp#387 IIRC FxOS used to do this too

At first it allow sw video decoder. But we disable sw video codec since b2g v1.0.0 by Bug 834150.

(In reply to Sotaro Ikeda [:sotaro] from comment #29) > At first it allow sw video decoder. But we disable sw video codec since b2g > v1.0.0 by Bug 834150. Even when gecko request hw video codec, some chip vendors implement it by sw decoder though.

(In reply to Sotaro Ikeda [:sotaro] from comment #26) > From comment 21 and comment 25, the problem seems gonk issues. So, it seems like POVB problem.

diego -- can you please confirm?

This issue also cause thumbnail image broken for camera recorded file. From current founding, N5 doesn't this issue on master branch. It happens on v2.2 branch. I'd try some steps to make sure Gecko/Gaia on v2.2 doesn't cause this issue happens. 1. Flash the full image for master branch. Thumbnail image for camera recorded file looks well. 2. Change Gecko/Gaia on v2.2 branch. Thumbnail image for camera recorded file looks well. 3. Flash the full image for v2.2 branch. Thumbnail image for camera recorded file is broken. The result may lead to POVB problem. (In reply to Inder from comment #32) > diego -- can you please confirm? You can take this bug if the confirmation is positive.

Sotaro, Where are you applying attachment 8594209 [details] [diff] [review] ? I can't find any reference to HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS in hardware/qcom/display/libgralloc/mapper.cpp in my workspace. Is this a project you are forking? If so, where is it hosted?

moz build nexus-5-l uses the following. https://github.com/mozilla-b2g/hardware_qcom_display/blob/b2g-5.1.0_r1/msm8974/libgralloc/mapper.cpp#L269 It is referenced by the manifest. https://github.com/mozilla-b2g/b2g-manifest/blob/v2.2/nexus-5-l.xml#L174

(In reply to Sotaro Ikeda [:sotaro] from comment #35) > moz build nexus-5-l uses the following. > https://github.com/mozilla-b2g/hardware_qcom_display/blob/b2g-5.1.0_r1/ > msm8974/libgralloc/mapper.cpp#L269 > > It is referenced by the manifest. > https://github.com/mozilla-b2g/b2g-manifest/blob/v2.2/nexus-5-l.xml#L174 It appears mwu added the HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS support there: https://github.com/mozilla-b2g/hardware_qcom_display/pull/1

(In reply to Diego Wilson [:diego] from comment #36) > (In reply to Sotaro Ikeda [:sotaro] from comment #35) > > moz build nexus-5-l uses the following. > > https://github.com/mozilla-b2g/hardware_qcom_display/blob/b2g-5.1.0_r1/ > > msm8974/libgralloc/mapper.cpp#L269 > > > > It is referenced by the manifest. > > https://github.com/mozilla-b2g/b2g-manifest/blob/v2.2/nexus-5-l.xml#L174 > > It appears mwu added the HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS support there: > > https://github.com/mozilla-b2g/hardware_qcom_display/pull/1 As in comment 23, comment 24 and comment 25. When the HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS support code is changed to same to android 5.1 aosp, the problem still happens.

(In reply to Sotaro Ikeda [:sotaro] from comment #37) > As in comment 23, comment 24 and comment 25. When the > HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS support code is changed to same to > android 5.1 aosp, the problem still happens. I see. From where exactly in AOSP 5.1 are you pulling the Venus code?

(In reply to Diego Wilson [:diego] from comment #38) > > I see. From where exactly in AOSP 5.1 are you pulling the Venus code? The following code. http://androidxref.com/5.1.0_r1/xref/hardware/qcom/display/msm8974/libgralloc/mapper.cpp#269

(In reply to Sotaro Ikeda [:sotaro] from comment #39) > (In reply to Diego Wilson [:diego] from comment #38) > > > > I see. From where exactly in AOSP 5.1 are you pulling the Venus code? > > The following code. > > http://androidxref.com/5.1.0_r1/xref/hardware/qcom/display/msm8974/ > libgralloc/mapper.cpp#269 I'm seeing a similar issue on a different color format. Can you check if the GraphicBuffer stride matches the ycbcr stride? Just add a query to aBuffer->getStride() here https://mxr.mozilla.org/mozilla-b2g37_v2_2/source/gfx/layers/GrallocImages.cpp#241

(In reply to Diego Wilson [:diego] from comment #40) > (In reply to Sotaro Ikeda [:sotaro] from comment #39) > > (In reply to Diego Wilson [:diego] from comment #38) > > > > > > I see. From where exactly in AOSP 5.1 are you pulling the Venus code? > > > > The following code. > > > > http://androidxref.com/5.1.0_r1/xref/hardware/qcom/display/msm8974/ > > libgralloc/mapper.cpp#269 > > I'm seeing a similar issue on a different color format. Can you check if the > GraphicBuffer stride matches the ycbcr stride? Diego, can you provide actual STR to reproduce it? In which color format?

Some videos have bad thumbnails due to bad stride values on HAL_PIXEL_FORMAT_YCrCb_420_SP frames. This is happening on my prototype L device. I'm not sure if exactly the same issue as what's happening in Nexus-L, but it's probably related. I have a patch for that which I'll share as soon as I can.

This patch fixes a similar issue I was having

Comment on attachment 8599531 [details] [diff] [review] Query stride when converting HAL_PIXEL_FORMAT_YCrCb_420_SP Review of attachment 8599531 [details] [diff] [review]: ----------------------------------------------------------------- This patch look good! But I thinks it is not related to this bug.

This bug's code path uses ConvertYVU420SPToRGB565() to convert color format. And stride values are got via android::GraphicBuffer::lockYCbCr(). It is actually implemented by gralloc_lock_ycbcr() in gralloc hal. And the problem happens even when gralloc_lock_ycbcr() is replaced to android 5.1 aosp code.

This bug no longer blocks CAF v2.2. I created bug 110689 for the patch that fixes the CAF v2.2 issue.