CR number: ALPS01727777 [Initial condition] [Step] 1.Play videos in Video App 2 Back to thumbnail screen [Error] Displayed green line on the top of video thumbnail

When I tried to grab the yuv data from GraphicBuffer and converted it on PC, I could still see the green line. Could partner also check yuv data from the output of decoder in driver layer to clarify the issue? Thanks

I will clean the ni? since partner gave me the clear info from mail. I will study the possible patch to fix this problem.

Hi Peter, Would you please have a feedback for the proposed WIP? According to discussion in the morning, I decide to propose this patch dedicated land to 2.0m. Thanks

Sorry about attached a WIP contained other patch in it. I'd obsoleted the previous one and attached again.

Comment on attachment 8495052 [details] [diff] [review] WIP-1.patch Review of attachment 8495052 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/GrallocImages.cpp @@ +322,3 @@ > // yuv data has already stored in GraphicBuffer. Here we try to confgiure the > // mData if it doesn't contain valid configuration. > layers::PlanarYCbCrData ycbcrData = aYcbcrData; Please add comment about your modification. @@ +322,5 @@ > // yuv data has already stored in GraphicBuffer. Here we try to confgiure the > // mData if it doesn't contain valid configuration. > layers::PlanarYCbCrData ycbcrData = aYcbcrData; > + gfx::IntSize alignedSize; > + uint8_t numByteOfPixel = gfx::BytesPerPixel(aSurface->GetFormat()); I think you can just hard code 32, since BytesPerPixel always return 4 for unknown format. http://dxr.mozilla.org/mozilla-central/source/gfx/2d/Tools.h#83 @@ +337,4 @@ > ycbcrData.mCrChannel = buffer + ycbcrData.mYStride * ycbcrData.mYSize.height; > ycbcrData.mCrSkip = 0; > + // Align to number of bytes boundary > + ycbcrData.mCbCrStride = ALIGN(ycbcrData.mYStride / 2, (numByteOfPixel << 3)); do we need to have this change to solve this problem?

(In reply to peter chang[:pchang][:peter] from comment #6) > Comment on attachment 8495052 [details] [diff] [review] > WIP-1.patch > > Review of attachment 8495052 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/GrallocImages.cpp > @@ +322,3 @@ > > // yuv data has already stored in GraphicBuffer. Here we try to confgiure the > > // mData if it doesn't contain valid configuration. > > layers::PlanarYCbCrData ycbcrData = aYcbcrData; > > Please add comment about your modification. > Comments was added. Could you please have feedback to see if the comment is clear? Thanks > @@ +322,5 @@ > > // yuv data has already stored in GraphicBuffer. Here we try to confgiure the > > // mData if it doesn't contain valid configuration. > > layers::PlanarYCbCrData ycbcrData = aYcbcrData; > > + gfx::IntSize alignedSize; > > + uint8_t numByteOfPixel = gfx::BytesPerPixel(aSurface->GetFormat()); > > I think you can just hard code 32, since BytesPerPixel always return 4 for > unknown format. > To avoid affecting the platform which no need to consider align for its height, this patch would be the solution dedicated for 2.0m. Based on this, I think hard coding would be fine. > http://dxr.mozilla.org/mozilla-central/source/gfx/2d/Tools.h#83 > > @@ +337,4 @@ > > ycbcrData.mCrChannel = buffer + ycbcrData.mYStride * ycbcrData.mYSize.height; > > ycbcrData.mCrSkip = 0; > > + // Align to number of bytes boundary > > + ycbcrData.mCbCrStride = ALIGN(ycbcrData.mYStride / 2, (numByteOfPixel << 3)); > > do we need to have this change to solve this problem? I'd verified that ALIGN(x, 16) = ((x) + 15) & ~0x0F. I think that ALIGN() would be a better code implementation for easily reading.

Comment on attachment 8495172 [details] [diff] [review] WIP-2.patch Review of attachment 8495172 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, I assume this patch will only be landed on woodduck branch.

(In reply to peter chang[:pchang][:peter] from comment #8) > Comment on attachment 8495172 [details] [diff] [review] > WIP-2.patch > > Review of attachment 8495172 [details] [diff] [review]: > ----------------------------------------------------------------- > > LGTM, I assume this patch will only be landed on woodduck branch. Thanks for your feedback.

Hi Sotaro, This issue happens because the video decoder aligns the image for both width and hight on woodduck. Our current code implementation only consider align for width, and I believe this is a general case in other platforms. Since the specified case for woodduck, the proposed patch will only land into 2.0m to avoid not affecting other platforms. Furthermore, Bug 880114 will be the feature landed in 2.2 release, so this kind of patch would be the short term solution. I have one more question. When I saw the align value, it would be 16 or 32. In the code like below http://dxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages.cpp#295 The align value hard coding to 32. The same thing also happens for the proposed patch. An idea came up when I saw your patch in Bug 1065492. If I use BytePerPixel to determine the align value, is it the right way to do? uint32_t numByteOfPixel = gfx::BytesPerPixel(aSurface->GetFormat()); ALIGN(aSurface->GetSize().height, numByteOfPixel*8); It seems there has relationship between BytePerPixel and ALIGN value, but I am not sure about this. Could you please have a review and give comment about my question above? Thanks

vliu, do you know android added an additional constraint to YV12 color format? It is written in the following. http://androidxref.com/4.4.4_r1/xref/system/core/include/system/graphics.h#91

(In reply to Vincent Liu[:vliu] from comment #10) > Created attachment 8495739 [details] [diff] [review] > bug-1068502-fix.patch > > Hi Sotaro, > > This issue happens because the video decoder aligns the image for both width > and hight on woodduck. Our current code implementation only consider align > for width, and I believe this is a general case in other platforms. Since > the specified case for woodduck, the proposed patch will only land into 2.0m > to avoid not affecting other platforms. Furthermore, Bug 880114 will be the > feature landed in 2.2 release, so this kind of patch would be the short term > solution. > > I have one more question. When I saw the align value, it would be 16 or 32. > In the code like below > > http://dxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages. > cpp#295 > > The align value hard coding to 32. The same thing also happens for the > proposed patch. An idea came up when I saw your patch in Bug 1065492. If I > use BytePerPixel to determine the align value, is it the right way to do? HAL_PIXEL_FORMAT_YCrCb_420_SP_ADRENO is a qcom proprietary color format. There is not formal specification about it in public. Its implementation is done just as a reverse hacking. Therefore I do not care about its code so much. But HAL_PIXEL_FORMAT_YV12 seems to be explained in the source code like Comment 11. Based on that information, the code could be more clear about what it is doing. I want to avoid just to put a result value like 32 if it is possible. You need to be careful about how to calculate the alignments. Alignment of source and destination might be different. Source alignment seems to be calculated from Comment 11.

(In reply to Sotaro Ikeda [:sotaro] from comment #11) > vliu, do you know android added an additional constraint to YV12 color > format? It is written in the following. > > http://androidxref.com/4.4.4_r1/xref/system/core/include/system/graphics.h#91 YV12 is defined in android, therefore this color conversion problem seems not hardware specific problem. If it is hardware specific problem, we have to ask to hardware vendor.

In Bug 1043558, I worte similar but a bit different color conversion code as a temporary patch attachment 8492488 [details] [diff] [review]. It might help.

> I want to avoid just to put a result value like 32 if it is possible. You > need to be careful about how to calculate the alignments. Alignment of > source and destination might be different. Source alignment seems to be > calculated from Comment 11. Sorry, destination's alignment is not related to your patch. Forget about that part.

(In reply to Sotaro Ikeda [:sotaro] from comment #11) > vliu, do you know android added an additional constraint to YV12 color > format? It is written in the following. > > http://androidxref.com/4.4.4_r1/xref/system/core/include/system/graphics.h#91 Yes, I know about this. But from the information you offered, I see some comments they have more clear to define YV12. * This format assumes * - an even width * - an even height * - a horizontal stride multiple of 16 pixels * - a vertical stride equal to the height For a vertical stride, it is clear to be defined by the sentence |a vertical stride equal to the height|. Based on the link, does partner can follow it to fix the problem? Thanks.

It seems I clear the ni? to @jocheng set by partner. I did it by accident ,and here I set it for partner again.

Hi Vincent, Since the 16-byte alignment of height is our limitation of HW decoder, it needs a lot of efforts to crop the video buffer within the OMXCodec module. Could you use that patch only for woodduck?

vliu, if woodduck's decoder has an additional restriction of decoding height that is multiple of 16, don't we need to modify only the following? Can you explain why you modify different parts? > ycbcrData.mCrChannel > ycbcrData.mCbChannel

Sotaro, I'd tried not considering the align in CbCr channels ,and it turns out that the thumbnail still got splash image in it. I guess SliceHeight should also be used in CbCr from the output of video decoder.

(In reply to Vincent Liu[:vliu] from comment #20) > Sotaro, I'd tried not considering the align in CbCr channels ,and it turns > out that the thumbnail still got splash image in it. I guess SliceHeight > should also be used in CbCr from the output of video decoder. vliu, if you tried the code, can you upload the patch? Without it, I do not know what you did. And can you explain concretely about your assumption by referencing the actual gecko code?

From the vendor information, we know that woodduck's codec adds the slice height restriction. Therefore we can make clear which kind of change is necessary. From your information and the patch, it is not clear yet.

vliu, the reason that I put off a review is that from your patch and comment, it is not clear enough why this change is necessary. The codec height limitation and your patch is not connected by clear enough logic.

(In reply to Vincent Liu[:vliu] from comment #20) >I guess SliceHeight > should also be used in CbCr from the output of video decoder. vliu, can you make clear the above guess?

(In reply to Sotaro Ikeda [:sotaro] from comment #19) > vliu, if woodduck's decoder has an additional restriction of decoding height > that is multiple of 16, don't we need to modify only the following? Can you > explain why you modify different parts? > > ycbcrData.mCrChannel > > ycbcrData.mCbChannel I imagined attachment 8497532 [details] [diff] [review] in Comment 19. I have no way to confirm the patch. It could be wrong.

Hi Sotaro, According to the definitions in the following. http://androidxref.com/4.4.4_r1/xref/system/core/include/system/graphics.h#91 107 * y_size = stride * height 108 * c_stride = ALIGN(stride/2, 16) 109 * c_size = c_stride * height/2 110 * size = y_size + c_size * 2 111 * cr_offset = y_size 112 * cb_offset = y_size + c_size For y_size, since it uses stride for width, I think we should also consider alignedSize.height for height. The same case should also apply for c_size. So y_size = stride * alignedSize.height c_size = c_stride * alignedSize.height/2 bug-1068502-fix.patch is trying to consider the align for mYSize, mCbCrSize, mCrChannel and mCbChannel based on the above idea. It can also fix the issue. (In reply to Sotaro Ikeda [:sotaro] from comment #19) > vliu, if woodduck's decoder has an additional restriction of decoding height > that is multiple of 16, don't we need to modify only the following? Can you > explain why you modify different parts? > > ycbcrData.mCrChannel > > ycbcrData.mCbChannel Sorry for my mislead your meaning. After seeing your temporary patch, I could know your idea. From your Comment, I tried not to conside align for mCrChannel and mCbChannel. Please see the attached temp_patch_1 to know the detail. Obviouly temp_patch_1 still got the splash in thumbnail. (In reply to Sotaro Ikeda [:sotaro] from comment #26) > > I imagined attachment 8497532 [details] [diff] [review] in Comment 19. I > have no way to confirm the patch. It could be wrong. I'd tried your patch ,and it can also fix this problem. From your patch, it seems the code only consider the offset for mCrChannel and mCbChannel. But if I look into the definition in graphics.h, I can not figure out why we shouldn't consider align for mYSize and mCbCrSize?

(In reply to Vincent Liu[:vliu] from comment #27) > Created attachment 8498030 [details] [diff] [review] > temp_patch_1 > > Hi Sotaro, > According to the definitions in the following. > > http://androidxref.com/4.4.4_r1/xref/system/core/include/system/graphics.h#91 > > 107 * y_size = stride * height > 108 * c_stride = ALIGN(stride/2, 16) > 109 * c_size = c_stride * height/2 > 110 * size = y_size + c_size * 2 > 111 * cr_offset = y_size > 112 * cb_offset = y_size + c_size > > For y_size, since it uses stride for width, I think we should also consider > alignedSize.height for height. The same case should also apply for c_size. > So > > y_size = stride * alignedSize.height > c_size = c_stride * alignedSize.height/2 > > bug-1068502-fix.patch is trying to consider the align for mYSize, mCbCrSize, > mCrChannel and mCbChannel based on the above idea. It can also fix the issue. vliu, can you explain the above based on implementations of gfx::ConvertYCbCrToRGB() and gfx::ConvertYCbCrToRGB565()? The above explanation still does not explain what is actually done during the color convert. http://dxr.mozilla.org/mozilla-central/source/gfx/ycbcr/YCbCrUtils.cpp#70 http://dxr.mozilla.org/mozilla-central/source/gfx/ycbcr/ycbcr_to_rgb565.cpp#600

> bug-1068502-fix.patch is trying to consider the align for mYSize, mCbCrSize, > mCrChannel and mCbChannel based on the above idea. It can also fix the issue. > > (In reply to Sotaro Ikeda [:sotaro] from comment #19) > > vliu, if woodduck's decoder has an additional restriction of decoding height > > that is multiple of 16, don't we need to modify only the following? Can you > > explain why you modify different parts? > > > ycbcrData.mCrChannel > > > ycbcrData.mCbChannel > > Sorry for my mislead your meaning. After seeing your temporary patch, I > could know your idea. From your Comment, I tried not to conside align for > mCrChannel and mCbChannel. Please see the attached temp_patch_1 to know the > detail. Obviouly temp_patch_1 still got the splash in thumbnail. As I explained in Comment 19, I said only following change is necessary. - ycbcrData.mCrChannel - ycbcrData.mCbChannel I can not understand what you want to do in attachment 8498030 [details] [diff] [review]. The patch seems to doing inconsistent things.

> (In reply to Sotaro Ikeda [:sotaro] from comment #26) > > > > I imagined attachment 8497532 [details] [diff] [review] in Comment 19. I > > have no way to confirm the patch. It could be wrong. > > I'd tried your patch ,and it can also fix this problem. From your patch, it > seems the code only consider the offset for mCrChannel and mCbChannel. But > if I look into the definition in graphics.h, I can not figure out why we > shouldn't consider align for mYSize and mCbCrSize? As I explained in Comment 19, I said only following change is necessary. From my point of view, it should be correct fix. - ycbcrData.mCrChannel - ycbcrData.mCbChannel Did you read the code of gfx::ConvertYCbCrToRGB() and gfx::ConvertYCbCrToRGB565()? They explain the reason. We do not need to extend picture size. To decode color correctly from YV12 to RGB565, we actually need only following information. - ycbcrData.mYChannel - ycbcrData.mYStride - ycbcrData.mCrChannel - ycbcrData.mCbChannel - ycbcrData.mCbCrStride Picture size is used only to limit the data access area. By attachment 8497532 [details] [diff] [review], I changed ycbcrData.mCrChannel and ycbcrData.mCbChannel to affect the decoded size height limitation. We do not need to modify mYSize and mCbCrSize, because they are used only to limit the data access area and we need only valid data area.

attachment 8495739 [details] [diff] [review] could do incorrect data access to destination. The following change extends the destination size to aligned size. But destination buffer is not allocated by that size. It might try to access unmapped area by buffer overrun. ---------------------------------------------------- gfx::ConvertYCbCrToRGB(ycbcrData, aSurface->GetFormat(), - aSurface->GetSize(), + alignedSize, aMappedSurface->mData, aMappedSurface->mStride);

If attachment 8497532 [details] [diff] [review] fixed the problem, attachment 8498156 [details] [diff] [review] seems also fixed the problem. And it seems better fix.

Changed to correct component.

This bug is set to confidential bug. Until when this bug reach to committing state, actual patch should be moved to a public bug that explains what kind of problem is the bug going to fix.

(In reply to Sotaro Ikeda [:sotaro] from comment #35) > This bug is set to confidential bug. Until when this bug reach to committing > state, actual patch should be moved to a public bug that explains what kind > of problem is the bug going to fix. If attachment 8498156 [details] [diff] [review] fixed the problem, this problem is android generic bug.

(In reply to Sotaro Ikeda [:sotaro] from comment #28) > (In reply to Vincent Liu[:vliu] from comment #27) > > vliu, can you explain the above based on implementations of > gfx::ConvertYCbCrToRGB() and gfx::ConvertYCbCrToRGB565()? The above > explanation still does not explain what is actually done during the color > convert. > > http://dxr.mozilla.org/mozilla-central/source/gfx/ycbcr/YCbCrUtils.cpp#70 > http://dxr.mozilla.org/mozilla-central/source/gfx/ycbcr/ycbcr_to_rgb565. > cpp#600 Thanks for your clear explanations. Now I know the unnecessary part including in attachment 8495739 [details] [diff] [review]. The arguments in ConvertYCbCrToRGB565(...) says we don't need mYSize and mCbCrSize. http://dxr.mozilla.org/mozilla-central/source/gfx/ycbcr/YCbCrUtils.cpp#126 (In reply to Sotaro Ikeda [:sotaro] from comment #31) > attachment 8495739 [details] [diff] [review] could do incorrect data access > to destination. The following change extends the destination size to aligned > size. But destination buffer is not allocated by that size. It might try to > access unmapped area by buffer overrun. > > ---------------------------------------------------- > gfx::ConvertYCbCrToRGB(ycbcrData, > aSurface->GetFormat(), > - aSurface->GetSize(), > + alignedSize, > aMappedSurface->mData, > aMappedSurface->mStride); Yes, I agree with your suggestion. This modification do incorrect access for data destination. (In reply to Sotaro Ikeda [:sotaro] from comment #33) > If attachment 8497532 [details] [diff] [review] fixed the problem, > attachment 8498156 [details] [diff] [review] seems also fixed the problem. > And it seems better fix. Correct. aBuffer->getHeight() gets the aligned high. (In reply to Sotaro Ikeda [:sotaro] from comment #35) > This bug is set to confidential bug. Until when this bug reach to committing > state, actual patch should be moved to a public bug that explains what kind > of problem is the bug going to fix. Do you mind if I can arrange the patch you suggested and help me to review it in another public bug?

> (In reply to Sotaro Ikeda [:sotaro] from comment #35) > > This bug is set to confidential bug. Until when this bug reach to committing > > state, actual patch should be moved to a public bug that explains what kind > > of problem is the bug going to fix. > > Do you mind if I can arrange the patch you suggested and help me to review > it in another public bug? It is nice if you create a new bug and works for it, since you are continuously working for this bug :-) I provided the idea how to fix, therefore it seems better to be reviewed by another person. :nical could review it.

(In reply to Sotaro Ikeda [:sotaro] from comment #38) > > (In reply to Sotaro Ikeda [:sotaro] from comment #35) > > > This bug is set to confidential bug. Until when this bug reach to committing > > > state, actual patch should be moved to a public bug that explains what kind > > > of problem is the bug going to fix. > > > > Do you mind if I can arrange the patch you suggested and help me to review > > it in another public bug? > > It is nice if you create a new bug and works for it, since you are > continuously working for this bug :-) > I provided the idea how to fix, therefore it seems better to be reviewed by > another person. :nical could review it. Thanks. I will create this new bug ,and you will be in cc list to let you know any update.

Comment on attachment 8495739 [details] [diff] [review] bug-1068502-fix.patch Clear review request based on Comment 39.

> > > > It is nice if you create a new bug and works for it, since you are > > continuously working for this bug :-) > > I provided the idea how to fix, therefore it seems better to be reviewed by > > another person. :nical could review it. > > Thanks. I will create this new bug ,and you will be in cc list to let you > know any update. vliu, I created a bug for it. It is Bug 1079251. Can you fix it?

(In reply to Sotaro Ikeda [:sotaro] from comment #41) > > > > > > It is nice if you create a new bug and works for it, since you are > > > continuously working for this bug :-) > > > I provided the idea how to fix, therefore it seems better to be reviewed by > > > another person. :nical could review it. > > > > Thanks. I will create this new bug ,and you will be in cc list to let you > > know any update. > > vliu, I created a bug for it. It is Bug 1079251. Can you fix it? Sorry, Bug 1079251 is closed as dup of bug 1075077. It seems better to handle change as part of bug 1075077.

hi, as it's in the end of woodduck project SQC period, would you please feedback if this issue could be fixed before the end of this week? Thank you very much!

(In reply to Sotaro Ikeda [:sotaro] from comment #42) > (In reply to Sotaro Ikeda [:sotaro] from comment #41) > > > > > > > > It is nice if you create a new bug and works for it, since you are > > > > continuously working for this bug :-) > > > > I provided the idea how to fix, therefore it seems better to be reviewed by > > > > another person. :nical could review it. > > > > > > Thanks. I will create this new bug ,and you will be in cc list to let you > > > know any update. > > > > vliu, I created a bug for it. It is Bug 1079251. Can you fix it? > > Sorry, Bug 1079251 is closed as dup of bug 1075077. It seems better to > handle change as part of bug 1075077. Bug 1075077 intends to fix the issue on v2.1 branch. Did you mean we should mark v2.0m affected in bug 1075077 to cherry pick to v2.0m?

Hi Sotaro, If attachment 8498156 [details] [diff] [review] [diff] [review] is the case different from Bug 1075077. Should we separate them into different bug for clear tracking? I suggest reopen bug 1079251 to land this align issue into master branch and cherry pick to 2.0m. How do you think?

(In reply to Vincent Liu[:vliu] from comment #45) > Hi Sotaro, > > If attachment 8498156 [details] [diff] [review] is the case > different from Bug 1075077. Should we separate them into different bug for > clear tracking? I suggest reopen bug 1079251 to land this align issue into > master branch and cherry pick to 2.0m. How do you think? Hi Sataro, Echo Vincent, this is partner blocker bug on 2.0M which they hope we can fix before Friday 10/10. Please help assess the suggestion of Vincent. Thank you very much!

(In reply to Vincent Liu[:vliu] from comment #45) > Hi Sotaro, > > If attachment 8498156 [details] [diff] [review] is the case > different from Bug 1075077. Should we separate them into different bug for > clear tracking? I suggest reopen bug 1079251 to land this align issue into > master branch and cherry pick to 2.0m. How do you think? Thanks for the advice, I re-open bug 1079251 and assign myself because of comment 46.

(In reply to Sotaro Ikeda [:sotaro] from comment #47) > (In reply to Vincent Liu[:vliu] from comment #45) > > Hi Sotaro, > > > > If attachment 8498156 [details] [diff] [review] is the case > > different from Bug 1075077. Should we separate them into different bug for > > clear tracking? I suggest reopen bug 1079251 to land this align issue into > > master branch and cherry pick to 2.0m. How do you think? > > Thanks for the advice, I re-open bug 1079251 and assign myself because of > comment 46. Hi Hi Sotaro, Thanks for the help. I have made bug 1079251 to 2.0M+

Hi Sotaro, May I duplicate this bug to bug 1079251? Seems bug 1079251 is fixed. May I ask Kai-Zhen the 2.0M sheriff to land it on 2.0M? Thanks!

(In reply to Josh Cheng [:josh] from comment #49) > Hi Sotaro, > May I duplicate this bug to bug 1079251? > Seems bug 1079251 is fixed. May I ask Kai-Zhen the 2.0M sheriff to land it > on 2.0M? > Thanks! Yes, thanks!