1127585 - HwcComposer2D flickers on Android Lollipop

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Diego Wilson [:diego]]

This is due to a misdefined constant HWC_BLIT. This constant value was hard coded into HwcComposer2D but has since changed value in CAF Android Lollipop headers.

https://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#42

Comment 1

[Boris Chiou [:boris]]

(In reply to Diego Wilson [:diego] from comment #0)
> This is due to a misdefined constant HWC_BLIT. This constant value was hard
> coded into HwcComposer2D but has since changed value in CAF Android Lollipop
> headers.
> 
> https://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.
> cpp#42

Should we define HWC_BLIT as a special number (-1 or other integer value), not just "HWC_FRAMEBUFFER_TARGET + 1" or "HW_XXX + 1" to prevent that Android adds new HW compose types in the future?

Comment 2

[Peter Chang[:pchang]]

Boris, please take a look this issue.
NI diego for comment 1.

Comment 3

[Michael Vines [:m1] [:evilmachines]]

There's a patch for this already at https://www.codeaurora.org/cgit/quic/lf/b2g/build/commit/?h=v2.2&id=5e825875151c89c5138a791135d3f976fdd124cf.

Comment 4

[Michael Vines [:m1] [:evilmachines]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #3)
> There's a patch for this already at
> https://www.codeaurora.org/cgit/quic/lf/b2g/build/commit/?h=v2.
> 2&id=5e825875151c89c5138a791135d3f976fdd124cf.

Err..wrong hwc bug, but Sushil is looking at this regardless.

Comment 5

[Sushil]

(In reply to Boris Chiou [:boris] from comment #1)
> (In reply to Diego Wilson [:diego] from comment #0)
> > This is due to a misdefined constant HWC_BLIT. This constant value was hard
> > coded into HwcComposer2D but has since changed value in CAF Android Lollipop
> > headers.
> > 
> > https://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.
> > cpp#42
> 
> Should we define HWC_BLIT as a special number (-1 or other integer value),
> not just "HWC_FRAMEBUFFER_TARGET + 1" or "HW_XXX + 1" to prevent that
> Android adds new HW compose types in the future?

No, we should not define HWC_BLIT to -1, it will not solve the issue with our HWC HAL. Unfortunately, HWC_BLIT definition has not been up-streamed to AOSP yet that's why we have to maintain the local definition of HWC_BLIT composition type in HwcComposer2D because Mozilla servers compile with AOSP HWC headers (while up-steaming). So it is OK to set the value of a new composition type enum as the value after already defined composition types in HWC header. I will upload the patch.

Comment 6

[Sushil]

Attachment: Define HWC_BLIT based on android version.

New definitions have been added in the latest Android version's HWC header, so HWC_BLIT needs to be defined correctly based on version. Otherwise HWC HAL will mark the layers for HWC_BLIT composition but HwcComposer2D will not understand this composition type and it will lead to flickers and corruption.

Comment 7

[Peter Chang[:pchang]]

Comment on attachment 8557715 [details] [diff] [review]
Define HWC_BLIT based on android version.

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

::: widget/gonk/HwcComposer2D.cpp
@@ +41,4 @@
>  
> +#if ANDROID_VERSION >= 21
> +#ifndef HWC_BLIT
> +#define HWC_BLIT (HWC_CURSOR_OVERLAY + 1)

Can we add a new HWC_CUSTOM_COMPOSITETYPE instead of using the last one for android release? Then we don't need to update this define when android version is changed.

For example, HWC_CUSTOM_COMPOSITETYPE 0xFF00
HWC_BLIT (HWC_CUSTOM_COMPOSITETYPE +1)

Comment 8

[Sushil]

(In reply to peter chang[:pchang][:peter] from comment #7)
> Can we add a new HWC_CUSTOM_COMPOSITETYPE instead of using the last one for
> android release? Then we don't need to update this define when android
> version is changed.
> 
> For example, HWC_CUSTOM_COMPOSITETYPE 0xFF00
> HWC_BLIT (HWC_CUSTOM_COMPOSITETYPE +1)

We can consider this suggestion, if we need to update HWC_BLIT for any future android version. But for now, it is too late for us to update HWC_BLIT value in HWC HAL for current android version.

Comment 9

[Sushil]

We can consider to set a high value for HWC_BLIT in HAL, in current Android version's HWC header.

Comment 10

[Sushil]

Attachment: Update HWC_BLIT composition-type value.

Set the value of HWC_BLIT composition type to a higher value, so that there is no need to update it, if new composition types are added in a future android version.

Comment 11

[Peter Chang[:pchang]]

Comment on attachment 8558353 [details] [diff] [review]
Update HWC_BLIT composition-type value.

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

Looks okay to me but it would be nice to have another defination to indicate the start the custom composition type, like HWC_BLIT = HWC_CUSTOM_COMPOSITE_TYPE_START + 1.

Comment 12

[Sushil]

Attachment: Update HWC_BLIT composition-type value. r=sotaro

Uploading the HG friendly version of reviewed patch.

Comment 13

[Sushil]

Try server link: https://tbpl.mozilla.org/?tree=Try&rev=5d6048b0cd36

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72825d3e935b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72825d3e935b

Comment 16

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 8558640 [details] [diff] [review]
Update HWC_BLIT composition-type value. r=sotaro

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 17

[Sushil]

Comment on attachment 8558640 [details] [diff] [review]
Update HWC_BLIT composition-type value. r=sotaro

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Caused by mismatch of HWC_BLIT composition type value in CAF Android Lollipop HWC header.
User impact if declined: Flickers and corruption in HWC_BLIT composition path.
Testing completed: Basic display testing.
Risk to taking this patch (and alternatives if risky): No
String or UUID changes made by this patch: No

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0e11e341b5ba