Closed Bug 1059023 Opened 10 years ago Closed 10 years ago

Do not compose layer hidden under the opaque layer.

Categories

(Core :: Graphics: Layers, defect)

2.0 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED INVALID
blocking-b2g 2.0+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- affected
b2g-v2.0 --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.2 --- wontfix

People

(Reporter: sushilchauhan, Assigned: sushilchauhan)

References

Details

(Whiteboard: [caf priority: p2][CR 714099] [POVB])

Attachments

(1 file, 1 obsolete file)

In Phone and Music Apps, there are frames where Color / Thebes layers are hidden under another Opaque layer. As the hidden layer will never be visible on screen so there is no need to compose the layer hidden under the Opaque layer, it leads to unnecessary processing in display HAL and driver. Display HAL expects the framework (HwcComposer2D) to take care of removing such layers from HWC layer list.
I have attached the patch to fix this issue.

Details: Do not compose layer hidden under the opaque layer. It leads to unnecessary processing in display HAL and driver.
Attachment #8479517 - Flags: review?(sotaro.ikeda.g)
Assignee: nobody → sushilchauhan
Whiteboard: [CR 714099]
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.0?
Comment on attachment 8479517 [details] [diff] [review]
Do not compose layer hidden under the opaque layer.

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

Forward a review to Matt Woodrow. He can do more correct review than I about this patch.
Attachment #8479517 - Flags: review?(sotaro.ikeda.g) → review?(matt.woodrow)
blocking-b2g: 2.0? → 2.0+
Comment on attachment 8479517 [details] [diff] [review]
Do not compose layer hidden under the opaque layer.

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

::: widget/gonk/HwcComposer2D.cpp
@@ +326,5 @@
> +            if (displayRect.Contains(lastRect)) {
> +                // Do not compose layer hidden under the opaque layer
> +                mHwcLayerMap.RemoveElementAt(current-1);
> +                current = --mList->numHwLayers;
> +            }

This only needs to be done for the layer immediately under, we don't have to also consider current-2, current-3, etc. for removal?
This happens recursively whenever a new layer is added to HWC layer list so it takes care of removing current-2 when current-3 was added, and so on.

For ex: In Phone App, before this patch, we had these visible layers in list:
HWC layer[0]::ColorLayerComposite: dst=[0 36 480 800] Color=ff000000 (Opaque)
HWC layer[1]::ColorLayerComposite: dst=[0 36 480 800] Color==ffffffff (Opaque)
HWC layer[2]::ThebesLayerComposite: dst=[0 36 480 800] (Opaque)
HWC layer[3]::ThebesLayerComposite: dst=[0 0 480 36] (Opaque)

With the patch, we are able to remove layer[0] and layer[1]. So remaining 2 HWC layers are sent to HAL for composition:
HWC layer[0]::ThebesLayerComposite: dst=[0 36 480 800] (Opaque)
HWC layer[1]::ThebesLayerComposite: dst=[0 0 480 36] (Opaque)

As per design of HwcComposer2D, we cannot remove layer[n-2] when we are adding layer[n] in HWC layer list. It will be very risky at this point of time. Layer tree build-up code should take care of not marking a translucent layer as visible, which will be hidden by another Opaque layer at much higher z-order.
:milan/:matt?;sotaro, 

I blocked on this hoping it will help battery/perf/memory but hopefully we are comfortable taking this change at this point. will be helpful to get a risk assessment from you'll here.
Comment on attachment 8479517 [details] [diff] [review]
Do not compose layer hidden under the opaque layer.

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

This seems ok for now. I the long term I'd prefer if we computed this sort of thing in LayerComposite code so it can be shared between HWC and OpenGL.

::: widget/gonk/HwcComposer2D.cpp
@@ +321,5 @@
>              mList->numHwLayers = current = 0;
>              mHwcLayerMap.Clear();
> +        } else {
> +            hwc_rect_t r = mList->hwLayers[current-1].displayFrame;
> +            nsIntRect lastRect = nsIntRect(r.left, r.top, r.right - r.left, r.bottom - r.top);

Maybe add a helper so these two lines can be written as:

nsIntRect lastRect = ToIntRect(mList->hwLayers[current-1].displayFrame);

displayRect above could use it too.

@@ +326,5 @@
> +            if (displayRect.Contains(lastRect)) {
> +                // Do not compose layer hidden under the opaque layer
> +                mHwcLayerMap.RemoveElementAt(current-1);
> +                current = --mList->numHwLayers;
> +            }

As Milan said, looping over the layers wouldn't hurt here.
Attachment #8479517 - Flags: review?(matt.woodrow) → review+
(In reply to bhavana bajaj [:bajaj] from comment #6)
> :milan/:matt?;sotaro, 
> 
> I blocked on this hoping it will help battery/perf/memory but hopefully we
> are comfortable taking this change at this point. will be helpful to get a
> risk assessment from you'll here.

It seems fairly low risk. Any bugs will just show up with us failing to render layers which should be spotted easily.
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> Comment on attachment 8479517 [details] [diff] [review]
> Do not compose layer hidden under the opaque layer.
> 
> 
> @@ +326,5 @@
> > +            if (displayRect.Contains(lastRect)) {
> > +                // Do not compose layer hidden under the opaque layer
> > +                mHwcLayerMap.RemoveElementAt(current-1);
> > +                current = --mList->numHwLayers;
> > +            }
> 
> As Milan said, looping over the layers wouldn't hurt here.

In all my testing, I have only seen cases which can be fixed by current patch. Since PrepareLayerList() is recursive, we cover most of the cases with current patch itself. Looping over the underlying layers will create problem in case which I mentioned below. As per current design of HwcComposer2D, it would need adjustments in HWC layer list, which is too risky for v2.0 branch.
Layer z-order 2: Opaque
Layer z-order 1: Opaque (But outside of Layer 2's displayRect on screen)
Layer z-order 0: Opaque (Hidden, it has exact same rectangle as Layer 2 on screen)

Ideally, this bug should be fixed in Layer tree painting code, the hidden layer should be marked as invisible, i.e. layer->GetEffectiveVisibleRegion() should be False. It would help both GPU and HWC.

Since the patch is r+. I am little confused, do you want me to add the helper function ToIntRect() in HwcUtils.cpp now ? Or we can add it later in v2.1 along with looping logic?
Flags: needinfo?(matt.woodrow)
Add the helper now, but don't worry about looping. No need for re-review.
Flags: needinfo?(matt.woodrow)
Uploading HG friendly version of the reviewed patch.
Attachment #8479517 - Attachment is obsolete: true
Attachment #8480743 - Flags: review+
Sotaro,

Can you please push the Attachment #8480743 [details] [diff] to Try Server ?

I tried it but it failed. It asked for Syntax builder string, so I built that from the link: http://trychooser.pub.build.mozilla.org/ by selecting b2g, emulator and android platforms. I specified this syntax with the following commands given at wiki: https://wiki.mozilla.org/Build:TryServer#How_to_push_to_try and pushed again. But it still failed. I can see my change at top of "hg log".

- hg qref --message "try: <your-computed-syntax-here>"
- hg push -f ssh://hg.mozilla.org/try/

Which commands, do you normally use to push to try server?
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sushil from comment #12)
> Sotaro,
> 
> Can you please push the Attachment #8480743 [details] [diff] [diff] to Try Server ?

Done.

> I tried it but it failed. It asked for Syntax builder string, so I built
> that from the link: http://trychooser.pub.build.mozilla.org/ by selecting
> b2g, emulator and android platforms. I specified this syntax with the
> following commands given at wiki:
> https://wiki.mozilla.org/Build:TryServer#How_to_push_to_try and pushed
> again. But it still failed. I can see my change at top of "hg log".
> 
> - hg qref --message "try: <your-computed-syntax-here>"
> - hg push -f ssh://hg.mozilla.org/try/
> 
> Which commands, do you normally use to push to try server?

I used the following to push tryserver.

> hg commit -m "Bug 1059023 - Do not compose layer hidden under the opaque layer. r=mattwoodrow. try: -b do -p emulator,emulator-jb,emulator-kk -u all -t none"
> hg push -f ssh://hg.mozilla.org/try


The above is explained in the following. But it's way seems older than "ReleaseEngineering/TryServer" page.

https://wiki.mozilla.org/Build:TryChooser
Thanks Sotaro. It will help me in future patches.
No problem :-)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0849d066f121
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(In reply to Sushil from comment #12)
> Sotaro,
> 
> Can you please push the Attachment #8480743 [details] [diff] [diff] to Try Server ?
> 
> I tried it but it failed. It asked for Syntax builder string, so I built
> that from the link: http://trychooser.pub.build.mozilla.org/ by selecting
> b2g, emulator and android platforms. I specified this syntax with the

Sushil, do you already have Commit Access permission for tryserver? To push to tryserver Commit Access (Level 1) is necessary.
https://www.mozilla.org/hacking/commit-access-policy/
https://www.mozilla.org/hacking/notification/

My example is Bug 881692 and Bug 829657. You are already a peer of "Widget - Hardware Compositor". You can have Level 3 access.

https://wiki.mozilla.org/Modules/FirefoxOS#Widget_-_Hardware_Compositor
Flags: needinfo?(sushilchauhan)
Yes, I have Level 3 access but I could not specify the build syntax string, required for Try server. It will not be an issue next time.
Flags: needinfo?(sushilchauhan)
Whiteboard: [CR 714099] → [caf priority: p2][CR 714099]
This was backed out for causing bug 1078189.
https://hg.mozilla.org/mozilla-central/rev/a672893cedca
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla34 → ---
The root cause of GPU Composition fallback of a layer, when it lies completely under an Opaque layer is in HAL. I will fix that in HAL. No change needed in Gecko framework.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → INVALID
Whiteboard: [caf priority: p2][CR 714099] → [caf priority: p2][CR 714099] [POVB]
You need to log in before you can comment on or make changes to this bug.