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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Bruce Sun [:brsun]]

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.

Comment 1

[ying.xu]

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

Comment 2

[Marco Chen [:mchen]]

Fugu met this issue - Bug 891274.

Comment 3

[Marco Chen [:mchen]]

flatfish met this issue too. Bug 924015.

Comment 4

[Marco Chen [:mchen]]

Hi Peter,

Maybe you have any idea for this issue? Thanks.

Comment 5

[Peter Chang[:pchang]]

cc solomon to check

Comment 6

[Solomon Chiu [: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)

Comment 7

[Solomon Chiu [:schiu]]

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

Comment 8

[James Zhang (Spreadtrum)]

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

Comment 9

[Peter Chang[:pchang]]

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

Comment 10

[ying.xu]

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

Comment 11

[James Zhang (Spreadtrum)]

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

Comment 12

[James Zhang (Spreadtrum)]

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

Comment 13

[Keven Kuo [:kkuo]]

Hi! Bruce,

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

--
Keven

Comment 14

[James Zhang (Spreadtrum)]

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

Comment 15

[Bruce Sun [:brsun]]

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

Comment 16

[Jason Smith [:jsmith]]

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

Comment 17

[Steven Yang [:styang]]

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

Comment 18

[Solomon Chiu [:schiu]]

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

Comment 19

[u459114]

(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

Comment 20

[Solomon Chiu [: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.

Comment 21

[u459114]

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?

Comment 22

[Peter Chang[:pchang]]

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 23

[Solomon Chiu [:schiu]]

Attachment: WIP: dolphin patch(1/4): for device/sprd/

Comment 24

[Solomon Chiu [:schiu]]

Attachment: WIP: dolphin patch(2/4): for system/core/include/system/graphics.h

Comment 25

[Solomon Chiu [:schiu]]

Attachment: WIP: dolphin patch(3/4): for gecko/gfx/layers/*

Comment 26

[Solomon Chiu [:schiu]]

Attachment: WIP: dolphin patch(4/4): for framework/av/media/libstagefright

Comment 27

[Solomon Chiu [:schiu]]

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 28

[Solomon Chiu [:schiu]]

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.

Comment 29

[Solomon Chiu [:schiu]]

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 30

[Solomon Chiu [:schiu]]

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 31

[Solomon Chiu [:schiu]]

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.

Comment 32

[Solomon Chiu [:schiu]]

NI pchang for comments.

Comment 33

[Sotaro Ikeda [:sotaro]]

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.

Comment 34

[Peter Chang[:pchang]]

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.

Comment 35

[Peter Chang[: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 36

[Peter Chang[:pchang]]

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.

Comment 37

[Solomon Chiu [:schiu]]

Attachment: WIP: dolphin patch(1/2): for device/sprd/

Comment 38

[Solomon Chiu [:schiu]]

Attachment: WIP: dolphin patch(2/2): for gecko/gfx/layers

Comment 39

[Solomon Chiu [:schiu]]

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 40

[Sotaro Ikeda [:sotaro]]

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 41

[Sotaro Ikeda [:sotaro]]

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.

Comment 42

[Peter Chang[:pchang]]

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

Comment 43

[Solomon Chiu [:schiu]]

Attachment: WIP: dolphin patch(2/2): for gecko/gfx/layers

Comment 44

[Solomon Chiu [:schiu]]

Upload WIP patch#2 according comment#42.

Comment 45

[James Zhang (Spreadtrum)]

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: [e-mail] <[e-mail]>
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,
   };

Comment 46

[Andreas Pehrson [:pehrsons]]

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?

Comment 47

[Solomon Chiu [:schiu]]

Attachment: WIP: dolphin patch(2/2): for gecko/gfx/layers

Comment 48

[Solomon Chiu [:schiu]]

Attachment: WIP: dolphin patch(2/2): for gecko/gfx/layers

Comment 49

[James Zhang (Spreadtrum)]

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

Comment 50

[James Zhang (Spreadtrum)]

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.

Comment 51

[Ivan Tsay (:ITsay)]

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

Comment 52

[Kai-Zhen Li [:seinlin][:kli]]

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

Comment 53

[James Zhang (Spreadtrum)]

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

Comment 54

[Solomon Chiu [:schiu]]

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

Comment 55

[Solomon Chiu [:schiu]]

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.

Comment 56

[Solomon Chiu [:schiu]]

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

Comment 57

[Michael Wu [:mwu]]

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.

Comment 58

[Michael Wu [: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 59

[Solomon Chiu [:schiu]]

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.

Comment 60

[James Zhang (Spreadtrum)]

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

Comment 61

[James Zhang (Spreadtrum)]

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

Comment 62

[Peter Chang[:pchang]]

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.

Comment 63

[Solomon Chiu [:schiu]]

Attachment: Converting with gralloc::lock_ycbcr

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.

Comment 64

[Solomon Chiu [:schiu]]

Attachment: Converting-with-gralloc-lock_ycbcr.patch

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.

Comment 65

[Solomon Chiu [:schiu]]

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.

Comment 66

[Michael Wu [: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..

Comment 67

[James Zhang (Spreadtrum)]

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

[Solomon Chiu [:schiu]]

Attachment: Converting-with-gralloc-lock_ycbcr.patch

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

Comment 69

[Solomon Chiu [:schiu]]

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.

Comment 70

[Solomon Chiu [:schiu]]

Attachment: [v1.4] Converting-with-gralloc-lock_ycbcr.patch

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

Comment 71

[Solomon Chiu [:schiu]]

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.

Comment 72

[Solomon Chiu [:schiu]]

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

Comment 73

[Michael Wu [:mwu]]

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.

Comment 74

[Solomon Chiu [:schiu]]

Attachment: [master m/c] Converting-with-gralloc-lock_ycbcr.patch

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

Comment 75

[Michael Wu [:mwu]]

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;

Comment 76

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

Comment 77

[Solomon Chiu [:schiu]]

Attachment: [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
  }
}

Comment 78

[Solomon Chiu [:schiu]]

Attachment: [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
  }
}

Comment 79

[Solomon Chiu [:schiu]]

Attachment: [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
  }
}

Comment 80

[Solomon Chiu [:schiu]]

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.

Comment 81

[Solomon Chiu [:schiu]]

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

Comment 82

[Michael Wu [: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) {

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.

Comment 83

[Michael Wu [: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.

Comment 84

[Solomon Chiu [:schiu]]

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

Comment 85

[Michael Wu [:mwu]]

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

Comment 86

[Solomon Chiu [:schiu]]

Attachment: [master m/c] Converting YUV with GraphicBuffer::lockYCbCr

Fix broken path base on comment#82

Comment 87

[Solomon Chiu [:schiu]]

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 88

[Peter Chang[: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
@@ +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.

Comment 89

[Solomon Chiu [:schiu]]

Attachment: Converting YUV formats with GraphicBuffer::lockYCbCr

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

Comment 90

[Michael Wu [: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

Comment 91

[Peter Chang[:pchang]]

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.

Comment 92

[Solomon Chiu [:schiu]]

Attachment: Converting YUV formats with GraphicBuffer lockYCbCr

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

Comment 93

[Solomon Chiu [:schiu]]

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.

Comment 94

[Michael Wu [: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

Comment 95

[Peter Chang[:pchang]]

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.

Comment 96

[Solomon Chiu [:schiu]]

Attachment: For mainline - converting color with lockYCbCr of GraphicBuffer, r=mwu

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

Comment 97

[Solomon Chiu [:schiu]]

Attachment: For 1.4+ - converting color with lockYCbCr of GraphicBuffer, r=mwu

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

Comment 98

[Solomon Chiu [:schiu]]

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

Comment 99

[Carsten Book [:Tomcat]]

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

Comment 100

[Carsten Book [:Tomcat]]

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

Comment 101

[Ryan VanderMeulen [:RyanVM]]

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