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

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vliu, Assigned: vliu)

Tracking

unspecified
mozilla35
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0M+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.0 wontfix, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch bug-1060811-fix.patch (obsolete) — Splinter Review
Hi Sotaro,

Can you please help me to review the patch? Thanks.
Attachment #8481806 - Flags: review?(sotaro.ikeda.g)
Assignee: nobody → vliu
Status: NEW → ASSIGNED
Blocks: 1050192
Blocks: 1040648
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
Attachment #8481806 - Flags: review?(sotaro.ikeda.g) → review+
(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.
Update the patch based on the review suggestion and the result of try server.
Attachment #8481806 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dc77ebda2445
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
[Blocking Requested - why for this release]:

Partner device need this patch on v2.0 branch.
blocking-b2g: --- → 2.0?
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.
(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?
blocking-b2g: 2.0? → 2.0M?
Duplicate of this bug: 1061239
this is necessary for Woodduck. 2.0m+
blocking-b2g: 2.0M? → 2.0M+
Blocks: 1043558
Does this need to land on v2.1? If so, please nominate it for Aurora approval :)
status-b2g-v2.1: --- → ?
Flags: needinfo?(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
Attachment #8482460 - Flags: approval-mozilla-aurora?
Flags: needinfo?(vliu)
Comment on attachment 8482460 [details] [diff] [review]
bug-1060811-fix-v2.patch

low risk patch to avoid a crash, approving on aurora
Attachment #8482460 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1075077
You need to log in before you can comment on or make changes to this bug.