1147763 - [Nexus 5][Video] thumbnail image of webm format is broken

Status: RESOLVED WONTFIX

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

Description

[Hermes Cheng (inactive after July 27, 2015)]

Attachment: Screen comparison between flame and Nexus 5.jpg

* 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

Comment 1

[Hermes Cheng (inactive after July 27, 2015)]

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

Comment 2

[Hema Koka [:hema]]

mwu: any update/input here?

thanks
hema

Comment 3

[Hema Koka [:hema]]

Inder: 

Is this also happening on QRD?

Comment 4

[bhavana bajaj [:bajaj]]

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.

Comment 5

[Inder]

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.

Comment 6

[No-Jun Park [:njpark]]

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

Comment 7

[Hema Koka [:hema]]

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

Comment 8

[Michael Wu [:mwu]]

Probably another stride issue, though probably on the gecko side of things.

Hermes, can you link your test video?

Comment 9

[Hermes Cheng (inactive after July 27, 2015)]

(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

Comment 10

[Bobby Chien]

steven, could you help on this?

Comment 11

[StevenLee[:slee]]

Hi Blake,

Please check this bug, thanks.

Comment 12

[Blake Wu [:bwu][:blakewu]]

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

Comment 13

[Vincent Liu[:vliu]]

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

Comment 14

[Vincent Liu[:vliu]]

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

Comment 15

[Vincent Liu[:vliu]]

Attachment: WIP-revert-color-converter.diff

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

Comment 16

[Vincent Liu[:vliu]]

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.

Comment 17

[Sotaro Ikeda [:sotaro]]

(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

Comment 18

[Sotaro Ikeda [:sotaro]]

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

Comment 19

[Sotaro Ikeda [:sotaro]]

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

Comment 20

[Sotaro Ikeda [:sotaro]]

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

Comment 21

[Sotaro Ikeda [:sotaro]]

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

Comment 22

[Sotaro Ikeda [:sotaro]]

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

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: temporary patch - Change to hardware/qcom/display

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

Comment 24

[Sotaro Ikeda [:sotaro]]

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;
+                }

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: thumbnail by applying attachment 8594209

Comment 26

[Sotaro Ikeda [:sotaro]]

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

Comment 27

[Sotaro Ikeda [:sotaro]]

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

Comment 28

[Diego Wilson [:diego]]

(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

Comment 29

[Sotaro Ikeda [:sotaro]]

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

Comment 30

[Sotaro Ikeda [:sotaro]]

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

Comment 31

[Sotaro Ikeda [:sotaro]]

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

Comment 32

[Inder]

diego -- can you please confirm?

Comment 33

[Vincent Liu[:vliu]]

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.

Comment 34

[Diego Wilson [:diego]]

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?

Comment 35

[Sotaro Ikeda [:sotaro]]

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

Comment 36

[Diego Wilson [:diego]]

(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

Comment 37

[Sotaro Ikeda [:sotaro]]

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

Comment 38

[Diego Wilson [:diego]]

(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?

Comment 39

[Sotaro Ikeda [:sotaro]]

(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

Comment 40

[Diego Wilson [:diego]]

(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

Comment 41

[Sotaro Ikeda [:sotaro]]

(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?

Comment 42

[Diego Wilson [:diego]]

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.

Comment 43

[Diego Wilson [:diego]]

Attachment: Query stride when converting HAL_PIXEL_FORMAT_YCrCb_420_SP

This patch fixes a similar issue I was having

Comment 44

[Sotaro Ikeda [:sotaro]]

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.

Comment 45

[Sotaro Ikeda [:sotaro]]

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.

Comment 46

[Diego Wilson [:diego]]

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