Closed Bug 1121065 Opened 9 years ago Closed 9 years ago

Ensure Software Vsync TimeStamp is never in the future

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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?
(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+
https://hg.mozilla.org/mozilla-central/rev/0c9c9c240b07
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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-
Attachment #8548404 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: