Closed Bug 881460 Opened 6 years ago Closed 6 years ago

Drawing color of CrystallSkull is not correct in master

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sotaro, Assigned: pchang)

References

Details

Attachments

(3 files, 6 obsolete files)

No description provided.
On master branch, drawing color of CrystalSkull is not correct. The color becomes blue. I confirmed for master Firefox OS on buri device.
Color becomes blue and a vertical line in the drawing.
I see no problem in v1.1 buri. But it might be side effect of Bug 878977.
When I disable the HwComposer. The problem disappears.
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> When I disable the HwComposer. The problem disappears.

But the performance becomes very bad.
Diego, do you know something about it?
Flags: needinfo?(dwilson)
(In reply to Diego Wilson [:diego] from comment #7)
> You're probably missing this patch:
> 
> https://www.codeaurora.org/cgit/quic/lf/b2g/build/commit/patch/all/hardware/
> qcom/display/remap-copybit-formats.patch?h=v1.
> 1&id=088c3bea92c9b66b0086180e6be62fbda5ba5731
> 
> Are you testing the trunk?

Thanks! I am going to check it.
Yes, master has a problem around camera and hw codec. So, I am checking how the master works.
(In reply to Diego Wilson [:diego] from comment #7)
> You're probably missing this patch:
> 
> https://www.codeaurora.org/cgit/quic/lf/b2g/build/commit/patch/all/hardware/
> qcom/display/remap-copybit-formats.patch?h=v1.
> 1&id=088c3bea92c9b66b0086180e6be62fbda5ba5731

I confirmed that the patch fixes the problem. Thanks!
(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> (In reply to Diego Wilson [:diego] from comment #7)
> > You're probably missing this patch:
> > 
> > https://www.codeaurora.org/cgit/quic/lf/b2g/build/commit/patch/all/hardware/
> > qcom/display/remap-copybit-formats.patch?h=v1.
> > 1&id=088c3bea92c9b66b0086180e6be62fbda5ba5731
> 
> I confirmed that the patch fixes the problem. Thanks!

Sorry, it was my misunderstanding. The problem is not fixed by the patch.
(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> Sorry, it was my misunderstanding. The problem is not fixed by the patch.

If you already had it, try removing it. Otherwise the pixel format for webgl canvas may have changed recently.
(In reply to Diego Wilson [:diego] from comment #11)
> (In reply to Sotaro Ikeda [:sotaro] from comment #10)
> > Sorry, it was my misunderstanding. The problem is not fixed by the patch.
> 
> If you already had it, try removing it. Otherwise the pixel format for webgl
> canvas may have changed recently.

Removing the path did not work. I also feel that pixel format might be changed.
with bug 843599, it enables gralloc backend for gl streaming.
Therefore, the format of WebGL/skiaGL is changed (please refer below line).

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/TextureHostOGL.cpp#765

I made a patch to pass format changed event to hwc and it works fine in my local.
Will create patch for review soon.

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> (In reply to Diego Wilson [:diego] from comment #11)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #10)
> > > Sorry, it was my misunderstanding. The problem is not fixed by the patch.
> > 
> > If you already had it, try removing it. Otherwise the pixel format for webgl
> > canvas may have changed recently.
> 
> Removing the path did not work. I also feel that pixel format might be
> changed.
Could this be related to a change I landed yesterday?  See
  
  https://hg.mozilla.org/mozilla-central/rev/492e19f9dd98

I tested this patch with CrystalSkull and never experienced the problem, though.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14)
> Could this be related to a change I landed yesterday?  See
>   
>   https://hg.mozilla.org/mozilla-central/rev/492e19f9dd98
> 
> I tested this patch with CrystalSkull and never experienced the problem,
> though.

I think your patch is fine.

The root cause was related to format changes from bug 843599.
And we need to handle format changes in HWC too.
Attached patch Add format RBSwap flag to hwc (obsolete) — Splinter Review
Assignee: nobody → pchang
uploaded two patches to fix this issue.
Attachment #760807 - Flags: feedback?(dwilson)
I have confirmed that the patches in comment 7, comment 16, and comment 17 fix the viewfinder issues in bug 880780 in master on Inari. Testing Unagi now.
Confirmed these patches fix the viewfinder on Unagi as well! \o/
Awesome, that is great to hear.  It looks like my testing was not done with HWComposer enabled (I thought it was default, guess not?).  Do we know why performance is so bad without hwcomposer?  It's pretty awful, and it seems like it shouldn't be quite that bad.
Standby--it turns out I slurped up a b2g18 build by accident in my tests. (Too much going on.) Retesting now with proper, certified, 100% m-c build. Apologies.
Sorry, folks--it turns out that the Unagi camera viewfinder is still broken, even with the above patches. I'm going to go back and retest Inari as well.

For the record, I have:
- gecko: m-c:86413e921d5d
- gaia: master:c7e704906372d397d26b7985866a1551d1650c1d
*sigh* nope, sorry--looks like the above patches do not fix Inari either. Really sorry to get your hopes up, guys.

For the record, Inari:
- gecko: m-c:86413e921d5d
- gaia: master:ed33f05e869945e12bb05ca94b56c2ce0dea25ef
Remove blocking bug as per mikeh's comments
No longer blocks: 880780
Comment on attachment 760807 [details] [diff] [review]
Add format RBSwap flag to hwc -- qct code changes

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

I like the approach in attachment 760807 [details] [diff] [review]. I think we should take it all the way and get rid of the patch in comment 7. Though to do this we also need to consider RGBX
Attachment #760807 - Flags: feedback?(dwilson)
Any updates on this?  We should really get this fixed; would be good to get rid of the hacky patch in the display driver as Diego mentioned.
Just to be clear, Hamachi on m-c works because HwComposer is off there?
(In reply to Milan Sreckovic [:milan] from comment #28)
> Just to be clear, Hamachi on m-c works because HwComposer is off there?

If Hw Composer is enabled on hamachi, drawing color is not correct.
Attached patch Add format Swap flag to hwc (obsolete) — Splinter Review
Attachment #760805 - Attachment is obsolete: true
Attachment #760807 - Attachment is obsolete: true
Attachment #763394 - Flags: review?(vladimir)
Attachment #763394 - Flags: review?(ncameron)
Attachment #763396 - Flags: review?(dwilson)
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #27)
> Any updates on this?  We should really get this fixed; would be good to get
> rid of the hacky patch in the display driver as Diego mentioned.

Spent some time to verified with RGBA and RGBX content test case.
Now patch was ready for review.
What exactly is being swapped here? The comment in the first patch looks like a swap between 32bit and 16bit colour and the second patch looks like a swap between RGBA and BGRA. I suspect we might just need a better comment in the texture host and something in LayersTypes.h too. BUT, even if it is a swap happening internally, I don't think a swap flag is the correct API to expose in Composer2D - perhaps adding a field for the pixel format is better? There are already at least two enums for this, in 2D and Thebes.
(In reply to Nick Cameron [:nrc] from comment #33)
> What exactly is being swapped here? The comment in the first patch looks
> like a swap between 32bit and 16bit colour and the second patch looks like a
> swap between RGBA and BGRA. I suspect we might just need a better comment in
> the texture host and something in LayersTypes.h too. BUT, even if it is a

The patches only affected the 32bit color format and tried to swap RB if need. For example, RGBA<->BRGA, RGBX->XBGR. Will update more clear description about this part. 

> swap happening internally, I don't think a swap flag is the correct API to
> expose in Composer2D - perhaps adding a field for the pixel format is
> better? There are already at least two enums for this, in 2D and Thebes.

The following are the pixel formats defined by android and used in lots of code sections but it doesn't define the BGRX which used by B2G.

Adding the swap flag is the easier way to notify HWC how to draw the content, without changing any interface or new pixel format which only uses inside FFOS.
http://mxr.mozilla.org/mozilla-central/source/widget/gonk/libui/PixelFormat.h#61
Attached patch Add format Swap flag to hwc v2 (obsolete) — Splinter Review
Attachment #763394 - Attachment is obsolete: true
Attachment #763394 - Flags: review?(vladimir)
Attachment #763394 - Flags: review?(ncameron)
Attachment #763535 - Flags: review?(vladimir)
Attachment #763535 - Flags: review?(ncameron)
Comment on attachment 763535 [details] [diff] [review]
Add format Swap flag to hwc v2

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

This looks fine; I assume that passing in the 0x40 flag to HWC will cause the RB swapping internally?

::: gfx/layers/LayersTypes.h
@@ +66,5 @@
>  // by other surfaces we will need a more generic LayerRenderState.
>  enum LayerRenderStateFlags {
>    LAYER_RENDER_STATE_Y_FLIPPED = 1 << 0,
> +  LAYER_RENDER_STATE_BUFFER_ROTATION = 1 << 1,
> +  LAYER_RENDER_STATE_FORMAT_SWAP = 1 << 2

should be explicit and call this FORMAT_RB_SWAP

::: widget/gonk/HwcComposer2D.cpp
@@ +53,5 @@
>      // Draw a solid color rectangle
>      // The color should be set on the transform member of the hwc_layer_t struct
>      // The expected format is a 32 bit ABGR with 8 bits per component
> +    HWC_COLOR_FILL = 0x8,
> +    HWC_FORMAT_SWAP = 0x40

same, FORMAT_RB_SWAP -- also please add a comment documenting it
Attachment #763535 - Flags: review?(vladimir) → review+
Comment on attachment 763535 [details] [diff] [review]
Add format Swap flag to hwc v2

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

::: gfx/layers/LayersTypes.h
@@ +62,5 @@
>  };
>  
>  // LayerRenderState for Composer2D
>  // We currently only support Composer2D using gralloc. If we want to be backed
>  // by other surfaces we will need a more generic LayerRenderState.

add a comment about LAYER_RENDER_STATE_FORMAT_RB_SWAP

@@ +94,5 @@
>  
>    bool BufferRotated() const
>    { return mFlags & LAYER_RENDER_STATE_BUFFER_ROTATION; }
> +
> +  bool FormatSwap() const

nit FormatSwapped for consistency, FormatRBSwapped would be even better

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +871,5 @@
> +     * For GL rendering content, the content format is RGBA or RGBX which is the same as
> +     * the pixel format of gralloc buffer and no need for the SWAP flag.
> +     */
> +
> +    gfx::SurfaceFormat grallocFormat = SurfaceFormatForAndroidPixelFormat(mGraphicBuffer->getPixelFormat());

When we set mFormat in the first place, lets store a boolean mIsRBSwapped and just use that here to set the flag. Using SurfaceFormatFor... twice (once here and once to set mFormat) and then reverse engineering whether we got an RB swap is too fragile.
Attached patch Add format Swap flag to hwc v3 (obsolete) — Splinter Review
Create v3 patch which includes
a. Change to FORMAT_RB_SWAP and add comment
b. Add mIsRBSwapped flag
Attachment #763535 - Attachment is obsolete: true
Attachment #763535 - Flags: review?(ncameron)
Attachment #763959 - Flags: review?(ncameron)
change wording to FORMAT_RB_SWAP
Attachment #763396 - Attachment is obsolete: true
Attachment #763396 - Flags: review?(dwilson)
Attachment #763960 - Flags: review?(dwilson)
Comment on attachment 763959 [details] [diff] [review]
Add format Swap flag to hwc v3

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

r=me with the added braces

::: widget/gonk/HwcComposer2D.cpp
@@ +408,5 @@
>      hwcLayer.compositionType = HWC_USE_COPYBIT;
>  
>      if (!fillColor) {
> +        if (state.FormatRBSwapped())
> +            hwcLayer.flags |= HWC_FORMAT_RB_SWAP;

use braces
Attachment #763959 - Flags: review?(ncameron) → review+
Comment on attachment 763960 [details] [diff] [review]
Add format Swap flag to hwc v2 - qct code changes

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

LGTM I can land this on CAF after the gecko patch lands
Attachment #763960 - Flags: review?(dwilson) → review+
Fix braces and add reviewers.
Attachment #763959 - Attachment is obsolete: true
Attachment #764507 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/327f0c4ab6d5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Diego Wilson [:diego] from comment #41)
> Comment on attachment 763960 [details] [diff] [review]
> Add format Swap flag to hwc v2 - qct code changes
> 
> Review of attachment 763960 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM I can land this on CAF after the gecko patch lands

Gecko patch was landed.
Blocks: 886415
Created bug 886415 for landing on CAF
You need to log in before you can comment on or make changes to this bug.