1317893 - Add CompositorVsyncScheduler to WebRenderBridgeParent

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Sotaro Ikeda [:sotaro]]

To update video frames rendering correctly, we need vsync scheduler based composition.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8811149 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

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

This looks ok to me. I'd prefer if a WebRenderBridgeParent didn't have to hold a ref to its "parent" - hopefully we can remove that and just have a ref to the CompositorOGL which can do some of this. But this is fine for now.

Comment 3

[Sotaro Ikeda [:sotaro]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Comment on attachment 8811149 [details] [diff] [review]
> patch - Add CompositorVsyncScheduler to WebRenderBridgeParent
> hopefully we can remove that and just have a
> ref to the CompositorOGL which can do some of this. But this is fine for now.

Yes, we could remove the ref by Bug 1317935.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Rebased.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8811706 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

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

::: gfx/layers/ipc/CompositorBridgeParent.h
@@ +83,5 @@
>    uint64_t mLayersId;
>  };
>  
> +
> +class CompositorVsyncSchedulerOwner

Do you mind moving this into its own header, gfx/layers/CompositorVsyncScheduleOwner.h? I don't like having to include all of CompositorBridgeParent.h just to get this one small thing. And we have too many files that are too big already.

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +148,2 @@
>    wr_dp_end(mWRWindowState, mWRState);
> +  ScheduleComposition();

Morris is touching some of this code in bug 1316903, so make sure you coordinate with him when landing.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Apply the comment.

Comment 10

[Sotaro Ikeda [:sotaro]]

attachment 8812090 [details] [diff] [review] used the commands.
  > hg cp gfx/layers/ipc/CompositorBridgeParent.h gfx/layers/ipc/CompositorVsyncScheduler.h
  > hg cp gfx/layers/ipc/CompositorBridgeParent.cpp gfx/layers/ipc/CompositorVsyncScheduler.cpp

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Rebased.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/b1542ac3fc70
Add CompositorVsyncScheduler to WebRenderBridgeParent r=kats

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch for m-c - Add CompositorVsyncScheduler to WebRenderBridgeParent

Comment 14

[Sotaro Ikeda [:sotaro]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92a5323c313c
Split CompositorVsyncScheduler to own file r=kats

Comment 16

[Jerry Shih[:jerry] (UTC+8) (inactive)]

The CompositorVsyncScheduler changes will hit an assert when I build with WR at mac.

Assert:
https://hg.mozilla.org/projects/graphics/annotate/b1542ac3fc70/gfx/layers/ipc/CompositorVsyncScheduler.cpp#l85

So, I revert your changes. 
https://hg.mozilla.org/projects/graphics/rev/c85e2d8dc8df

Comment 17

[Kartikaya Gupta (email:[e-mail])]

(In reply to Pulsebot from comment #15)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/92a5323c313c
> Split CompositorVsyncScheduler to own file r=kats

I wanted to move CompositorVsyncScheduleOwner, not CompositorVsyncScheduler. But this is good too, it belongs in a separate file. It would have been nice to split this commit into two - one that just extracts CompositorVsyncScheduler, and another that adds the Owner. And since you landed on inbound, don't try to reland it on graphics, we'll get this on the next merge from m-c.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92a5323c313c

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Sotaro, please do the follow-up work in another bug. I'm going to do a merge from m-c over to graphics which picks up this change.

Comment 20

[Sotaro Ikeda [:sotaro]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> (In reply to Pulsebot from comment #15)
> > Pushed by [e-mail]:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/92a5323c313c
> > Split CompositorVsyncScheduler to own file r=kats
> 
> I wanted to move CompositorVsyncScheduleOwner, not CompositorVsyncScheduler.
> But this is good too, it belongs in a separate file. It would have been nice
> to split this commit into two - one that just extracts
> CompositorVsyncScheduler, and another that adds the Owner. And since you
> landed on inbound, don't try to reland it on graphics, we'll get this on the
> next merge from m-c.

Thanks, but 1318780 is created to split CompositorVsyncScheduleOwner.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Can we leave this bug closed, since a patch landed on m-c here? We can file another bug for the Webrender code changes.

Comment 22

[Sotaro Ikeda [:sotaro]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #16)
> The CompositorVsyncScheduler changes will hit an assert when I build with WR
> at mac.
> 
> Assert:
> https://hg.mozilla.org/projects/graphics/annotate/b1542ac3fc70/gfx/layers/
> ipc/CompositorVsyncScheduler.cpp#l85
> 
> So, I revert your changes. 
> https://hg.mozilla.org/projects/graphics/rev/c85e2d8dc8df

Sorry, I will address the problem.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Rebased.

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8812647 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

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

::: gfx/layers/ipc/CompositorVsyncScheduler.cpp
@@ +83,5 @@
>    , mSetNeedsCompositeMonitor("SetNeedsCompositeMonitor")
>    , mSetNeedsCompositeTask(nullptr)
>  {
> +  MOZ_ASSERT(NS_IsMainThread() || XRE_GetProcessType() == GeckoProcessType_GPU ||
> +             CompositorThreadHolder::IsInCompositorThread());

Is there any point to having this assert any more then? It allows basically anything.

Comment 26

[Sotaro Ikeda [:sotaro]]

(In reply to Kartikaya Gupta away[Nov24,Dec5) (email:[e-mail]) from comment #25)
> Is there any point to having this assert any more then? It allows basically
> anything.

There is no point to keep the assert. I am going to remove the assert.

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Addressed the comment.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/998a69cbaf54
Add CompositorVsyncScheduler to WebRenderBridgeParent r=kats