1157441 - Remove hwc buffer swap from LayerManagerComposite::Render()

Status: RESOLVED INVALID

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

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().

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

The patch is created based on Bug 1156981 patch.

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Fix ICS problem.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove hwc buffer swap from LayerManagerComposite::Render()

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove hwc buffer swap from LayerManagerComposite::Render()

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove hwc buffer swap from LayerManagerComposite::Render()

Fix nits.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove hwc buffer swap from LayerManagerComposite::Render()

Update nits.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove hwc buffer swap from LayerManagerComposite::Render()

unbitrot.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove hwc buffer swap from LayerManagerComposite::Render()

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove hwc buffer swap from LayerManagerComposite::Render()

Update nits.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove hwc buffer swap from LayerManagerComposite::Render()

Fix some problems. The patch seems to work on master flame-kk.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: 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.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: 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.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change t gfx layers

Correct patch.

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change t gfx layers

Update nits.

Comment 15

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd7d7ab7a349

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change t gfx layers

Fix timestamp handling.

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change t gfx layers

Update nits.

Comment 18

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b55fa2d62ad3

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Change to Widget

Update a comment.

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Change to Widget

Update nits.

Comment 21

[Mason Chang [Inactive] [: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

Comment 22

[Sotaro Ikeda [:sotaro]]

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

It seems better to update based on Bug 1138287.

Comment 23

[Sotaro Ikeda [:sotaro]]

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

It is better to be updated based on Bug 1138287.

Comment 24

[Sotaro Ikeda [:sotaro]]

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.