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

RESOLVED WONTFIX

Status

()

Core
Audio/Video: Playback
RESOLVED WONTFIX
3 years ago
2 years ago

People

(Reporter: Hermes Cheng (inactive after July 27, 2015), Assigned: vliu)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [v2.2-nexus-5-l])

Attachments

(4 attachments, 1 obsolete attachment)

Created attachment 8583635 [details]
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
I found this issue when verifying bug 1120780.
Michael, could you take a look?
Blocks: 1120780
blocking-b2g: --- → 2.2?
Flags: needinfo?(mwu)
Whiteboard: [v2.2-nexus-5-l]

Comment 2

3 years ago
mwu: any update/input here?

thanks
hema

Comment 3

3 years ago
Inder: 

Is this also happening on QRD?
Flags: needinfo?(ikumar)
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

3 years ago
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.
Flags: needinfo?(ikumar)
Sotaro, do you have any insight about this bug?  Looks like this is a generic L issue, rather than being specific to nexus 5.
Flags: needinfo?(sotaro.ikeda.g)

Comment 7

3 years ago
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
blocking-b2g: 2.2? → 2.2+

Comment 8

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

Hermes, can you link your test video?
Flags: needinfo?(mwu)
Flags: needinfo?(hcheng)
(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
Flags: needinfo?(hcheng)

Updated

3 years ago
Blocks: 1063044
Whiteboard: [v2.2-nexus-5-l] → [v2.2-nexus-5-l][CR 812421]

Updated

3 years ago
Component: Gaia::Video → Video/Audio
Product: Firefox OS → Core

Updated

3 years ago
Whiteboard: [v2.2-nexus-5-l][CR 812421] → [caf priority: p2][v2.2-nexus-5-l][CR 812421]

Comment 10

3 years ago
steven, could you help on this?
Flags: needinfo?(slee)

Comment 11

3 years ago
Hi Blake,

Please check this bug, thanks.
Flags: needinfo?(slee) → needinfo?(bwu)

Updated

3 years ago
Keywords: regression
I think this should be related to color space alignment. 
vliu, 
Can you help check it?
Flags: needinfo?(bwu) → needinfo?(vliu)
(Assignee)

Comment 13

3 years ago
Sure, I can reproduce this issue on Nexus 5. I will try to look into this problem.
(Assignee)

Comment 14

3 years ago
It seems that the recent change for ColorConverter in stagefright to cause this problem.
Flags: needinfo?(vliu)
(Assignee)

Comment 15

3 years ago
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.

Updated

3 years ago
Assignee: nobody → vliu
Status: NEW → ASSIGNED
(Assignee)

Comment 16

3 years ago
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
Flags: needinfo?(sotaro.ikeda.g)
(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.
Created attachment 8594209 [details] [diff] [review]
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.
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.
(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.

Comment 32

3 years ago
diego -- can you please confirm?
Flags: needinfo?(dwilson)
(Assignee)

Comment 33

3 years ago
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.
Flags: needinfo?(ikumar)
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?
Flags: needinfo?(dwilson) → needinfo?(sotaro.ikeda.g)
(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?
Flags: needinfo?(ikumar) → needinfo?(sotaro.ikeda.g)
(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
Flags: needinfo?(sotaro.ikeda.g)
(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
Flags: needinfo?(sotaro.ikeda.g)
(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?
Flags: needinfo?(sotaro.ikeda.g) → needinfo?(dwilson)
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.
Flags: needinfo?(dwilson)
Created attachment 8599531 [details] [diff] [review]
Query stride when converting HAL_PIXEL_FORMAT_YCrCb_420_SP

This patch fixes a similar issue I was having
Attachment #8599531 - Flags: feedback?(sotaro.ikeda.g)
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.
Attachment #8599531 - Flags: feedback?(sotaro.ikeda.g) → feedback+
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.

Updated

3 years ago
Blocks: 1160689
This bug no longer blocks CAF v2.2. I created bug 110689 for the patch that fixes the CAF v2.2 issue.

Updated

3 years ago
No longer blocks: 1063044
blocking-b2g: 2.2+ → ---

Updated

3 years ago
Whiteboard: [caf priority: p2][v2.2-nexus-5-l][CR 812421] → [v2.2-nexus-5-l]

Updated

3 years ago
Attachment #8599531 - Attachment is obsolete: true

Updated

3 years ago
No longer blocks: 1160689

Updated

3 years ago
See Also: → bug 1179162
Component: Audio/Video → Audio/Video: Playback
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.