Vincent Liu[:vliu] Assignee
	Description • 10 years ago

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.

	Vincent Liu[:vliu] Assignee
	Comment 1 • 10 years ago

Hi Sotaro,

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

	Sotaro Ikeda [:sotaro]
	Comment 2 • 10 years ago

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

	Vincent Liu[:vliu] Assignee
	Comment 3 • 10 years ago

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

	Vincent Liu[:vliu] Assignee
	Comment 4 • 10 years ago

Try server result:

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

	Vincent Liu[:vliu] Assignee
	Comment 5 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 6 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 10 years ago

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

	Vincent Liu[:vliu] Assignee
	Comment 8 • 10 years ago

[Blocking Requested - why for this release]:

Partner device need this patch on v2.0 branch.

	bhavana bajaj [:bajaj]
	Comment 9 • 10 years ago

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.

	Vincent Liu[:vliu] Assignee
	Comment 10 • 10 years ago

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

	Francis Lee [:frlee]
	Comment 12 • 10 years ago

this is necessary for Woodduck. 2.0m+

	Kai-Zhen Li [:seinlin][:kli]
	Comment 13 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 10 years ago

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

	Vincent Liu[:vliu] Assignee
	Comment 15 • 10 years ago

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

	bhavana bajaj [:bajaj]
	Comment 16 • 10 years ago

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

low risk patch to avoid a crash, approving on aurora

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 10 years ago

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