Bruce Sun [:brsun] Reporter
	Description • 11 years ago

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.

	ying.xu
	Comment 1 • 11 years ago

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,
  };

	Marco Chen [:mchen]
	Comment 2 • 11 years ago

Fugu met this issue - Bug 891274.

	Marco Chen [:mchen]
	Comment 3 • 11 years ago

flatfish met this issue too. Bug 924015.

	Marco Chen [:mchen]
	Comment 4 • 11 years ago

Hi Peter,

Maybe you have any idea for this issue? Thanks.

	Peter Chang[:pchang]
	Comment 5 • 11 years ago

cc solomon to check

	Solomon Chiu [:schiu] Assignee
	Comment 6 • 11 years ago

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)

	Solomon Chiu [:schiu] Assignee
	Comment 7 • 11 years ago

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

	James Zhang (Spreadtrum)
	Comment 8 • 11 years ago

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

	Peter Chang[:pchang]
	Comment 9 • 11 years ago

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

	ying.xu
	Comment 10 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/763ace0830dc

	James Zhang (Spreadtrum)
	Comment 11 • 11 years ago

Dolphin on v1.4 still has this issue, video playback can't work now.

	James Zhang (Spreadtrum)
	Comment 12 • 11 years ago

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

	Keven Kuo [:kkuo]
	Comment 13 • 11 years ago

Hi! Bruce,

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

--
Keven

	James Zhang (Spreadtrum)
	Comment 14 • 11 years ago

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

	Bruce Sun [:brsun] Reporter
	Comment 15 • 11 years ago

Hi Solomon, thanks for your information during our offline discussion. Would you mind share your ideas on bugzilla as well?

	Jason Smith [:jsmith]
	Comment 16 • 11 years ago

Steven - Should we be blocking on Dolphin-specific issues for 1.4 right now or not?

	Steven Yang [:styang]
	Comment 17 • 11 years ago

No, we won't block it for generic 1.4.

	Solomon Chiu [:schiu] Assignee
	Comment 18 • 11 years ago

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

	u459114
	Comment 19 • 11 years ago

(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

	Solomon Chiu [:schiu] Assignee
	Comment 20 • 10 years ago

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.

	u459114
	Comment 21 • 10 years ago

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?

	Peter Chang[:pchang]
	Comment 22 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 27 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 28 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 29 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 30 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 31 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 32 • 10 years ago

NI pchang for comments.

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

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.

	Peter Chang[:pchang]
	Comment 34 • 10 years ago

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.

	Peter Chang[:pchang]
	Comment 35 • 10 years ago

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.

	Peter Chang[:pchang]
	Comment 36 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 39 • 10 years ago

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

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

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.

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

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.

	Peter Chang[:pchang]
	Comment 42 • 10 years ago

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

	Solomon Chiu [:schiu] Assignee
	Comment 44 • 10 years ago

Upload WIP patch#2 according comment#42.

	James Zhang (Spreadtrum)
	Comment 45 • 10 years ago

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,
   };

	Andreas Pehrson [:pehrsons]
	Comment 46 • 10 years ago

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?

	James Zhang (Spreadtrum)
	Comment 49 • 10 years ago

(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 Zhang (Spreadtrum)
	Comment 50 • 10 years ago

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.

	Ivan Tsay (:ITsay)
	Comment 51 • 10 years ago

Per discussion with Peter and Solomon, this bug no longer blocks the bug 846421 because the solution will be directly provided in this bug.

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

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

	James Zhang (Spreadtrum)
	Comment 53 • 10 years ago

Sorry, Andreas and Kaizhen, it's my mistake, dolphin's value is different from tarako, and you're right.

	Solomon Chiu [:schiu] Assignee
	Comment 54 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 55 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 56 • 10 years ago

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

	Michael Wu [:mwu]
	Comment 57 • 10 years ago

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.

	Michael Wu [:mwu]
	Comment 58 • 10 years ago

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

	Solomon Chiu [:schiu] Assignee
	Comment 59 • 10 years ago

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.

	James Zhang (Spreadtrum)
	Comment 60 • 10 years ago

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

	James Zhang (Spreadtrum)
	Comment 61 • 10 years ago

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

	Peter Chang[:pchang]
	Comment 62 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 64 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 65 • 10 years ago

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.

	Michael Wu [:mwu]
	Comment 66 • 10 years ago

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

	James Zhang (Spreadtrum)
	Comment 67 • 10 years ago

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

	Solomon Chiu [:schiu] Assignee
	Comment 69 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 70 • 10 years ago

1. Refine the struct initilization according Michael's advice.
2. Refine the unlocking for lockYCbCr.

	Solomon Chiu [:schiu] Assignee
	Comment 71 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 72 • 10 years ago

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

	Michael Wu [:mwu]
	Comment 73 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 74 • 10 years ago

Hi Michael,
I made the patch for master m/c again, and sorry for your inconcenience.

	Michael Wu [:mwu]
	Comment 75 • 10 years ago

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;

	ying.xu
	Comment 76 • 10 years ago

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

	Solomon Chiu [:schiu] Assignee
	Comment 79 • 10 years ago

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
  }
}

	Solomon Chiu [:schiu] Assignee
	Comment 80 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 81 • 10 years ago

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

	Michael Wu [:mwu]
	Comment 82 • 10 years ago

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.

	Michael Wu [:mwu]
	Comment 83 • 10 years ago

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

	Solomon Chiu [:schiu] Assignee
	Comment 84 • 10 years ago

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

	Michael Wu [:mwu]
	Comment 85 • 10 years ago

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

	Solomon Chiu [:schiu] Assignee
	Comment 86 • 10 years ago

Fix broken path base on comment#82

	Solomon Chiu [:schiu] Assignee
	Comment 87 • 10 years ago

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.

	Peter Chang[:pchang]
	Comment 88 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 89 • 10 years ago

1. Move old conversion code to ConvertOmxYUVFormatToRGB565() from GetAsSourceSurface
2. do ConvertVendorYUVFormatToRGB565() first, which perform lockYCbCr for vendor formats.

	Michael Wu [:mwu]
	Comment 90 • 10 years ago

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

	Peter Chang[:pchang]
	Comment 91 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 92 • 10 years ago

Refine the last patch according comment#90 and comment#91

	Solomon Chiu [:schiu] Assignee
	Comment 93 • 10 years ago

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.

	Michael Wu [:mwu]
	Comment 94 • 10 years ago

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

	Peter Chang[:pchang]
	Comment 95 • 10 years ago

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.

	Solomon Chiu [:schiu] Assignee
	Comment 96 • 10 years ago

Rebase to mainline and refine last patch base on comment#94

	Solomon Chiu [:schiu] Assignee
	Comment 97 • 10 years ago

Rebase to 1.4, refine last patch base on comment#94 and carry r+

	Solomon Chiu [:schiu] Assignee
	Comment 98 • 10 years ago

try server result: 
mainline:
1.4: https://tbpl.mozilla.org/?tree=Try&rev=204d7edf3fc7
mainline:   https://tbpl.mozilla.org/?tree=Try&rev=67f30d618c80

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

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

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

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

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/c6da1999f77a
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/9de98994f3d2