1078184 - Investigate changing the throttling mechanism in the refresh driver from DidComposite ipc message with silk

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

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

When the content receive the DidComposite(), it will call refresh driver Tick().
In project silk, we should change this behavior to schedule the tick task at next vsync event.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Do we still need this Jerry? I don't think we have to do this, any particular reason we should change this behavior? I think some tests rely on it. Otherwise, please resolve this bug.

Comment 2

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

In https://bugzilla.mozilla.org/show_bug.cgi?id=1094760#c4
If rd enter throttling state, it will send another layer update to compositor in DidComposite().
Then compositor thread will still fill with rendering task and rd will be always in throttling mode.
If compositor can defer the rendering task to next vsync, we don't need this bug.

Comment 3

[Mason Chang [Inactive] [:mchang]]

If I understand this problem correctly, we essentially always do this from bug 854421:

1) Tick the refresh driver
2) After a composite, send the DidComposite with the corresponding transaction id back to the refresh driver
3) If the refresh driver keeps ticking without receiving a DidComposite with the appropriate transaction id, we freeze the refresh driver and prevent normal ticks from occurring.
4) When the compositor recovers and sends the DidComposite, the refresh driver will tick immediately rather than wait for the next timer activated tick. 
5) We hopefully go back to the normal timer activated refresh driver tick rates.

In the normal case, composites finish in time so the refresh driver can always tick. This essentially throttles the refresh driver to the composite rate.

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #2)
> In https://bugzilla.mozilla.org/show_bug.cgi?id=1094760#c4
> If rd enter throttling state, it will send another layer update to
> compositor in DidComposite().
> Then compositor thread will still fill with rendering task and rd will be
> always in throttling mode.
> If compositor can defer the rendering task to next vsync, we don't need this
> bug.

I don't think we do this. I think the Refresh Driver stops producing tasks so the Compositor can catch up. Once the Compositor has caught up, we will no longer be in throttling mode.

But I do think when we recover, we can wait until the next vsync interval to tick the refresh drivers rather than doing it immediately. From https://bugzilla.mozilla.org/show_bug.cgi?id=854421#c10.

@mattwoodrow - What do you think about waiting for the next vsync to tick the refresh drivers rather than ticking them immediately after a DidComposite() in the cases where the refresh driver is waiting on a transaction? I worry that we could get in situations that we tick the refresh drivers right away and never recover back to ticking on vsync intervals. But I guess even if the tick was kind of long like 20 ms, we would still keep ticking the refresh drivers at the normal timer intervals as the refresh driver can get 1 frame ahead. Is this right? Thanks!

Comment 4

[Matt Woodrow (:mattwoodrow)]

(In reply to Mason Chang [:mchang] from comment #3)
> If I understand this problem correctly, we essentially always do this from
> bug 854421:
> 
> 1) Tick the refresh driver
> 2) After a composite, send the DidComposite with the corresponding
> transaction id back to the refresh driver
> 3) If the refresh driver keeps ticking without receiving a DidComposite with
> the appropriate transaction id, we freeze the refresh driver and prevent
> normal ticks from occurring.
> 4) When the compositor recovers and sends the DidComposite, the refresh
> driver will tick immediately rather than wait for the next timer activated
> tick. 
> 5) We hopefully go back to the normal timer activated refresh driver tick
> rates.
> 
> In the normal case, composites finish in time so the refresh driver can
> always tick. This essentially throttles the refresh driver to the composite
> rate.

Yes, that's correct. Though note that we let the refresh driver get one frame ahead of the compositor (such that both are working in parallel).

> 
> (In reply to Jerry Shih[:jerry] (UTC+8) from comment #2)
> > In https://bugzilla.mozilla.org/show_bug.cgi?id=1094760#c4
> > If rd enter throttling state, it will send another layer update to
> > compositor in DidComposite().
> > Then compositor thread will still fill with rendering task and rd will be
> > always in throttling mode.
> > If compositor can defer the rendering task to next vsync, we don't need this
> > bug.
> 
> I don't think we do this. I think the Refresh Driver stops producing tasks
> so the Compositor can catch up. Once the Compositor has caught up, we will
> no longer be in throttling mode.
> 
> But I do think when we recover, we can wait until the next vsync interval to
> tick the refresh drivers rather than doing it immediately. From
> https://bugzilla.mozilla.org/show_bug.cgi?id=854421#c10.
> 
> @mattwoodrow - What do you think about waiting for the next vsync to tick
> the refresh drivers rather than ticking them immediately after a
> DidComposite() in the cases where the refresh driver is waiting on a
> transaction? I worry that we could get in situations that we tick the
> refresh drivers right away and never recover back to ticking on vsync
> intervals. But I guess even if the tick was kind of long like 20 ms, we
> would still keep ticking the refresh drivers at the normal timer intervals
> as the refresh driver can get 1 frame ahead. Is this right? Thanks!

Yeah, that sounds reasonable. The downside is that we might miss one frame (if it was going to be quick we might have got it done and in before vblank), but I doubt that matters much since we've already missed our 60fps targets if we're throttling.

Comment 5

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

Attachment: pending DidComposite() tick to next vsync(png file)

Comment 6

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

Attachment: Gecko profiler link for original DidComposite() tick

Comment 7

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

I think we might just use the original DidComposite() method.

If we suffer the problem as bug 1094760(compositor sleeps for a long time for hwc, and wake up near next vsync), refresh driver will enter throttle mode. Even though each refresh driver tick is not aligned with vsync, but compositor sends the DidComposite() message just near the next vsync event. So refresh driver still receive uniform message for tick.

Here is the content requestAnimationFrame test with the problem in bug 1094760. Please check attachment 8565309 [details]. We can see compositor spends about 16.6 ms for one frame in these region. And content refresh driver tick itself just after compositor sends the DidComposite().

If we pending the DidComposite() tick to next vsync, the composition time will usually recover soon. And it will cost about 6~8ms per frame and refresh driver will exit the throttle mode. In this case, refresh driver might pending some frames and go back to normal back and forth, then the animation becomes non-uniform. Please check attachment 8565307 [details].

Comment 8

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

We still can't know the root cause for bug 1094760. But if the compositor always wakes up near next vsync, we can just tick the refresh driver by the DidComposite() message from compositor.

Comment 9

[Mason Chang [Inactive] [:mchang]]

From comment 7, resolving as WONTFIX.