If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add CompositorVsyncScheduler to WebRenderBridgeParent

RESOLVED FIXED in Firefox 53

Status

()

Core
Graphics: WebRender
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

(Blocks: 1 bug)

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 10 obsolete attachments)

15.02 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 months ago
To update video frames rendering correctly, we need vsync scheduler based composition.
(Assignee)

Updated

10 months ago
Assignee: nobody → sotaro.ikeda.g
Version: 38 Branch → Trunk
(Assignee)

Updated

10 months ago
Depends on: 1312316
(Assignee)

Comment 1

10 months ago
Created attachment 8811149 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent
(Assignee)

Updated

10 months ago
Attachment #8811149 - Flags: feedback?(mchang)
Attachment #8811149 - Flags: feedback?(bugmail)
(Assignee)

Updated

10 months ago
Blocks: 1311790
(Assignee)

Updated

10 months ago
Blocks: 1317935
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.
Attachment #8811149 - Flags: feedback?(bugmail) → feedback+
(Assignee)

Comment 3

10 months ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Assignee)

Comment 4

10 months ago
Created attachment 8811582 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent
Attachment #8811149 - Attachment is obsolete: true
Attachment #8811149 - Flags: feedback?(mchang)
(Assignee)

Comment 5

10 months ago
Created attachment 8811583 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent
Attachment #8811582 - Attachment is obsolete: true
(Assignee)

Comment 6

10 months ago
Created attachment 8811584 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent
Attachment #8811583 - Attachment is obsolete: true
(Assignee)

Comment 7

10 months ago
Created attachment 8811706 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Rebased.
Attachment #8811584 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8811706 - Flags: review?(bugmail)
Attachment #8811706 - Flags: feedback+
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.
Attachment #8811706 - Flags: review?(bugmail) → review+
(Assignee)

Updated

10 months ago
Depends on: 1316903
(Assignee)

Comment 9

10 months ago
Created attachment 8812090 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Apply the comment.
Attachment #8811706 - Attachment is obsolete: true
Attachment #8812090 - Flags: review+
(Assignee)

Comment 10

10 months ago
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
(Assignee)

Comment 11

10 months ago
Created attachment 8812095 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Rebased.
Attachment #8812090 - Attachment is obsolete: true
Attachment #8812095 - Flags: review+

Comment 12

10 months ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/b1542ac3fc70
Add CompositorVsyncScheduler to WebRenderBridgeParent r=kats
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
(Assignee)

Updated

10 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

10 months ago
Created attachment 8812103 [details] [diff] [review]
patch for m-c - Add CompositorVsyncScheduler to WebRenderBridgeParent
Attachment #8812103 - Flags: review+
(Assignee)

Comment 14

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

Comment 15

10 months ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92a5323c313c
Split CompositorVsyncScheduler to own file r=kats
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
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Pulsebot from comment #15)
> Pushed by sikeda@mozilla.com:
> 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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/92a5323c313c
Status: REOPENED → RESOLVED
Last Resolved: 10 months ago10 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.
(Assignee)

Updated

10 months ago
Status: RESOLVED → REOPENED
Flags: needinfo?(sotaro.ikeda.g)
Resolution: FIXED → ---
(Assignee)

Updated

10 months ago
Depends on: 1318780
(Assignee)

Comment 20

10 months ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> (In reply to Pulsebot from comment #15)
> > Pushed by sikeda@mozilla.com:
> > 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.
Can we leave this bug closed, since a patch landed on m-c here? We can file another bug for the Webrender code changes.
(Assignee)

Updated

10 months ago
Attachment #8812103 - Attachment is obsolete: true
(Assignee)

Comment 22

10 months ago
(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.
(Assignee)

Comment 23

10 months ago
Created attachment 8812644 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Rebased.
Attachment #8812095 - Attachment is obsolete: true
Attachment #8812644 - Flags: review+
(Assignee)

Comment 24

10 months ago
Created attachment 8812647 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent
Attachment #8812644 - Attachment is obsolete: true
Attachment #8812647 - Flags: review+
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.
(Assignee)

Comment 26

10 months ago
(In reply to Kartikaya Gupta away[Nov24,Dec5) (email:kats@mozilla.com) 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.
(Assignee)

Comment 27

10 months ago
Created attachment 8812991 [details] [diff] [review]
patch - Add CompositorVsyncScheduler to WebRenderBridgeParent

Addressed the comment.
Attachment #8812647 - Attachment is obsolete: true
Attachment #8812991 - Flags: review+

Comment 28

10 months ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/998a69cbaf54
Add CompositorVsyncScheduler to WebRenderBridgeParent r=kats
Status: REOPENED → RESOLVED
Last Resolved: 10 months ago10 months ago
Resolution: --- → FIXED
Depends on: 1319510
You need to log in before you can comment on or make changes to this bug.