Closed Bug 1157441 Opened 7 years ago Closed 7 years ago

Remove hwc buffer swap from LayerManagerComposite::Render()

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID
Tracking Status
firefox40 --- affected

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(2 files, 16 obsolete files)

30.96 KB, patch
Details | Diff | Splinter Review
8.83 KB, patch
Details | Diff | Splinter Review
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: nobody → sotaro.ikeda.g
Attached patch wip patch (obsolete) — Splinter Review
The patch is created based on Bug 1156981 patch.
Blocks: 1116089
Attached patch wip patch (obsolete) — Splinter Review
Fix ICS problem.
Attachment #8596172 - Attachment is obsolete: true
Attachment #8596578 - Attachment is obsolete: true
Attachment #8597421 - Attachment is obsolete: true
Fix nits.
Attachment #8597423 - Attachment is obsolete: true
Update nits.
Attachment #8597426 - Attachment is obsolete: true
unbitrot.
Attachment #8597430 - Attachment is obsolete: true
Attachment #8607711 - Attachment is obsolete: true
Update nits.
Attachment #8608362 - Attachment is obsolete: true
Fix some problems. The patch seems to work on master flame-kk.
Attachment #8608722 - Attachment is obsolete: true
Attached patch patch part 1 - Change to Widget (obsolete) — Splinter Review
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
Attachment #8608927 - Attachment description: patch - Change to Widget → patch part 1 - Change to Widget
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.
Correct patch.
Attachment #8608930 - Attachment is obsolete: true
Update nits.
Attachment #8608933 - Attachment is obsolete: true
Fix timestamp handling.
Attachment #8608985 - Attachment is obsolete: true
Update nits.
Attachment #8609357 - Attachment is obsolete: true
Blocks: 1144103
Attached patch patch part 1 - Change to Widget (obsolete) — Splinter Review
Update a comment.
Attachment #8608927 - Attachment is obsolete: true
Update nits.
Attachment #8609404 - Attachment is obsolete: true
Attachment #8609405 - Flags: review?(mwu)
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
Depends on: 1138287
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)
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)
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
Closed: 7 years ago
Resolution: --- → INVALID
Blocks: 1225402
No longer blocks: 1225402
You need to log in before you can comment on or make changes to this bug.