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

Ensure Software Vsync TimeStamp is never in the future

RESOLVED FIXED in Firefox 38, Firefox OS v2.2

Status

()

Core
Graphics
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

unspecified
mozilla38
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
The SoftwareVsyncSource schedules vsyncs in 16.6 ms increments. However, the base::thread post delayed tasks only posts tasks in integer delays. This can cause the issue where the VsyncTimestamp in the NotifyVsync method is in the future. e.g. the task executed 16ms from the scheduled time, but the Vsynctimestamp is 0.6ms ahead of TimeStamp::Now(). Fix the timestamp to be at now for much the same logic as https://bugzilla.mozilla.org/show_bug.cgi?id=1119850#c4.
(Assignee)

Comment 1

3 years ago
Created attachment 8548331 [details] [diff] [review]
Ensure Software VsyncTimestamp is not in the future
Attachment #8548331 - Flags: review?(bugmail.mozilla)
Comment on attachment 8548331 [details] [diff] [review]
Ensure Software VsyncTimestamp is not in the future

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

::: gfx/thebes/SoftwareVsyncSource.cpp
@@ +85,5 @@
> +  // whereas TimeDurations can have floating point delays.
> +  // Thus the vsync timestamp can be in the future, which large parts
> +  // of the system can't handle, including animations. Force the timestamp to be now.
> +  if (aVsyncTimestamp > now) {
> +    aVsyncTimestamp = now;

I think that the value you pass in to Display::NotifyVsync should be this adjusted value, but the value you pass in to ScheduleNextVsync should still be the original value. Otherwise you're effectively doing 16ms vsync intervals every time, which will drift from the ideal over time. If you pass the original aVsyncTimestamp in to ScheduleNextVsync you'll maintain a long-term average of 16.6ms intervals. Does that sound reasonable?
(Assignee)

Comment 3

3 years ago
Created attachment 8548404 [details] [diff] [review]
Ensure Software VsyncTimestamp is not in the future

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> Comment on attachment 8548331 [details] [diff] [review]
> Ensure Software VsyncTimestamp is not in the future
> 
> Review of attachment 8548331 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/SoftwareVsyncSource.cpp
> @@ +85,5 @@
> > +  // whereas TimeDurations can have floating point delays.
> > +  // Thus the vsync timestamp can be in the future, which large parts
> > +  // of the system can't handle, including animations. Force the timestamp to be now.
> > +  if (aVsyncTimestamp > now) {
> > +    aVsyncTimestamp = now;
> 
> I think that the value you pass in to Display::NotifyVsync should be this
> adjusted value, but the value you pass in to ScheduleNextVsync should still
> be the original value. Otherwise you're effectively doing 16ms vsync
> intervals every time, which will drift from the ideal over time. If you pass
> the original aVsyncTimestamp in to ScheduleNextVsync you'll maintain a
> long-term average of 16.6ms intervals. Does that sound reasonable?

Yeah that makes sense. Updated to do this.
Attachment #8548331 - Attachment is obsolete: true
Attachment #8548331 - Flags: review?(bugmail.mozilla)
Attachment #8548404 - Flags: review?(bugmail.mozilla)
Comment on attachment 8548404 [details] [diff] [review]
Ensure Software VsyncTimestamp is not in the future

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

Thanks, lgtm.
Attachment #8548404 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c9c9c240b07
https://hg.mozilla.org/mozilla-central/rev/0c9c9c240b07
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Comment 7

3 years ago
Comment on attachment 8548404 [details] [diff] [review]
Ensure Software VsyncTimestamp is not in the future

Approval Request Comment
[Feature/regressing bug #]: Silk, bug 987532
[User impact if declined]: None at the moment, some later. This patch is used by the silk test infrastructure, which also has yet to land in master but will need uplift approval. If we enable silk by default, we'd like to have the silk tests run on gecko 37 as well.
[Describe test coverage new/current, TBPL]: I manually tested this patch to ensure Software vsync works with gtests. The software vsync should only be used on the b2g emulators with the silk tests. This patch is currently prefed off, so the code path is not executed on current treeherder tests. 
[Risks and why]: Low Risk - This patch is used only for silk gtests, which are currently not running. This code shouldn't be executing on master or gecko37 until we enable silk gtests by default.
[String/UUID change made/needed]: None
Attachment #8548404 - Flags: approval-mozilla-aurora?
Comment on attachment 8548404 [details] [diff] [review]
Ensure Software VsyncTimestamp is not in the future

As this is B2G specific, I'm marking as Aurora- and setting the b2g37 approval. Please see comment 7 for the approval request.
Attachment #8548404 - Flags: approval-mozilla-b2g37?
Attachment #8548404 - Flags: approval-mozilla-aurora?
Attachment #8548404 - Flags: approval-mozilla-aurora-

Updated

3 years ago
Attachment #8548404 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/80c7b6c14033
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
status-firefox36: --- → wontfix
status-firefox37: --- → wontfix
status-firefox38: --- → fixed
You need to log in before you can comment on or make changes to this bug.