Bug: Times passed to AnimationTimer should not fluctuate

[John Hendrikx]

It's a constant value for all handler invocations for a given frame, I 
think that part is correct.  It should not matter that a later handler 
is doing its calculations for the next frame at a different point in 
real time; it would still be doing calculations for the next frame, and 
as long as it completes those before the next frame is rendered it will 
be "on time".

So if I have two handlers, A and B, and A is called at (real) time X, 
and B is called at (real) time X + 200 ns, they both should get a "now" 
value that corresponds to the next frame to be rendered -- this doesn't 
have to be derived from System.nanoTime at all.  It only needs to start 
at some value and be incremented for each frame rendered (including 
missed frames).  AnimationTimer is not documented to say that the value 
is derived from nanoTime -- it is just happenstance that the value 
currently used corresponds closely to System.nanoTime.

--John

On 29/08/2024 20:53, Andy Goryachev wrote:
>
> This might be slightly off-topic, but I noticed that instead of 
> computing 'now' time for AnimationTimer.handle(long) just before 
> calling the actual handler, we do compute it once before sending to 
> possibly multiple handlers.  Each subsequent invocation produces 
> progressively larger discrepancy (AbstractPrimaryTimer:264).
>
> Does not explain the issue John raised though.
>
> -andy
>
> *From: *openjfx-dev <openjfx-dev-retn at openjdk.org> on behalf of 
> Michael Strauß <michaelstrau2 at gmail.com>
> *Date: *Thursday, August 29, 2024 at 11:45
> *To: *
> *Cc: *openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
> *Subject: *Re: Bug: Times passed to AnimationTimer should not fluctuate
>
> Yes, that makes sense. In any case, we shouldn't be using a system
> timer, but simply record the timestamp at v-sync, and then pass this
> precise timestamp to all AnimationTimers. It shouldn't matter when
> AnimationTimers are invoked between frames, as long as the timestamp
> corresponds to the v-sync signal. (Well, unless the timer callback
> measures its own time, which it shouldn't do.)
>
>
> On Thu, Aug 29, 2024 at 8:20 PM John Hendrikx 
> <john.hendrikx at gmail.com> wrote:
> >
> > I think they're a bit separate.  Even with VSync, the time it takes 
> to kick the FX thread in action is still going to be between 0-30ms.  
> If it then passes `System.nanoTime()` to the AnimationRunnables, 
> you're basically saying that they should render a frame at the precise 
> time of VSync-Time + random time it took to schedule the FX thread... 
> suffice to say that the extra accuracy of the more accurate VSync 
> timer (just like my far more accurate timer) is made completely 
> redundant by the jitter introduced by the scheduler.
> >
> > This brings me back to my original point: we should not be passing 
> `System.nanoTime()` to AnimationRunnables.  Passing 
> `System.nanoTime()` is basically asking to create a frame with a time 
> index that will NEVER be rendered, so why are we asking Animations to 
> use this value for calculating animation locations/offsets/sizes ?
> >
> > This problem is also present on Mac and Linux, just less noticeable 
> because their schedulers generally react within 0-2 ms (vs 0-30 ms on 
> Windows).  2 ms is "close enough" to the most commonly used frame 
> rates (60 fps, at 16.667 ms per frame), but on Windows it can 
> practically be a two frame difference.
> >
> > Even in the absence of V-sync, when JavaFX arbitrarily picks 60 Hz 
> as its refresh frequency, the times passed to AnimationTimer should be 
> multiples of 16.667 ms, not 16.667 ms + however long it took to wake 
> up the FX thread.  In other words this code in AbstactPrimaryTimer:
> >
> > private long nextPulseTime = nanos();
> >
> > private long lastPulseDuration = Integer.MIN_VALUE;
> >
> > @Override
> >
> > public void run() {
> >
> > if (paused) {
> >
> > return;
> >
> > }
> >
> > final long now = nanos();
> >
> > recordStart((nextPulseTime - now) / 1000000);
> >
> > timePulseImpl(now);
> >
> > recordEnd();
> >
> > updateNextPulseTime(now);
> >
> > // reschedule animation runnable if needed
> >
> > updateAnimationRunnable();
> >
> > }
> >
> > ...would be far better if it passed "nextPulseTime" to 
> `timePulseImpl` (which eventually calls the AnimationRunnables) 
> instead of "now".
> >
> > Note: this is assuming the adaptive pulse flag is disabled.  If it 
> is enabled, nextPulseTime won't be a nice multiple of the frame rate 
> -- so when this is enabled we may want to round it up/down before 
> passing it to the AnimationRunnables.
> >
> > Note 2: you can **already** achieve far smoother animation even on 
> Windows by rounding the value you get passed in to a multiple of 
> 1/frameRate. This only works when you have access to the this time. It 
> won't solve Timeline calculations -- they will still calculate 
> positions and values for frames that will never exist, subject to FX 
> thread scheduling jitter...
> >
> > --John
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240830/ab6dbb2a/attachment-0001.htm>