Remove hwc buffer swap from LayerManagerComposite::Render()

RESOLVED INVALID

Status

()

Core
Graphics: Layers
RESOLVED INVALID
3 years ago
2 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected)

Details

Attachments

(2 attachments, 16 obsolete attachments)

30.96 KB, patch
Details | Diff | Splinter Review
8.83 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
hwc buffer swap is called in LayerManagerComposite::Render() via Composer2D::Render() and Composer2D::TryRenderWithHwc(). It is not good if we want to add multiple display support. Because hwc buffer swap needs to be called for all hwc displays. We need to remove hwc buffer swap from LayerManagerComposite::Render().
(Assignee)

Updated

3 years ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Comment 1

3 years ago
Created attachment 8596172 [details] [diff] [review]
wip patch

The patch is created based on Bug 1156981 patch.
(Assignee)

Updated

3 years ago
Blocks: 1116089
(Assignee)

Comment 2

3 years ago
Created attachment 8596578 [details] [diff] [review]
wip patch

Fix ICS problem.
Attachment #8596172 - Attachment is obsolete: true
(Assignee)

Comment 3

3 years ago
Created attachment 8597421 [details] [diff] [review]
patch - Remove hwc buffer swap from LayerManagerComposite::Render()
Attachment #8596578 - Attachment is obsolete: true
(Assignee)

Comment 4

3 years ago
Created attachment 8597423 [details] [diff] [review]
patch - Remove hwc buffer swap from LayerManagerComposite::Render()
Attachment #8597421 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Created attachment 8597426 [details] [diff] [review]
patch - Remove hwc buffer swap from LayerManagerComposite::Render()

Fix nits.
Attachment #8597423 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
Created attachment 8597430 [details] [diff] [review]
patch - Remove hwc buffer swap from LayerManagerComposite::Render()

Update nits.
Attachment #8597426 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8607711 [details] [diff] [review]
patch - Remove hwc buffer swap from LayerManagerComposite::Render()

unbitrot.
Attachment #8597430 - Attachment is obsolete: true
(Assignee)

Comment 8

2 years ago
Created attachment 8608362 [details] [diff] [review]
patch - Remove hwc buffer swap from LayerManagerComposite::Render()
Attachment #8607711 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
Created attachment 8608722 [details] [diff] [review]
patch - Remove hwc buffer swap from LayerManagerComposite::Render()

Update nits.
Attachment #8608362 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
Created attachment 8608916 [details] [diff] [review]
patch - Remove hwc buffer swap from LayerManagerComposite::Render()

Fix some problems. The patch seems to work on master flame-kk.
Attachment #8608722 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
Created attachment 8608927 [details] [diff] [review]
patch part 1 - Change to Widget

Change HwcComposer2D as to do hwc swap only at HwcComposer2D::Render() for supporting multiple displays. On current gecko, HwcComposer2D::TryHwComposition() also do hwc swap.
Attachment #8608916 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8608927 - Attachment description: patch - Change to Widget → patch part 1 - Change to Widget
(Assignee)

Comment 12

2 years ago
Created attachment 8608930 [details] [diff] [review]
patch part 2 - Change t gfx layers

Remove calling composer2D->Render() from LayerManagerComposite::Render(). It is necessary to handle correctly multiple hwc displays rendering. hwc hal api does  all hwc displays rendering by one function call and depends on primary display's vsync.
(Assignee)

Comment 13

2 years ago
Created attachment 8608933 [details] [diff] [review]
patch part 2 - Change t gfx layers

Correct patch.
Attachment #8608930 - Attachment is obsolete: true
(Assignee)

Comment 14

2 years ago
Created attachment 8608985 [details] [diff] [review]
patch part 2 - Change t gfx layers

Update nits.
Attachment #8608933 - Attachment is obsolete: true
(Assignee)

Comment 15

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd7d7ab7a349
(Assignee)

Comment 16

2 years ago
Created attachment 8609357 [details] [diff] [review]
patch part 2 - Change t gfx layers

Fix timestamp handling.
Attachment #8608985 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
Created attachment 8609367 [details] [diff] [review]
patch part 2 - Change t gfx layers

Update nits.
Attachment #8609357 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b55fa2d62ad3
(Assignee)

Updated

2 years ago
Blocks: 1144103
(Assignee)

Comment 19

2 years ago
Created attachment 8609404 [details] [diff] [review]
patch part 1 - Change to Widget

Update a comment.
Attachment #8608927 - Attachment is obsolete: true
(Assignee)

Comment 20

2 years ago
Created attachment 8609405 [details] [diff] [review]
patch part 1 - Change to Widget

Update nits.
Attachment #8609404 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8609405 - Flags: review?(mwu)
(Assignee)

Updated

2 years ago
Attachment #8609367 - Flags: review?(mchang)
Comment on attachment 8609367 [details] [diff] [review]
patch part 2 - Change t gfx layers

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

From my understanding, it seems like the HwcComposer should render all displays based on the primary display's vsync from comment 12. With the current approach, it looks like we can have multiple compositors going at different times and rates which always call HwcComposer::Render. Each compositor gets it's own scheduler. If the HwcComposer should render all displays based on the primary displays' vsync, can we have one global compositor scheduler that's allocated at the first CompositorParent, and every CompositorParent is ticked based off the one global scheduler? If we do need different compositors going at different times, what would happen if we use one global scheduler?

How does HwcComposer work with multiple displays? Does each display also get it's own widget? If so, they should already be listening to vsync from the primary display and ticking off that.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +747,5 @@
> +      }
> +      AddPrimaryScheduler(scheduler);
> +      scheduler = nullptr;
> +    }
> +    // Create proxy scheduler for CompositorParent

should this be else { ... }? Otherwise it will overwrite the primary scheduler. Should the primary scheduler be both in the primary scheduler + list of schedulers?

@@ +770,5 @@
> +  MOZ_ASSERT(!mPrimaryScheduler);
> +  MOZ_ASSERT(mSchedulers.Length() == 0);
> +
> +  aScheduler->SetManager(this);
> +  mPrimaryScheduler = aScheduler;

Do we ever reset the primary scheduler to something else? Can we set it during initialization and it won't be changed / cleaned up until actual shutudown time?

@@ +809,5 @@
> +
> +  bool didComposite = false;
> +  {
> +    MutexAutoLock lock(mMutex);
> +    // Composite layers

nit: Composite all displays

@@ +823,5 @@
> +    }
> +  }
> +
> +  // Render by Composer2D
> +  nsRefPtr<Composer2D> composer2D;

Why not hold a reference to the composer 2D as part of the CompositorShedulerManager instead of going through the widget?

::: gfx/layers/ipc/CompositorParent.h
@@ +148,5 @@
>    CompositorParent* mCompositorParent;
>    TimeStamp mLastCompose;
>    CancelableTask* mCurrentCompositeTask;
> +  bool mNeedsComposite;
> +  nsIWidget* mWidget;

Is owning a reference to mWidget going to be ok? From the current code, the widget shouldn't really be accessed on the compositor thread. 

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=CompositorParent.cpp&case=true#1434
(Assignee)

Updated

2 years ago
Depends on: 1138287
(Assignee)

Comment 22

2 years ago
Comment on attachment 8609405 [details] [diff] [review]
patch part 1 - Change to Widget

It seems better to update based on Bug 1138287.
Attachment #8609405 - Flags: review?(mwu)
(Assignee)

Comment 23

2 years ago
Comment on attachment 8609367 [details] [diff] [review]
patch part 2 - Change t gfx layers

It is better to be updated based on Bug 1138287.
Attachment #8609367 - Flags: review?(mchang)
(Assignee)

Comment 24

2 years ago
I recognized that I made a big misunderstanding :-( hwc hal api can rendered to multiple displays by one function call. And SurfaceFlinger rendered all display's rendering once. But the codeaurora's hwc hal's internal implementation handle each display's rendering independently. Therefore, we seen not need to remove Remove hwc buffer swap from LayerManagerComposite::Render().

So, it seems better to set this bug to invalid until necessary again.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INVALID
(Assignee)

Updated

2 years ago
Blocks: 1225402
(Assignee)

Updated

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