1060811 - PlanarYCbCrData contains null for color conversion if the output format of HW decoder is YV12.

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Vincent Liu[:vliu]]

When B2G runs gaia app, long pressing home key triggers system app to execute screenshot for showing cardview. For camera app, this screenshot also contains doing color conversion to convert YUV to RGB in GrallocImages. If YUV is HW decoded and the format is YV12, YUV data already stored in GraphicBuffer instead of passing it by SetData(). In this case, ConvertOmxYUVFormatToRGB565 gets null YUV data to convert. To fix this problem, we need to validate data before converting it.

Comment 1

[Vincent Liu[:vliu]]

Attachment: bug-1060811-fix.patch

Hi Sotaro,

Can you please help me to review the patch? Thanks.

Comment 2

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8481806 [details] [diff] [review]
bug-1060811-fix.patch

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

Looks good:-)

::: gfx/layers/GrallocImages.cpp
@@ +323,5 @@
> +    // mData if it doesn't contain valid configuration.
> +    layers::PlanarYCbCrData ycbcrData = aYcbcrData;
> +    if (!ycbcrData.mYChannel) {
> +      ycbcrData.mYChannel     = buffer;
> +      ycbcrData.mYStride      = aBuffer->getStride();

It might be better to initialize also mYSkip, mCrSkip and mCbSkip to make clear the code's intent like the following.
http://mxr.mozilla.org/mozilla-central/source/content/media/omx/OMXCodecWrapper.cpp#338

Comment 3

[Vincent Liu[:vliu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> Comment on attachment 8481806 [details] [diff] [review]
> bug-1060811-fix.patch
> 
> Review of attachment 8481806 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good:-)
> 
> ::: gfx/layers/GrallocImages.cpp
> @@ +323,5 @@
> > +    // mData if it doesn't contain valid configuration.
> > +    layers::PlanarYCbCrData ycbcrData = aYcbcrData;
> > +    if (!ycbcrData.mYChannel) {
> > +      ycbcrData.mYChannel     = buffer;
> > +      ycbcrData.mYStride      = aBuffer->getStride();
> 
> It might be better to initialize also mYSkip, mCrSkip and mCbSkip to make
> clear the code's intent like the following.
> http://mxr.mozilla.org/mozilla-central/source/content/media/omx/
> OMXCodecWrapper.cpp#338

Thanks for your review and suggestion. I will follow it and then run the try server.

Comment 4

[Vincent Liu[:vliu]]

Try server result:

https://tbpl.mozilla.org/?tree=Try&rev=d23af36700d7
https://tbpl.mozilla.org/?tree=Try&rev=c09c38a64230

Comment 5

[Vincent Liu[:vliu]]

Attachment: bug-1060811-fix-v2.patch

Update the patch based on the review suggestion and the result of try server.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc77ebda2445

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc77ebda2445

Comment 8

[Vincent Liu[:vliu]]

[Blocking Requested - why for this release]:

Partner device need this patch on v2.0 branch.

Comment 9

[bhavana bajaj [:bajaj]]

Discussing offline with vincent is this is 2.0M specific or if this needs to land on 2.0 before making a blocking call here. Given where we are in 2.0 this branch is only for ship-blockers at this time and the risk/reward here is unclear at this time to make a blocking call.

Comment 10

[Vincent Liu[:vliu]]

(In reply to bhavana bajaj [:bajaj] from comment #9)
> Discussing offline with vincent is this is 2.0M specific or if this needs to
> land on 2.0 before making a blocking call here. Given where we are in 2.0
> this branch is only for ship-blockers at this time and the risk/reward here
> is unclear at this time to make a blocking call.

The reason why I want to mark 2.0? is I think this is a common issue need to be fixed. Flame uses another color format and that's why it can't be reproduced. But considering the risk at this moment, plus no foreseeable device will use 2.0, I think it is a better way to mark it to 2.0M?

Comment 12

[Francis Lee [:frlee]]

this is necessary for Woodduck. 2.0m+

Comment 13

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/3a99d6855393

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Does this need to land on v2.1? If so, please nominate it for Aurora approval :)

Comment 15

[Vincent Liu[:vliu]]

Comment on attachment 8482460 [details] [diff] [review]
bug-1060811-fix-v2.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1060811
[User impact if declined]: Without this patch, any device which uses YV12 may cause crash.
[Describe test coverage new/current, TBPL]: try server, flame and woodduck
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 16

[bhavana bajaj [:bajaj]]

Comment on attachment 8482460 [details] [diff] [review]
bug-1060811-fix-v2.patch

low risk patch to avoid a crash, approving on aurora

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a5196df8db6c