Closed Bug 931733 Opened 6 years ago Closed 6 years ago

[Tarako/ Dolphin]Ease vendor porting of HAL_PIXEL_FORMAT_XXX for color format conversion.

Categories

(Core :: Graphics: Layers, defect)

28 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
blocking-b2g 1.4+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: brsun, Assigned: schiu, NeedInfo)

References

Details

Attachments

(2 files, 21 obsolete files)

11.71 KB, patch
schiu
: review+
Details | Diff | Splinter Review
19.19 KB, patch
schiu
: review+
Details | Diff | Splinter Review
Currently HAL_PIXEL_FORMAT_XXX is hardcoded in GrallocImages.h. If we want to port different vendor's pixel format enum, we have to add vendor's customized enum in GrallocImages.h. If the pixel format enum conflicts between any two vendors, the porting mechanism will fail.

Suggest to use a more scalable way to port HAL_PIXEL_FORMAT_XXX enum.
There maybe be a problem in GrallocTextureHost.cpp::SurfaceFormatForAndroidPixelFormat.
code are as followed:

  case HAL_PIXEL_FORMAT_YCbCr_422_SP:
  case HAL_PIXEL_FORMAT_YCrCb_420_SP:
  case HAL_PIXEL_FORMAT_YCbCr_422_I:
  case GrallocImage::HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED:   //here
  case GrallocImage::HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS:   //here
  case HAL_PIXEL_FORMAT_YV12:
    return gfx::FORMAT_B8G8R8A8; 

I think you should handle all the format defined in GrallocImage, not portion of them
  enum {
    /* OEM specific HAL formats */
    HAL_PIXEL_FORMAT_YCbCr_422_P            = 0x12,
    HAL_PIXEL_FORMAT_YCbCr_420_P            = 0x13,
    HAL_PIXEL_FORMAT_YCbCr_420_SP           = 0x21,
    HAL_PIXEL_FORMAT_YCrCb_420_SP_ADRENO    = 0x10A,
    HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED     = 0x20,
    HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS     = 0x7FA30C04,
  };
Fugu met this issue - Bug 891274.
See Also: → 891274
flatfish met this issue too. Bug 924015.
See Also: → 924015
Hi Peter,

Maybe you have any idea for this issue? Thanks.
Flags: needinfo?(pchang)
Blocks: 883051
cc solomon to check
Flags: needinfo?(pchang) → needinfo?(schiu)
Assignee: nobody → schiu
Flags: needinfo?(schiu)
WIP:

I am planning to make following changes:
1. add hardware/libhardware/include/hardware/pixel_format.h. With this file, Mozilla or vendors defines the knowledge of pixel format or relative header files. For example(need to be refined with better including path): 
    a. for Qualcomm: #include "../../../../qcom/display/libgralloc/gralloc_priv.h"
    b. for Samsung: #include "../../../../samsung_slsi/exynos5/include/exynos_format.h"
2. In GrallocImages.h, let's remove the enumeration of HAL_PIXEL_FORMAT_*, and use "#include <hardware/pixel_format.h>" instead.
3. For project fugu(spreadtrum), previous changes about HAL_PIXEL_FORMAT_* should go to HAL directory(such as: defined in hardware/spreadtrum/include/pixel_format.h)
Furthermore, there seems no addtional HAL_PIXEL_FORMAT_* definitions exist in project flatfish code base. The libgralloc only be provided with binary. I guest there should be addtional pixel formats other than those standard definition in "system/core/include/system/graphics.h".
(In reply to Solomon Chiu [:schiu] from comment #7)
> Furthermore, there seems no addtional HAL_PIXEL_FORMAT_* definitions exist
> in project flatfish code base. The libgralloc only be provided with binary.
> I guest there should be addtional pixel formats other than those standard
> definition in "system/core/include/system/graphics.h".

we define in "system/core/include/system/graphics.h".
(In reply to Solomon Chiu [:schiu] from comment #6)
> WIP:
> 
> I am planning to make following changes:
> 1. add hardware/libhardware/include/hardware/pixel_format.h. With this file,
Do we need to create new file?
Why couldn't we put to gralloc.h? Because the format is also used inside gralloc.h
hardware/libhardware/include/hardware/gralloc.h

> Mozilla or vendors defines the knowledge of pixel format or relative header
> files. For example(need to be refined with better including path): 
>     a. for Qualcomm: #include
> "../../../../qcom/display/libgralloc/gralloc_priv.h"
>     b. for Samsung: #include
> "../../../../samsung_slsi/exynos5/include/exynos_format.h"
> 2. In GrallocImages.h, let's remove the enumeration of HAL_PIXEL_FORMAT_*,
> and use "#include <hardware/pixel_format.h>" instead.
> 3. For project fugu(spreadtrum), previous changes about HAL_PIXEL_FORMAT_*
> should go to HAL directory(such as: defined in
> hardware/spreadtrum/include/pixel_format.h)

How does it work in Android to use the colorformat which only exists in their chipset without including related header?
Dolphin on v1.4 still has this issue, video playback can't work now.
blocking-b2g: --- → 1.4?
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(mchen)
Flags: needinfo?(kkuo)
(In reply to peter chang[:pchang][:peter] from comment #9)
> (In reply to Solomon Chiu [:schiu] from comment #6)
> > WIP:
> > 
> > I am planning to make following changes:
> > 1. add hardware/libhardware/include/hardware/pixel_format.h. With this file,
> Do we need to create new file?
> Why couldn't we put to gralloc.h? Because the format is also used inside
> gralloc.h
> hardware/libhardware/include/hardware/gralloc.h
> 
> > Mozilla or vendors defines the knowledge of pixel format or relative header
> > files. For example(need to be refined with better including path): 
> >     a. for Qualcomm: #include
> > "../../../../qcom/display/libgralloc/gralloc_priv.h"
> >     b. for Samsung: #include
> > "../../../../samsung_slsi/exynos5/include/exynos_format.h"
> > 2. In GrallocImages.h, let's remove the enumeration of HAL_PIXEL_FORMAT_*,
> > and use "#include <hardware/pixel_format.h>" instead.
> > 3. For project fugu(spreadtrum), previous changes about HAL_PIXEL_FORMAT_*
> > should go to HAL directory(such as: defined in
> > hardware/spreadtrum/include/pixel_format.h)
> 
> How does it work in Android to use the colorformat which only exists in
> their chipset without including related header?
Can we use dev_pref.js to configure different chipset vendor head file?
Hi! Bruce,

Need your help to take a look.
Please also work with Vincent Liu about dev_pref.js portion. Thanks.

--
Keven
Flags: needinfo?(kkuo) → needinfo?(brsun)
(In reply to Keven Kuo [:kkuo] from comment #13)
> Hi! Bruce,
> 
> Need your help to take a look.
> Please also work with Vincent Liu about dev_pref.js portion. Thanks.
> 
> --
> Keven

Thank you, Keven.
We'll use WIP patch to pass Spreadtrum sanity test first.
Flags: needinfo?(ttsai)
Flags: needinfo?(mchen)
Hi Solomon, thanks for your information during our offline discussion. Would you mind share your ideas on bugzilla as well?
Flags: needinfo?(brsun) → needinfo?(schiu)
Component: General → Graphics: Layers
Product: Firefox OS → Core
Version: unspecified → 28 Branch
Flags: needinfo?(styang)
Steven - Should we be blocking on Dolphin-specific issues for 1.4 right now or not?
Flags: needinfo?(styang)
No, we won't block it for generic 1.4.
Flags: needinfo?(styang)
(In reply to Bruce Sun [:brsun] from comment #15)
> Hi Solomon, thanks for your information during our offline discussion. Would
> you mind share your ideas on bugzilla as well?

Currently Chiajung is working on Bug#880114, which will convert the color with GPU in upper stream call path. We think all vendor-specific color format can all be recognized by corresponding GPU of chipset, such that we can use color-converting shader program to perform the job.
Flags: needinfo?(schiu)
Depends on: 880114
blocking-b2g: 1.4? → backlog
Blocks: 988291
(In reply to Solomon Chiu [:schiu] from comment #18)
> (In reply to Bruce Sun [:brsun] from comment #15)
> > Hi Solomon, thanks for your information during our offline discussion. Would
> > you mind share your ideas on bugzilla as well?
> 
> Currently Chiajung is working on Bug#880114, which will convert the color
> with GPU in upper stream call path. We think all vendor-specific color
> format can all be recognized by corresponding GPU of chipset, such that we
> can use color-converting shader program to perform the job.
Not only bug 880114, but also bug 814524.
I can't see a direct relation between this one and those two. Can you explain
1. The error case of format mismatch(scenario) and why GPU color conversion can prevent this problem.
2. Do we really need color conversion in your use case?
3. In those two bugs, only webgl and canvas2D leverage the power of GLBlitHelper::BlitImageToTexture... is that all we need?

Thanks
Flags: needinfo?(schiu)
The failed cases are:
a. Creating thumbnail(after stop recording video, or while new video appears in video list of player)
b. Press home key to quite camera previewing, snapshot function kicks in and need color conversion job for Card View.

GrallocImage::GetAsSurface() is called to do the color conversion job in these 2 cases.

In this bug, there are 4 related problems exist: 
1. the first problem is: some platform-specfic color format of vendor-A is undefined and unnecessary in vendor-B.
2. the definition and handling of platform-specific color format should not exist in Gecko framework code(in this case, the converting for YCrCb_420_SP_ADRENO/YCrCb_420_SP/YV12 in GrallocImage::GetAsSourceSurface), they should be remove from GrallocImages.cpp.
3. there is no convension/rule among vendors for how to define platform-specific color format. Google defined a common minimal set of color format in system/core/include/system/graphics.h which every vendor have to recognize these format. Thing get complicate when vendors extend their color formats with their own way(as comment#6 mentioned).
4. the last problem is: conflicted format(two vendor use the same definition to represent different color)

Previously we discussed several approaches while we still have to consider 2 rules: using one single official branch, and don't modify the android related code.

1. Create a file - says vendor-color.h, place it in the device/vendor/SOMEVENDOR directory. 
Then duplicate the definition of vendor-specific format to this file. It's straightforward, however doesn't help much for easing vendor's burden from maintaining.
2. Use "#ifndef VENDOR_A_FORMAT" to include header. This will be fail if problem-4 occurs.

Using GPU to do the color conversion do helps for this bug, which is based on the principle: GPU must be able to recognize platform-specific color format and do conversion work.

The pros and cons are:
Pros:
1. GetAsSurface use CPU to do color conversion, GPU converting will be much faster than CPU one.
2. The color format is transparent to the user. GetAsSourceSurface() no longer have to aware the format.
Cons:
1. Need a context to perform the conversion. If there is no existed context can be leveraged, the overhead maybe considerable. 
Card view will encounter this kind of problem, and we need to discuss further to figure out a effective solution.
Flags: needinfo?(schiu)
Depends on: 846421
IC, thanks for your clear answer.

<a> Create a thumbnail in camera app
<b> Create a snapshot of camera preview in card view.

Bug 880114 - Change behavior in CanvasRenderingContext2D::DrawImage. Prevent calling to GrallocImage::GetAsSourceSurface. Fix scenario <a>.
Bug 846421 - by using compositor to generate snapshot image should prevent GrallocImage::GetAsSourceSurface call as well(need to double confirm!!). Fix scenario <b>

I think there are still many things need to be done in this bug.
1. Clean up propietary YUV format in GrallocImage::sColorIdMap[]
2. How to handle SurfaceFormatForAndroidPixelFormat in GrallocTextureHostOGL.cpp?
As we discuss, using BoardConfig.mk to define the pixel format among different chipset is one option.
We could loop more to discuss this proposal after WIP is ready.
Comment on attachment 8438879 [details] [diff] [review]
WIP: dolphin patch(1/4): for device/sprd/

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

::: scx15_sp7715ga/BoardConfig.mk
@@ +106,5 @@
>  
>  # Disable JIT
>  TARGET_DISABLE_JIT := true
> +
> +export GPU_PLATFORM := mali

The variable will not be defined those projects which use Qualcomm SoC for backward compatibility reason now. For those projects other than Qualcomm, the GPU platform should be well defined here. The variable will propagate to gecko/gfx/layers/Makefile.in and relative source files in build time.
Comment on attachment 8438881 [details] [diff] [review]
WIP: dolphin patch(2/4): for system/core/include/system/graphics.h

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

::: include/system/graphics.h
@@ -248,5 @@
> -    HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED = 0x18,
> -    HAL_PIXEL_FORMAT_YCbCr_420_SP       = 0x19,
> -    HAL_PIXEL_FORMAT_YCrCb_420_SP_TILED = 0x1A,
> -    HAL_PIXEL_FORMAT_YCrCb_422_SP       = 0x1B,
> -    HAL_PIXEL_FORMAT_YCrCb_420_P  = 0x1C,

I removed the extra definition which add by the vendor of dolphin. However it cause another problem in Stagefright. Please see patch 4/4 for details. Another consideration that I made this change is: we should align to official AOSP system/* codes as possible as we can, for maintaining reason and minimize the branch diversity.
These patches are discussed with Kaie-Zhen, and we create respective version of patch on each bug. The difference is my version eliminate the vendor-extended format in graphics.h and relative framework changes.
Comment on attachment 8438882 [details] [diff] [review]
WIP: dolphin patch(3/4): for gecko/gfx/layers/*

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

::: gfx/layers/GrallocImages.cpp
@@ +24,5 @@
>  
>  namespace mozilla {
>  namespace layers {
>  
> +#ifndef GPU_PLATFORM

Now this WIP can only fix the conflict between Dolphin and Qualcomm projects. And due to the pre-processor cannot do string comparing, we might need to tweak the variable definition in Makefile advance.
Comment on attachment 8438883 [details] [diff] [review]
WIP: dolphin patch(4/4): for framework/av/media/libstagefright

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

::: media/libstagefright/ACodec.cpp
@@ +572,5 @@
>      if (err != OK) {
>          return err;
>      }
>  
> +#define HAL_PIXEL_FORMAT_YCbCr_420_SP 0x1b

This is the trouble if we eliminate the vendor extended format in system/core/include/system/graphics.h. The vendor of dolphin need this format to pass to its HAL libs. We need to discuss further to see how to fix this problem.
NI pchang for comments.
Flags: needinfo?(pchang)
Attachment #8438883 - Attachment description: WIP: dolphin patch(1/4): for framework/av/media/libstagefright → WIP: dolphin patch(4/4): for framework/av/media/libstagefright
Deliver ifdef to gecko's source code need a lot of change. If we want to deliver device specific code, system property might be used for it. Other thing is deliver ifdef to gonk code that use Android.mk.
My original proposal is like the following.
//BoardConfig.mk
HAL_PIXEL_FORMAT_YCbCr_422_P := 0x12
HAL_PIXEL_FORMAT_YCbCr_420_P := 0x13

//Makefile.in
ifdef HAL_PIXEL_FORMAT_YCbCr_422_P
CXXFLAGS += -DHAL_PIXEL_FORMAT_YCbCr_422_P=$(HAL_PIXEL_FORMAT_YCbCr_422_P)
$(warning CXXFLAGS=$(CXXFLAGS))
endif
...

//GrallocImage.h
#ifndef HAL_PIXEL_FORMAT_YCbCr_422_P
#define HAL_PIXEL_FORMAT_YCbCr_422_P xxx //default value for QCT;
#endif
...

As Sotaro said,we might consider to put those definition to gonk instead of GrallocImage.h.
Flags: needinfo?(pchang)
Comment on attachment 8438882 [details] [diff] [review]
WIP: dolphin patch(3/4): for gecko/gfx/layers/*

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

::: gfx/layers/GrallocImages.cpp
@@ +24,5 @@
>  
>  namespace mozilla {
>  namespace layers {
>  
> +#ifndef GPU_PLATFORM

With the changes of comment 34, we don't need this hack condition checking.
Comment on attachment 8438883 [details] [diff] [review]
WIP: dolphin patch(4/4): for framework/av/media/libstagefright

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

::: media/libstagefright/ACodec.cpp
@@ +572,5 @@
>      if (err != OK) {
>          return err;
>      }
>  
> +#define HAL_PIXEL_FORMAT_YCbCr_420_SP 0x1b

I think this file is maintained by vendor. Does it have problem if we don't remove definition in graphics.h? With new boardconfig.mk, you can suggest vendor how to change this part.
Attachment #8438879 - Attachment is obsolete: true
Attachment #8438881 - Attachment is obsolete: true
Attachment #8438882 - Attachment is obsolete: true
Attachment #8438883 - Attachment is obsolete: true
Attachment #8442957 - Flags: review?(sotaro.ikeda.g)
Attachment #8442957 - Flags: review?(pchang)
Attachment #8442959 - Flags: review?(sotaro.ikeda.g)
Attachment #8442959 - Flags: review?(pchang)
Comment on attachment 8442959 [details] [diff] [review]
WIP: dolphin patch(2/2): for gecko/gfx/layers

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

::: gfx/layers/GrallocImages.cpp
@@ +30,5 @@
>      HAL_PIXEL_FORMAT_YCbCr_420_P, OMX_COLOR_FormatYUV420Planar,
>      HAL_PIXEL_FORMAT_YCbCr_422_P, OMX_COLOR_FormatYUV422Planar,
>      HAL_PIXEL_FORMAT_YCbCr_420_SP, OMX_COLOR_FormatYUV420SemiPlanar,
>      HAL_PIXEL_FORMAT_YCrCb_420_SP, -1,
> +#ifndef HAL_PIXEL_FORMAT_CbYCrY_420_I

Currently the format "HAL_PIXEL_FORMAT_CbYCrY_420_I" exists in project Dolphin, so we can leverage it to distinguish with QCT projects.

::: gfx/layers/GrallocImages.h
@@ -135,5 @@
> -    HAL_PIXEL_FORMAT_YCbCr_420_SP           = 0x109,
> -    HAL_PIXEL_FORMAT_YCrCb_420_SP_ADRENO    = 0x10A,
> -    HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED     = 0x7FA30C03,
> -    HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS     = 0x7FA30C04,
> -  };

The formats are moved to gecko/hal/gonk/VendorPixelFormat.h
Comment on attachment 8442959 [details] [diff] [review]
WIP: dolphin patch(2/2): for gecko/gfx/layers

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

some general comments.

VendorPixelFormat.h is used only from gfx layers in gecko. It seems better to store the file under gfx/layers.
HAL_PIXEL_FORMAT_CbYCrY_420_I is used to check spreadtrum specific definition. The neme of HAL_PIXEL_FORMAT_CbYCrY_420_I seems too general to identify it.

Makefile.in need to be checked by build peer.
Comment on attachment 8442957 [details] [diff] [review]
WIP: dolphin patch(1/2): for device/sprd/

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

This patch seems better to be checked by mwu. Forward the review.
Attachment #8442957 - Flags: review?(sotaro.ikeda.g) → review?(mwu)
Attachment #8442959 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8442959 [details] [diff] [review]
WIP: dolphin patch(2/2): for gecko/gfx/layers

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

::: hal/gonk/VendorPixelFormat.h
@@ +21,5 @@
> +    HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED     = 0x7FA30C03,
> +    HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS     = 0x7FA30C04,
> +};
> +
> +#endif

It is hard for others to know why it defines above formats if not define HAL_PIXEL_FORMAT_YCbCr_420_I.

I would prefer the following changes and it is easier to maintain these vendor pixel format.

#ifndef HAL_PIXEL_FORMAT_A
#define HAL_PIXEL_FORMAT_A 1 //default value for QCT;
#endif

#ifndef HAL_PIXEL_FORMAT_B
#define HAL_PIXEL_FORMAT_B 2 //default value for QCT;
#endif

#ifndef HAL_PIXEL_FORMAT_C
#define HAL_PIXEL_FORMAT_C 3 //default value for QCT;
#endif
Attachment #8442959 - Flags: review?(pchang) → review-
Attachment #8442959 - Attachment is obsolete: true
Attachment #8443381 - Flags: review?(pchang)
Attachment #8443381 - Flags: review?(mwu)
Upload WIP patch#2 according comment#42.
Hi Andreas, you should land this patch on your git repo for Spreadtrum chipset.
We have landed this patch on v1.3t, but can't land on 1.4 now.

commit 864deea9694e07e113d8b1bef58a345e7988c5b6
Author: ying.xu@spreadtrum.com <ying.xu@spreadtrum.com>
Date:   Thu Feb 13 13:46:07 2014 +0800

    Bug 931733 - Ease vendor porting of HAL_PIXEL_FORMAT_XXX for color format conversion r=me a=1.3t+

diff --git a/gfx/layers/GrallocImages.h b/gfx/layers/GrallocImages.h
index 742d6fb..99800bc 100644
--- a/gfx/layers/GrallocImages.h
+++ b/gfx/layers/GrallocImages.h
@@ -112,11 +112,11 @@ public:
   // From [android 4.0.4]/hardware/msm7k/libgralloc-qsd8k/gralloc_priv.h
   enum {
     /* OEM specific HAL formats */
-    HAL_PIXEL_FORMAT_YCbCr_422_P            = 0x102,
-    HAL_PIXEL_FORMAT_YCbCr_420_P            = 0x103,
-    HAL_PIXEL_FORMAT_YCbCr_420_SP           = 0x109,
+    HAL_PIXEL_FORMAT_YCbCr_422_P            = 0x12,
+    HAL_PIXEL_FORMAT_YCbCr_420_P            = 0x13,
+    HAL_PIXEL_FORMAT_YCbCr_420_SP           = 0x21,
     HAL_PIXEL_FORMAT_YCrCb_420_SP_ADRENO    = 0x10A,
-    HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED     = 0x7FA30C03,
+    HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED     = 0x20,
     HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS     = 0x7FA30C04,
   };
Flags: needinfo?(pehrsons)
Blocks: 1028053
blocking-b2g: backlog → 1.4?
Thanks James. Just to confirm that patch. In the dolphin tree I have system/core/include/system/graphics.h which says:

    HAL_PIXEL_FORMAT_YCbCr_422_SP       = 0x10, // NV16
    HAL_PIXEL_FORMAT_YCrCb_420_SP       = 0x11, // NV21                                                                                                       
    HAL_PIXEL_FORMAT_YCbCr_422_P  = 0x12,
    HAL_PIXEL_FORMAT_YCbCr_420_P  = 0x13,
    HAL_PIXEL_FORMAT_YCbCr_422_I        = 0x14, // YUY2
    HAL_PIXEL_FORMAT_YCbCr_420_I  = 0x15,
    HAL_PIXEL_FORMAT_CbYCrY_422_I = 0x16,
    HAL_PIXEL_FORMAT_CbYCrY_420_I = 0x17,
    HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED = 0x18,  <-- your patch says 0x20
    HAL_PIXEL_FORMAT_YCbCr_420_SP       = 0x19,  <-- your patch says 0x21
    HAL_PIXEL_FORMAT_YCrCb_420_SP_TILED = 0x1A,
    HAL_PIXEL_FORMAT_YCrCb_422_SP       = 0x1B,
    HAL_PIXEL_FORMAT_YCrCb_420_P  = 0x1C,


Which values are correct?
Flags: needinfo?(pehrsons) → needinfo?(james.zhang)
Attachment #8443381 - Attachment is obsolete: true
Attachment #8443381 - Flags: review?(pchang)
Attachment #8443381 - Flags: review?(mwu)
Attachment #8444194 - Flags: review?(pchang)
Attachment #8444194 - Flags: review?(mwu)
Attachment #8444194 - Attachment is obsolete: true
Attachment #8444194 - Flags: review?(pchang)
Attachment #8444194 - Flags: review?(mwu)
Attachment #8444197 - Flags: review?(pchang)
Attachment #8444197 - Flags: review?(mwu)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #46)
> Thanks James. Just to confirm that patch. In the dolphin tree I have
> system/core/include/system/graphics.h which says:
> 
>     HAL_PIXEL_FORMAT_YCbCr_422_SP       = 0x10, // NV16
>     HAL_PIXEL_FORMAT_YCrCb_420_SP       = 0x11, // NV21                     
> 
>     HAL_PIXEL_FORMAT_YCbCr_422_P  = 0x12,
>     HAL_PIXEL_FORMAT_YCbCr_420_P  = 0x13,
>     HAL_PIXEL_FORMAT_YCbCr_422_I        = 0x14, // YUY2
>     HAL_PIXEL_FORMAT_YCbCr_420_I  = 0x15,
>     HAL_PIXEL_FORMAT_CbYCrY_422_I = 0x16,
>     HAL_PIXEL_FORMAT_CbYCrY_420_I = 0x17,
>     HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED = 0x18,  <-- your patch says 0x20
>     HAL_PIXEL_FORMAT_YCbCr_420_SP       = 0x19,  <-- your patch says 0x21
>     HAL_PIXEL_FORMAT_YCrCb_420_SP_TILED = 0x1A,
>     HAL_PIXEL_FORMAT_YCrCb_422_SP       = 0x1B,
>     HAL_PIXEL_FORMAT_YCrCb_420_P  = 0x1C,
> 
> 
> Which values are correct?

My patch for dolphin is correct.
HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED     = 0x20,
HAL_PIXEL_FORMAT_YCbCr_420_SP           = 0x21,
Flags: needinfo?(james.zhang)
Hi Andreas, you can also wait for Solomon's patch landing, it will remove QC hard code, and Spreadtrum can configure this value in device/sprd git repo.
Blocks: 1027517
No longer depends on: 880114
Per discussion with Peter and Solomon, this bug no longer blocks the bug 846421 because the solution will be directly provided in this bug.
No longer depends on: 846421
(In reply to James Zhang (Spreadtrum) from comment #49)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #46)
> > Thanks James. Just to confirm that patch. In the dolphin tree I have
> > system/core/include/system/graphics.h which says:
> > 
> >     HAL_PIXEL_FORMAT_YCbCr_422_SP       = 0x10, // NV16
> >     HAL_PIXEL_FORMAT_YCrCb_420_SP       = 0x11, // NV21                     
> > 
> >     HAL_PIXEL_FORMAT_YCbCr_422_P  = 0x12,
> >     HAL_PIXEL_FORMAT_YCbCr_420_P  = 0x13,
> >     HAL_PIXEL_FORMAT_YCbCr_422_I        = 0x14, // YUY2
> >     HAL_PIXEL_FORMAT_YCbCr_420_I  = 0x15,
> >     HAL_PIXEL_FORMAT_CbYCrY_422_I = 0x16,
> >     HAL_PIXEL_FORMAT_CbYCrY_420_I = 0x17,
> >     HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED = 0x18,  <-- your patch says 0x20
> >     HAL_PIXEL_FORMAT_YCbCr_420_SP       = 0x19,  <-- your patch says 0x21
> >     HAL_PIXEL_FORMAT_YCrCb_420_SP_TILED = 0x1A,
> >     HAL_PIXEL_FORMAT_YCrCb_422_SP       = 0x1B,
> >     HAL_PIXEL_FORMAT_YCrCb_420_P  = 0x1C,
> > 
> > 
> > Which values are correct?
> 
> My patch for dolphin is correct.
> HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED     = 0x20,
> HAL_PIXEL_FORMAT_YCbCr_420_SP           = 0x21,

James, where can we get the values correctly?
In system/core/include/system/graphics.h, I can see 0x18, 0x19 and the patch is copied from here.
249     HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED = 0x18,
250     HAL_PIXEL_FORMAT_YCbCr_420_SP       = 0x19,
Flags: needinfo?(james.zhang)
Sorry, Andreas and Kaizhen, it's my mistake, dolphin's value is different from tarako, and you're right.
Flags: needinfo?(james.zhang)
Michael Wu gave me a great idea, which is use GraphicBuffer::lockYCbCr() to get the addresses of Y/Cb/Cr, and strides, and the do the color conversion jobs. If the vendors can provide the implementation of lock_ycbcr() in gralloc module, then we can do the conversion without handling the definition/enumeration of  color formats.
Attachment #8442957 - Attachment is obsolete: true
Attachment #8444197 - Attachment is obsolete: true
Attachment #8442957 - Flags: review?(pchang)
Attachment #8442957 - Flags: review?(mwu)
Attachment #8444197 - Flags: review?(pchang)
Attachment #8444197 - Flags: review?(mwu)
Attachment #8445042 - Flags: review?(pchang)
Attachment #8445042 - Flags: review?(mwu)
Comment on attachment 8445042 [details] [diff] [review]
Converting-with-gralloc-lock_ycbcr.patch

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

::: gfx/layers/GrallocImages.cpp
@@ +304,3 @@
>    int32_t rv =
> +    graphicBuffer->lockYCbCr(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &ycbcr);
> +  bool isVendorFormat = false;

Here we use lockYCbCr() to get the addresses of Y/Cb/Cr buffer, and strides, which are packed in the structure android_ycbcr. If the vendor provides the implementation of lock_ycbcr() in gralloc module, then we can use it to do the color conversion job by using existed function. However, if the vendor doesn't provide the implementation of lock_ycbcr, or lack of handling taks of specific format, then we have to call GraphicsBuffer::lock(), to get the address of YCbCr raw buffer.

@@ -323,5 @@
> -
> -  if (!omxFormat) {
> -    NS_WARNING("Unknown color format");
> -    return nullptr;
> -  }

Above code which handling omxFormat is moved to line 396, in order to put the handling logic togather.
James,

I saw you already handled 2 color formats: HAL_PIXEL_FORMAT_YCrCb_420_SP and HAL_PIXEL_FORMAT_YCbCr_420_SP. Could you please add another 2 color format: HAL_PIXEL_FORMAT_YCbCr_420_P and HAL_PIXEL_FORMAT_YCrCb_420_P into the function?
Meanwhile, to test the patch that I just uploaded, could you please provide some sample videos which is encoded with following formats?

HAL_PIXEL_FORMAT_YCbCr_420_SP
HAL_PIXEL_FORMAT_YCrCb_420_SP
HAL_PIXEL_FORMAT_YCbCr_420_P
HAL_PIXEL_FORMAT_YCrCb_420_P
HAL_PIXEL_FORMAT_YV12

Thanks
Flags: needinfo?(james.zhang)
Comment on attachment 8445042 [details] [diff] [review]
Converting-with-gralloc-lock_ycbcr.patch

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

I think the lock_ycbcr code should be placed after checking for YV12 and maybe some other AOSP standard formats. Running lock_ycbcr without checking the format is wrong, but seems to be more compatible.

::: gfx/layers/GrallocImages.cpp
@@ +304,3 @@
>    int32_t rv =
> +    graphicBuffer->lockYCbCr(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &ycbcr);
> +  bool isVendorFormat = false;

Where is this variable used? (isVendorFormat)

@@ +326,5 @@
> +      // next.  This is 2 bytes for semiplanar (because chroma values are interleaved
> +      // and each chroma value is one byte) and 1 for planar.
> +      ConvertYVU420SPToRGB565(ycbcr.y, ycbcr.ystride, ycbcr.cb, ycbcr.cstride,
> +          mappedSurface.mData,
> +          width, height);

This looks wrong - we need to verify the cb/cr order.

@@ +336,5 @@
> +      ycbcrData.mYSize        = GetSize();
> +      ycbcrData.mCbChannel    = static_cast<uint8_t*>(ycbcr.cb);
> +      ycbcrData.mCrChannel    = static_cast<uint8_t*>(ycbcr.cr);
> +      ycbcrData.mCbCrStride   = ycbcr.cstride;
> +      ycbcrData.mPicSize      = GetSize();

We should also be setting mYCbCrSize, I think. Use chroma_step to set mCbSkip/mCrSkip. Unfortunately, ConvertYCbCrToRGB doesn't support mC*Skip setting. If we can get that working right, we can avoid the ConvertYVU420SPToRGB565 special case above.
Attachment #8445042 - Flags: review?(mwu)
(In reply to Solomon Chiu [:schiu] from comment #56)
> Meanwhile, to test the patch that I just uploaded, could you please provide
> some sample videos which is encoded with following formats?
> 

These surface formats have no relation with specific videos. They're an artifact of the hardware decoder. Testing this with a video recorded with the camera is usually enough.
Comment on attachment 8445042 [details] [diff] [review]
Converting-with-gralloc-lock_ycbcr.patch

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

::: gfx/layers/GrallocImages.cpp
@@ +304,3 @@
>    int32_t rv =
> +    graphicBuffer->lockYCbCr(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &ycbcr);
> +  bool isVendorFormat = false;

Not used anymore, will removed.

@@ +326,5 @@
> +      // next.  This is 2 bytes for semiplanar (because chroma values are interleaved
> +      // and each chroma value is one byte) and 1 for planar.
> +      ConvertYVU420SPToRGB565(ycbcr.y, ycbcr.ystride, ycbcr.cb, ycbcr.cstride,
> +          mappedSurface.mData,
> +          width, height);

Yre are right, I think I can get the order by comparing address of cb/cr.

@@ +336,5 @@
> +      ycbcrData.mYSize        = GetSize();
> +      ycbcrData.mCbChannel    = static_cast<uint8_t*>(ycbcr.cb);
> +      ycbcrData.mCrChannel    = static_cast<uint8_t*>(ycbcr.cr);
> +      ycbcrData.mCbCrStride   = ycbcr.cstride;
> +      ycbcrData.mPicSize      = GetSize();

Agree, I will try this idea.
Flags: needinfo?(james.zhang)
(In reply to Solomon Chiu [:schiu] from comment #56)
> James,
> 
> I saw you already handled 2 color formats: HAL_PIXEL_FORMAT_YCrCb_420_SP and
> HAL_PIXEL_FORMAT_YCbCr_420_SP. Could you please add another 2 color format:
> HAL_PIXEL_FORMAT_YCbCr_420_P and HAL_PIXEL_FORMAT_YCrCb_420_P into the
> function?
> Meanwhile, to test the patch that I just uploaded, could you please provide
> some sample videos which is encoded with following formats?
> 
> HAL_PIXEL_FORMAT_YCbCr_420_SP
> HAL_PIXEL_FORMAT_YCrCb_420_SP
> HAL_PIXEL_FORMAT_YCbCr_420_P
> HAL_PIXEL_FORMAT_YCrCb_420_P
> HAL_PIXEL_FORMAT_YV12
> 
> Thanks

Need Alan's help.
Flags: needinfo?(alan.wang)
(In reply to Solomon Chiu [:schiu] from comment #56)
> James,
> 
> I saw you already handled 2 color formats: HAL_PIXEL_FORMAT_YCrCb_420_SP and
> HAL_PIXEL_FORMAT_YCbCr_420_SP. Could you please add another 2 color format:
> HAL_PIXEL_FORMAT_YCbCr_420_P and HAL_PIXEL_FORMAT_YCrCb_420_P into the
> function?
> Meanwhile, to test the patch that I just uploaded, could you please provide
> some sample videos which is encoded with following formats?
> 
> HAL_PIXEL_FORMAT_YCbCr_420_SP
> HAL_PIXEL_FORMAT_YCrCb_420_SP
> HAL_PIXEL_FORMAT_YCbCr_420_P
> HAL_PIXEL_FORMAT_YCrCb_420_P
> HAL_PIXEL_FORMAT_YV12
> 
> Thanks

Our OMXCodec owner said Spreadtrum chipset don't support HAL_PIXEL_FORMAT_YCbCr_420_P and HAL_PIXEL_FORMAT_YCrCb_420_P video input/output now, I'll ask our graphic owner why add these format here.
blocking-b2g: 1.4? → 1.4+
Comment on attachment 8445042 [details] [diff] [review]
Converting-with-gralloc-lock_ycbcr.patch

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

::: gfx/layers/GrallocImages.cpp
@@ +299,5 @@
>  {
>    android::sp<GraphicBuffer> graphicBuffer =
>      GrallocBufferActor::GetFrom(GetSurfaceDescriptor());
>  
> +  struct android_ycbcr ycbcr;

android_ycbcr ycbcr = android_ycbcr();
Initialize the struct first.

@@ +304,2 @@
>    int32_t rv =
> +    graphicBuffer->lockYCbCr(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &ycbcr);

Need to check android version which supports the lockYCbCr interface

BTW, I think we only need to call lockYCbCr after below line and return if rv!=OK
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages.cpp#217

@@ +316,5 @@
>      NS_WARNING("Could not map DataSourceSurface");
>      return nullptr;
>    }
>  
> +  if (!rv) {

Check rv==OK

@@ +329,5 @@
> +          mappedSurface.mData,
> +          width, height);
> +    } else {
> +      struct layers::PlanarYCbCrData ycbcrData;
> +

Initialize above structure.
Attachment #8445042 - Flags: review?(pchang) → review-
1. Modify the the function ConvertYVU420SPToRGB565 to respect the order of Cb/Cr
2. Apply the same modification of GetAsSourceSurface to DeprecatedGetAsSourceSurface
3. Handing AOSP format first, then omxFormat, and then vendor format.
Attachment #8445800 - Attachment is obsolete: true
Comment on attachment 8445042 [details] [diff] [review]
Converting-with-gralloc-lock_ycbcr.patch

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

::: gfx/layers/GrallocImages.cpp
@@ +299,5 @@
>  {
>    android::sp<GraphicBuffer> graphicBuffer =
>      GrallocBufferActor::GetFrom(GetSurfaceDescriptor());
>  
> +  struct android_ycbcr ycbcr;

done.

@@ +304,2 @@
>    int32_t rv =
> +    graphicBuffer->lockYCbCr(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &ycbcr);

done.

@@ +316,5 @@
>      NS_WARNING("Could not map DataSourceSurface");
>      return nullptr;
>    }
>  
> +  if (!rv) {

done.

@@ +329,5 @@
> +          mappedSurface.mData,
> +          width, height);
> +    } else {
> +      struct layers::PlanarYCbCrData ycbcrData;
> +

done.
Attachment #8445802 - Flags: review?(pchang)
Attachment #8445802 - Flags: review?(mwu)
Comment on attachment 8445802 [details] [diff] [review]
Converting-with-gralloc-lock_ycbcr.patch

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

I don't see DeprecatedGetAsSurface in my gecko tree, so I don't think you need to duplicate this code. A backport might need it, but I need to review patches against master, not branches.

::: gfx/layers/GrallocImages.cpp
@@ +288,5 @@
> +
> +  if (!omxFormat) {
> +    NS_WARNING("Unknown color format");
> +#if ANDROID_VERSION >= 18
> +  struct android_ycbcr ycbcr = android_ycbcr();

No need to use struct in c++ code - android_ycbcr is enough. The same applies to PlanarYCbCrData below.

Also, this part of the code is misindented.

@@ +290,5 @@
> +    NS_WARNING("Unknown color format");
> +#if ANDROID_VERSION >= 18
> +  struct android_ycbcr ycbcr = android_ycbcr();
> +
> +  graphicBuffer->unlock();

This is messy. We should be locking and unlocking the graphicBuffer closer to where it's actually used, preferably using a class that'll own that for us. Also, I think this means we'll unlock *twice* unless you do something explicit to the GraphicBufferAutoUnlock that's holding on to this.

@@ +306,5 @@
> +                              ycbcr.cb, ycbcr.cr, ycbcr.cstride,
> +                              imageSurface->Data(),
> +                              width, height);
> +    } else {
> +      struct layers::PlanarYCbCrData ycbcrData = layers::PlanarYCbCrData();

Don't think we need to explicitly initialize ycbcrData - I think C++ should do that for us..
Attachment #8445802 - Flags: review?(mwu)
(In reply to James Zhang (Spreadtrum) from comment #61)
> (In reply to Solomon Chiu [:schiu] from comment #56)
> > James,
> > 
> > I saw you already handled 2 color formats: HAL_PIXEL_FORMAT_YCrCb_420_SP and
> > HAL_PIXEL_FORMAT_YCbCr_420_SP. Could you please add another 2 color format:
> > HAL_PIXEL_FORMAT_YCbCr_420_P and HAL_PIXEL_FORMAT_YCrCb_420_P into the
> > function?
> > Meanwhile, to test the patch that I just uploaded, could you please provide
> > some sample videos which is encoded with following formats?
> > 
> > HAL_PIXEL_FORMAT_YCbCr_420_SP
> > HAL_PIXEL_FORMAT_YCrCb_420_SP
> > HAL_PIXEL_FORMAT_YCbCr_420_P
> > HAL_PIXEL_FORMAT_YCrCb_420_P
> > HAL_PIXEL_FORMAT_YV12
> > 
> > Thanks
> 
> Our OMXCodec owner said Spreadtrum chipset don't support
> HAL_PIXEL_FORMAT_YCbCr_420_P and HAL_PIXEL_FORMAT_YCrCb_420_P video
> input/output now, I'll ask our graphic owner why add these format here.

Our GPU owner said these formats are in android2.3, and android4.0 remove them.
Our GPU supports these formats but we don't use them now.
Comment on attachment 8445802 [details] [diff] [review]
Converting-with-gralloc-lock_ycbcr.patch

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

::: gfx/layers/GrallocImages.cpp
@@ +288,5 @@
> +
> +  if (!omxFormat) {
> +    NS_WARNING("Unknown color format");
> +#if ANDROID_VERSION >= 18
> +  struct android_ycbcr ycbcr = android_ycbcr();

done.

@@ +290,5 @@
> +    NS_WARNING("Unknown color format");
> +#if ANDROID_VERSION >= 18
> +  struct android_ycbcr ycbcr = android_ycbcr();
> +
> +  graphicBuffer->unlock();

I plcaed a dedicated auto unlock for lockYCbCr in the new patch. The nesting lock here seems ok because the usage of GraphicBuffer is "read often", and they all return the same address after locking.

@@ +306,5 @@
> +                              ycbcr.cb, ycbcr.cr, ycbcr.cstride,
> +                              imageSurface->Data(),
> +                              width, height);
> +    } else {
> +      struct layers::PlanarYCbCrData ycbcrData = layers::PlanarYCbCrData();

done.
1. Refine the struct initilization according Michael's advice.
2. Refine the unlocking for lockYCbCr.
Attachment #8446270 - Attachment is obsolete: true
Comment on attachment 8446273 [details] [diff] [review]
[v1.4] Converting-with-gralloc-lock_ycbcr.patch

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

::: gfx/layers/GrallocImages.cpp
@@ +291,5 @@
> +#if ANDROID_VERSION >= 18
> +    android_ycbcr ycbcr;
> +
> +    rv =
> +      graphicBuffer->lockYCbCr(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &ycbcr);

This part of code is still needed by project Dolphin, due to it's still using v1.4. Making thunbnail of video in Video app calls DeprecatedGetAsSurface(). And CardView calls GetAsSurface().

@@ +441,5 @@
> +
> +    if (rv == OK) {
> +      // The vendor provides explicit addresses of Y/Cb/Cr buffer from lockYCbCr
> +
> +      GraphicBufferAutoUnlock ycbcr_unlock(graphicBuffer);

I placed a dedicated auto unlock here. The nesting lock here seems ok because the usage of GraphicBuffer is "read often", and they all return the same address after locking.
Attachment #8446273 - Flags: review?(pchang)
Attachment #8446273 - Flags: review?(mwu)
(In reply to James Zhang (Spreadtrum) from comment #67)
> (In reply to James Zhang (Spreadtrum) from comment #61)
> > (In reply to Solomon Chiu [:schiu] from comment #56)
> > > James,
> > > 
> > > I saw you already handled 2 color formats: HAL_PIXEL_FORMAT_YCrCb_420_SP and
> > > HAL_PIXEL_FORMAT_YCbCr_420_SP. Could you please add another 2 color format:
> > > HAL_PIXEL_FORMAT_YCbCr_420_P and HAL_PIXEL_FORMAT_YCrCb_420_P into the
> > > function?
> > > Meanwhile, to test the patch that I just uploaded, could you please provide
> > > some sample videos which is encoded with following formats?
> > > 
> > > HAL_PIXEL_FORMAT_YCbCr_420_SP
> > > HAL_PIXEL_FORMAT_YCrCb_420_SP
> > > HAL_PIXEL_FORMAT_YCbCr_420_P
> > > HAL_PIXEL_FORMAT_YCrCb_420_P
> > > HAL_PIXEL_FORMAT_YV12
> > > 
> > > Thanks
> > 
> > Our OMXCodec owner said Spreadtrum chipset don't support
> > HAL_PIXEL_FORMAT_YCbCr_420_P and HAL_PIXEL_FORMAT_YCrCb_420_P video
> > input/output now, I'll ask our graphic owner why add these format here.
> 
> Our GPU owner said these formats are in android2.3, and android4.0 remove
> them.
> Our GPU supports these formats but we don't use them now.

Thanks for your information, James, and would you please also have your engineer to check my patch to see if there exist any corner case that I didn't consider?
Flags: needinfo?(james.zhang)
Comment on attachment 8446273 [details] [diff] [review]
[v1.4] Converting-with-gralloc-lock_ycbcr.patch

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

As mentioned before, I don't want to review branch patches. Please provide a patch against master/m-c.
Attachment #8446273 - Flags: review?(mwu)
Hi Michael,
I made the patch for master m/c again, and sorry for your inconcenience.
Attachment #8446297 - Flags: review?(pchang)
Attachment #8446297 - Flags: review?(mwu)
Attachment #8446273 - Attachment filename: Converting-with-gralloc-lock_ycbcr.patch → [v1.4] Converting-with-gralloc-lock_ycbcr.patch
Attachment #8446273 - Attachment description: Converting-with-gralloc-lock_ycbcr.patch → [v1.4] Converting-with-gralloc-lock_ycbcr.patch
Comment on attachment 8446297 [details] [diff] [review]
[master m/c] Converting-with-gralloc-lock_ycbcr.patch

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

::: gfx/layers/GrallocImages.cpp
@@ +216,5 @@
>  
>    if (rv) {
>      NS_WARNING("Couldn't lock graphic buffer");
>      return nullptr;
>    }

The documentation for gralloc lock says that lock will fail on certain buffers that need to be locked by lock_ycbcr. This will prevent us from hitting lock_ycbcr in the cases where we need it.

@@ +286,5 @@
> +    }
> +  }
> +
> +  if (!omxFormat) {
> +    NS_WARNING("Unknown color format");

This isn't a valid warning in cases where the format is HAL_PIXEL_FORMAT_YCbCr_*_888.

@@ +294,5 @@
> +
> +    rv =
> +      graphicBuffer->lockYCbCr(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &ycbcr);
> +
> +    if (rv == OK) {

if (rv != OK)
  return nullptr;
Attachment #8446297 - Flags: review?(mwu)
Flags: needinfo?(james.zhang) → needinfo?(ying.xu)
(In reply to Solomon Chiu [:schiu] from comment #54)
> Created attachment 8445042 [details] [diff] [review]
> Converting-with-gralloc-lock_ycbcr.patch
> 
> Michael Wu gave me a great idea, which is use GraphicBuffer::lockYCbCr() to
> get the addresses of Y/Cb/Cr, and strides, and the do the color conversion
> jobs. If the vendors can provide the implementation of lock_ycbcr() in
> gralloc module, then we can do the conversion without handling the
> definition/enumeration of  color formats.

Frankly, we don't prefer this method very much.
The reason of this bug is simple, and what we want is gecko can recognize the format of sprd.
Change the flow of GetAsSourceSurface as following pseudo code:

if (lock failed) {
  // The vendor provide a proper way to lock YUV buffer
  try lockYCbCr to get Y/Cb/Cr addresses and do converting
} else {
  handle HAL_PIXEL_FORMAT_YCrCb_420_SP_ADRENO
  handle HAL_PIXEL_FORMAT_YCrCb_420_SP
  handle HAL_PIXEL_FORMAT_YV12

  if (pixel format doesn't match omxFormat) {
    // The vendor doesn't provide a proper locking method
    // (means YUV buffer can be locked by lock())
    // which we have to use lockYCbCr to lock again.
    try lockYCbCr to get Y/Cb/Cr addresses and do converting
  } else {
    do stagefright's color converter
  }
}
Attachment #8446551 - Attachment is obsolete: true
Comment on attachment 8446297 [details] [diff] [review]
[master m/c] Converting-with-gralloc-lock_ycbcr.patch

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

::: gfx/layers/GrallocImages.cpp
@@ +216,5 @@
>  
>    if (rv) {
>      NS_WARNING("Couldn't lock graphic buffer");
>      return nullptr;
>    }

In the latest patch I just uploaded, I put the lockCbCr() and relative converting job into a function - ConvertVendorYUVFormatToRGB565(), and change the flow in GetAsSourceSurface() which based on your previous advices.

@@ +286,5 @@
> +    }
> +  }
> +
> +  if (!omxFormat) {
> +    NS_WARNING("Unknown color format");

Moved to a proper place(line 362 of new patch).

@@ +294,5 @@
> +
> +    rv =
> +      graphicBuffer->lockYCbCr(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &ycbcr);
> +
> +    if (rv == OK) {

Added "else" block to handle the failure case.
(In reply to ying.xu from comment #76)
> (In reply to Solomon Chiu [:schiu] from comment #54)
> > Created attachment 8445042 [details] [diff] [review]
> > Converting-with-gralloc-lock_ycbcr.patch
> > 
> > Michael Wu gave me a great idea, which is use GraphicBuffer::lockYCbCr() to
> > get the addresses of Y/Cb/Cr, and strides, and the do the color conversion
> > jobs. If the vendors can provide the implementation of lock_ycbcr() in
> > gralloc module, then we can do the conversion without handling the
> > definition/enumeration of  color formats.
> 
> Frankly, we don't prefer this method very much.
> The reason of this bug is simple, and what we want is gecko can recognize
> the format of sprd.

I understand your concern, this changes seems a little bit complicate than using conditional compilation. However, by using the lockYCbCr() call which Michael adviced, we can cover most vendor's need(not only you and Qualcomm) in the future, and maximize the generality of color format handling.
Attachment #8446555 - Flags: review?(pchang)
Attachment #8446555 - Flags: review?(mwu)
Attachment #8446273 - Flags: review?(pchang)
Comment on attachment 8446555 [details] [diff] [review]
[master m/c] Converting YUV with GraphicBuffer::lockYCbCr

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

::: gfx/layers/GrallocImages.cpp
@@ +208,5 @@
> + * of vendor formats.
> + */
> +static status_t
> +ConvertVendorYUVFormatToRGB565(android::sp<GraphicBuffer>& aBuffer,
> +                               gfx::DataSourceSurface *aSurface) {

Probably need to check ANDROID_VERSION for this code.

This function is misindented.

@@ +214,5 @@
> +
> +    int32_t rv =
> +      aBuffer->lockYCbCr(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &ycbcr);
> +
> +    if (rv == OK) {

if (rv != OK)
  return rv;

@@ +227,5 @@
> +        // next.  This is 2 bytes for semiplanar (because chroma values are interleaved
> +        // and each chroma value is one byte) and 1 for planar.
> +        ConvertYVU420SPToRGB565(ycbcr.y, ycbcr.ystride,
> +            ycbcr.cb, ycbcr.cr, ycbcr.cstride,
> +            aSurface->GetData(),

GetData() is deprecated - you should get data and stride info from MappedSurface.

@@ +268,5 @@
>    RefPtr<gfx::DataSourceSurface> surface
>      = gfx::Factory::CreateDataSourceSurface(GetSize(), gfx::SurfaceFormat::R5G6B5);
>    if (!surface) {
>      NS_WARNING("Failed to create SourceSurface.");
>      return nullptr;

In this failure path, graphicBuffer is not unlocked.

@@ +275,4 @@
>    gfx::DataSourceSurface::MappedSurface mappedSurface;
>    if (!surface->Map(gfx::DataSourceSurface::WRITE, &mappedSurface)) {
>      NS_WARNING("Could not map DataSourceSurface");
>      return nullptr;

Also in this failure path, graphicBuffer is not unlocked. You should move the lock to after the ConvertVendorYUVFormatToRGB565 code.
Attachment #8446555 - Flags: review?(mwu)
(In reply to Solomon Chiu [:schiu] from comment #79)
> Created attachment 8446555 [details] [diff] [review]
> [master m/c] Converting YUV with GraphicBuffer::lockYCbCr
> 
> Change the flow of GetAsSourceSurface as following pseudo code:
> 
> if (lock failed) {
>   // The vendor provide a proper way to lock YUV buffer
>   try lockYCbCr to get Y/Cb/Cr addresses and do converting
> } else {
>   handle HAL_PIXEL_FORMAT_YCrCb_420_SP_ADRENO
>   handle HAL_PIXEL_FORMAT_YCrCb_420_SP
>   handle HAL_PIXEL_FORMAT_YV12
> 
>   if (pixel format doesn't match omxFormat) {
>     // The vendor doesn't provide a proper locking method
>     // (means YUV buffer can be locked by lock())
>     // which we have to use lockYCbCr to lock again.
>     try lockYCbCr to get Y/Cb/Cr addresses and do converting
>   } else {
>     do stagefright's color converter
>   }
> }

I would suggest moving all the old conversion code that relies on gralloc lock to a separate function so it can properly manage unlocking when it's done. The code can do something like this:

1. Check for HAL_PIXEL_FORMAT_YCbCr_*_888 formats. If found, use the new lock_ycbcr based conversion code immediately and return.
2. Run the old conversion code.
3. If that fails, run the new lock_ycbcr based conversion code.
(In reply to Michael Wu [:mwu] from comment #83)
> (In reply to Solomon Chiu [:schiu] from comment #79)
> > Created attachment 8446555 [details] [diff] [review]
> > [master m/c] Converting YUV with GraphicBuffer::lockYCbCr
> > 
> > Change the flow of GetAsSourceSurface as following pseudo code:
> > 
> > if (lock failed) {
> >   // The vendor provide a proper way to lock YUV buffer
> >   try lockYCbCr to get Y/Cb/Cr addresses and do converting
> > } else {
> >   handle HAL_PIXEL_FORMAT_YCrCb_420_SP_ADRENO
> >   handle HAL_PIXEL_FORMAT_YCrCb_420_SP
> >   handle HAL_PIXEL_FORMAT_YV12
> > 
> >   if (pixel format doesn't match omxFormat) {
> >     // The vendor doesn't provide a proper locking method
> >     // (means YUV buffer can be locked by lock())
> >     // which we have to use lockYCbCr to lock again.
> >     try lockYCbCr to get Y/Cb/Cr addresses and do converting
> >   } else {
> >     do stagefright's color converter
> >   }
> > }
> 
> I would suggest moving all the old conversion code that relies on gralloc
> lock to a separate function so it can properly manage unlocking when it's
> done. The code can do something like this:
> 
> 1. Check for HAL_PIXEL_FORMAT_YCbCr_*_888 formats. If found, use the new
> lock_ycbcr based conversion code immediately and return.
> 2. Run the old conversion code.
> 3. If that fails, run the new lock_ycbcr based conversion code.

Agree, meanwhile we are also dealing with several new vendors, I need to check further if they have specicial interpretation against android_ycbcr struct, especially some of vendor gralloc library are not open-source.

I would like to create another bug to do this refactoring work. How about your opinion?
(In reply to Solomon Chiu [:schiu] from comment #84)
> I would like to create another bug to do this refactoring work. How about
> your opinion?

The patch as it stands now has several broken error paths. The restructuring I suggested lets you fix them, but if you can find some other simple way, that's fine. The error paths can't be left broken.
Fix broken path base on comment#82
Attachment #8446273 - Attachment is obsolete: true
Attachment #8446555 - Attachment is obsolete: true
Attachment #8446555 - Flags: review?(pchang)
Attachment #8446994 - Flags: review?(mwu)
Comment on attachment 8446555 [details] [diff] [review]
[master m/c] Converting YUV with GraphicBuffer::lockYCbCr

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

::: gfx/layers/GrallocImages.cpp
@@ +208,5 @@
> + * of vendor formats.
> + */
> +static status_t
> +ConvertVendorYUVFormatToRGB565(android::sp<GraphicBuffer>& aBuffer,
> +                               gfx::DataSourceSurface *aSurface) {

done.

@@ +227,5 @@
> +        // next.  This is 2 bytes for semiplanar (because chroma values are interleaved
> +        // and each chroma value is one byte) and 1 for planar.
> +        ConvertYVU420SPToRGB565(ycbcr.y, ycbcr.ystride,
> +            ycbcr.cb, ycbcr.cr, ycbcr.cstride,
> +            aSurface->GetData(),

done.

@@ +268,5 @@
>    RefPtr<gfx::DataSourceSurface> surface
>      = gfx::Factory::CreateDataSourceSurface(GetSize(), gfx::SurfaceFormat::R5G6B5);
>    if (!surface) {
>      NS_WARNING("Failed to create SourceSurface.");
>      return nullptr;

fixed.

@@ +275,4 @@
>    gfx::DataSourceSurface::MappedSurface mappedSurface;
>    if (!surface->Map(gfx::DataSourceSurface::WRITE, &mappedSurface)) {
>      NS_WARNING("Could not map DataSourceSurface");
>      return nullptr;

done.
Comment on attachment 8446555 [details] [diff] [review]
[master m/c] Converting YUV with GraphicBuffer::lockYCbCr

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

::: gfx/layers/GrallocImages.cpp
@@ +275,4 @@
>    gfx::DataSourceSurface::MappedSurface mappedSurface;
>    if (!surface->Map(gfx::DataSourceSurface::WRITE, &mappedSurface)) {
>      NS_WARNING("Could not map DataSourceSurface");
>      return nullptr;

I would prefer to call graphicBuffer->lock first and then call ConvertVendorYUVFormatToRGB565 if needs.
Because we always need to call lock API but not call ConvertVendorYUVFormatToRGB565 every time.

@@ +277,5 @@
>      NS_WARNING("Could not map DataSourceSurface");
>      return nullptr;
>    }
>  
> +  if (rv != OK) {

Do we need to handle when graphicBuffer->lock failed?

@@ +294,5 @@
> +    surface->Unmap();
> +    return nullptr;
> +  }
> +
> +  GraphicBufferAutoUnlock unlock(graphicBuffer);

Suggest to move this back after graphicBuffer->lock called.

@@ +356,5 @@
> +
> +    if (rv == OK) {
> +      surface->Unmap();
> +      return surface;
> +    } else

Need to Unmap surface if rv is not OK.
Attachment #8446555 - Attachment is obsolete: false
Summary: Ease vendor porting of HAL_PIXEL_FORMAT_XXX for color format conversion. → [Tarako/ Dolphin]Ease vendor porting of HAL_PIXEL_FORMAT_XXX for color format conversion.
Attachment #8446994 - Flags: review?(mwu)
1. Move old conversion code to ConvertOmxYUVFormatToRGB565() from GetAsSourceSurface
2. do ConvertVendorYUVFormatToRGB565() first, which perform lockYCbCr for vendor formats.
Attachment #8446555 - Attachment is obsolete: true
Attachment #8446994 - Attachment is obsolete: true
Attachment #8448603 - Flags: review?(pchang)
Attachment #8448603 - Flags: review?(mwu)
Comment on attachment 8448603 [details] [diff] [review]
Converting YUV formats with GraphicBuffer::lockYCbCr

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

::: gfx/layers/GrallocImages.cpp
@@ +210,5 @@
> +static status_t
> +ConvertVendorYUVFormatToRGB565(android::sp<GraphicBuffer>& aBuffer,
> +                               gfx::DataSourceSurface *aSurface,
> +                               gfx::DataSourceSurface::MappedSurface *aMappedSurface) {
> +  int32_t rv = -EINVAL;

use status_t and BAD_VALUE;

@@ +219,5 @@
> +  // Check if the vendor provides explicit addresses of Y/Cb/Cr buffer from lockYCbCr
> +  rv = aBuffer->lockYCbCr(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &ycbcr);
> +
> +  if (rv != OK) {
> +    NS_WARNING("Couldn't lock graphic buffer by lockYCbCr()");

s/by/using/

@@ +265,3 @@
>    uint32_t omxFormat = 0;
>  
> +  for (int i = 0; GrallocImage::sColorIdMap[i]; i += 2) {

GrallocImage:: is unnecessary.

@@ +277,5 @@
> +static status_t
> +ConvertOmxYUVFormatToRGB565(android::sp<GraphicBuffer>& aBuffer,
> +                            gfx::DataSourceSurface *aSurface,
> +                            gfx::DataSourceSurface::MappedSurface *aMappedSurface,
> +                            layers::PlanarYCbCrData *aYcbcrData) {

I think aYcbcrData would work better as a reference than a pointer.

@@ +284,5 @@
> +
> +  rv = aBuffer->lock(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &buffer);
> +  if (rv != OK) {
> +    NS_WARNING("Couldn't lock graphic buffer");
> +    return -EINVAL;

Since this returns status_t, you should use BAD_VALUE here and elsewhere in this function.

@@ +292,5 @@
>  
> +  uint32_t format = aBuffer->getPixelFormat();
> +  uint32_t width = aSurface->GetSize().width;
> +  uint32_t height = aSurface->GetSize().height;
> +  uint8_t* buffer_as_bytes = static_cast<uint8_t*>(buffer);

Can you just make buffer a uint8_t* to begin with?

@@ +368,5 @@
> +  android::sp<GraphicBuffer> graphicBuffer =
> +    mTextureClient->GetGraphicBuffer();
> +
> +  RefPtr<gfx::DataSourceSurface> surface
> +    = gfx::Factory::CreateDataSourceSurface(GetSize(), gfx::SurfaceFormat::R5G6B5);

The = needs to go on the previous line.

@@ +383,5 @@
> +
> +  int32_t rv;
> +  uint32_t omxFormat = 0;
> +
> +  omxFormat = GrallocImage::GetOmxFormat(graphicBuffer->getPixelFormat());

pass omxFormat to ConvertOmxYUVFormatTORGB565 so that function doesn't need to determine it again.

@@ +399,5 @@
> +
> +  rv = ConvertOmxYUVFormatToRGB565(graphicBuffer, surface, &mappedSurface, &mData);
> +  surface->Unmap();
> +
> +  if (rv != OK ) {

unnecessary space after OK
Attachment #8448603 - Flags: review?(mwu)
Comment on attachment 8448603 [details] [diff] [review]
Converting YUV formats with GraphicBuffer::lockYCbCr

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

::: gfx/layers/GrallocImages.cpp
@@ +155,5 @@
>                          uint32_t aWidth, uint32_t aHeight)
>  {
>    uint8_t *y = (uint8_t*)aYData;
> +  bool isOrderCbCr;
> +  int8_t *uv;

s/isOrderCbCr/isCbCr

@@ +226,2 @@
>  
> +  GraphicBufferAutoUnlock unlock(aBuffer);

I guess we don't need unlock buffer here.
Attachment #8448603 - Flags: review?(pchang)
Refine the last patch according comment#90 and comment#91
Attachment #8448603 - Attachment is obsolete: true
Comment on attachment 8448603 [details] [diff] [review]
Converting YUV formats with GraphicBuffer::lockYCbCr

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

::: gfx/layers/GrallocImages.cpp
@@ +155,5 @@
>                          uint32_t aWidth, uint32_t aHeight)
>  {
>    uint8_t *y = (uint8_t*)aYData;
> +  bool isOrderCbCr;
> +  int8_t *uv;

done.

@@ +210,5 @@
> +static status_t
> +ConvertVendorYUVFormatToRGB565(android::sp<GraphicBuffer>& aBuffer,
> +                               gfx::DataSourceSurface *aSurface,
> +                               gfx::DataSourceSurface::MappedSurface *aMappedSurface) {
> +  int32_t rv = -EINVAL;

done.

@@ +219,5 @@
> +  // Check if the vendor provides explicit addresses of Y/Cb/Cr buffer from lockYCbCr
> +  rv = aBuffer->lockYCbCr(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &ycbcr);
> +
> +  if (rv != OK) {
> +    NS_WARNING("Couldn't lock graphic buffer by lockYCbCr()");

done.

@@ +226,2 @@
>  
> +  GraphicBufferAutoUnlock unlock(aBuffer);

We still need it, becuase lockYCbCr() calls lock_ycbcr() of gralloc module, and lock_ycbcr() also does mmap() as lock(). So we still need unlock do the munmap() and cleaning jobs.

@@ +265,3 @@
>    uint32_t omxFormat = 0;
>  
> +  for (int i = 0; GrallocImage::sColorIdMap[i]; i += 2) {

removed.

@@ +277,5 @@
> +static status_t
> +ConvertOmxYUVFormatToRGB565(android::sp<GraphicBuffer>& aBuffer,
> +                            gfx::DataSourceSurface *aSurface,
> +                            gfx::DataSourceSurface::MappedSurface *aMappedSurface,
> +                            layers::PlanarYCbCrData *aYcbcrData) {

fixed.

@@ +284,5 @@
> +
> +  rv = aBuffer->lock(android::GraphicBuffer::USAGE_SW_READ_OFTEN, &buffer);
> +  if (rv != OK) {
> +    NS_WARNING("Couldn't lock graphic buffer");
> +    return -EINVAL;

fixed.

@@ +292,5 @@
>  
> +  uint32_t format = aBuffer->getPixelFormat();
> +  uint32_t width = aSurface->GetSize().width;
> +  uint32_t height = aSurface->GetSize().height;
> +  uint8_t* buffer_as_bytes = static_cast<uint8_t*>(buffer);

done.

@@ +368,5 @@
> +  android::sp<GraphicBuffer> graphicBuffer =
> +    mTextureClient->GetGraphicBuffer();
> +
> +  RefPtr<gfx::DataSourceSurface> surface
> +    = gfx::Factory::CreateDataSourceSurface(GetSize(), gfx::SurfaceFormat::R5G6B5);

fixed.

@@ +383,5 @@
> +
> +  int32_t rv;
> +  uint32_t omxFormat = 0;
> +
> +  omxFormat = GrallocImage::GetOmxFormat(graphicBuffer->getPixelFormat());

done.

@@ +399,5 @@
> +
> +  rv = ConvertOmxYUVFormatToRGB565(graphicBuffer, surface, &mappedSurface, &mData);
> +  surface->Unmap();
> +
> +  if (rv != OK ) {

fixed.
Attachment #8449260 - Flags: review?(pchang)
Attachment #8449260 - Flags: review?(mwu)
Comment on attachment 8449260 [details] [diff] [review]
Converting YUV formats with GraphicBuffer lockYCbCr

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

Looks good to me, but you'll probably also need a gfx peer to stamp this.

::: gfx/layers/GrallocImages.cpp
@@ +289,3 @@
>    }
>  
> +  int32_t rv;

nit: status_t

@@ +384,5 @@
> +
> +  int32_t rv;
> +  uint32_t omxFormat = 0;
> +
> +  omxFormat = GrallocImage::GetOmxFormat(graphicBuffer->getPixelFormat());

Now that there's only one caller of GetOmxFormat, you can inline that function into here.

@@ +392,5 @@
> +
> +    if (rv != OK) {
> +      NS_WARNING("Unknown color format");
> +      return nullptr;
> +    } else {

nit: don't need else

@@ +402,5 @@
> +  surface->Unmap();
> +
> +  if (rv != OK) {
> +    return nullptr;
> +  } else {

nit: don't need else
Attachment #8449260 - Flags: review?(mwu) → review+
Comment on attachment 8449260 [details] [diff] [review]
Converting YUV formats with GraphicBuffer lockYCbCr

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

LGTM, add sotaro as peer review.
Attachment #8449260 - Flags: review?(sotaro.ikeda.g)
Attachment #8449260 - Flags: review?(pchang)
Attachment #8449260 - Flags: review+
No longer blocks: 1028053
Attachment #8449260 - Flags: review?(sotaro.ikeda.g) → review+
Rebase to mainline and refine last patch base on comment#94
Attachment #8449260 - Attachment is obsolete: true
Attachment #8450865 - Flags: review+
Rebase to 1.4, refine last patch base on comment#94 and carry r+
Attachment #8450867 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cd8c0b04b8e6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.