HwcComposer2D flickers on Android Lollipop

RESOLVED FIXED in Firefox 38, Firefox OS v2.2

Status

()

Core
Widget: Gonk
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: diego, Assigned: Sushil)

Tracking

unspecified
mozilla38
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [caf priority: p2][CR 789657])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Blocks: 1094121
status-b2g-v2.2: --- → ?
Blocks: 1063044
blocking-b2g: --- → 2.2?

Comment 1

3 years ago
(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?

Updated

3 years ago
Whiteboard: [CR 789657]

Updated

3 years ago
Whiteboard: [CR 789657] → [caf priority: p2][CR 789657]

Comment 2

3 years ago
Boris, please take a look this issue.
NI diego for comment 1.
Flags: needinfo?(dwilson)
Flags: needinfo?(boris.chiou)

Updated

3 years ago
blocking-b2g: 2.2? → 2.2+
(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.

Updated

3 years ago
Flags: needinfo?(boris.chiou)
(Assignee)

Comment 5

3 years ago
(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.
(Assignee)

Comment 6

3 years ago
Created attachment 8557715 [details] [diff] [review]
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.
Attachment #8557715 - Flags: review?(dwilson)

Comment 7

3 years ago
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)
(Assignee)

Comment 8

3 years ago
(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.
(Assignee)

Comment 9

3 years ago
We can consider to set a high value for HWC_BLIT in HAL, in current Android version's HWC header.
(Assignee)

Comment 10

3 years ago
Created attachment 8558353 [details] [diff] [review]
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.
Attachment #8557715 - Attachment is obsolete: true
Attachment #8557715 - Flags: review?(dwilson)
Attachment #8558353 - Flags: review?(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.
Attachment #8558353 - Flags: review?(sotaro.ikeda.g)
Attachment #8558353 - Flags: review?(pchang)
Attachment #8558353 - Flags: review+

Updated

3 years ago
Attachment #8558353 - Flags: review?(sotaro.ikeda.g) → review+
(Assignee)

Comment 12

3 years ago
Created attachment 8558640 [details] [diff] [review]
Update HWC_BLIT composition-type value. r=sotaro

Uploading the HG friendly version of reviewed patch.
Attachment #8558353 - Attachment is obsolete: true
Attachment #8558640 - Flags: review+
(Assignee)

Comment 13

3 years ago
Try server link: https://tbpl.mozilla.org/?tree=Try&rev=5d6048b0cd36
Flags: needinfo?(dwilson)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/72825d3e935b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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:
Attachment #8558640 - Flags: approval-mozilla-b2g37?
(Assignee)

Comment 17

3 years ago
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

Updated

3 years ago
Attachment #8558640 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
(Reporter)

Updated

3 years ago
status-b2g-v2.2: ? → affected
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0e11e341b5ba
status-b2g-v2.2: affected → fixed
status-b2g-master: --- → fixed
status-firefox36: --- → wontfix
status-firefox37: --- → wontfix

Updated

3 years ago
Blocks: 1142220

Updated

3 years ago
No longer blocks: 1142220
You need to log in before you can comment on or make changes to this bug.