JDK-8210547 synchronise with OpenGL after the last window is presented

[notzed]

As discussed on the mailing list.

OpenGL requires an explicit glFinish() to synchronise with the video refresh. The current usage is incorrect and against the specification. This patch calls glFinish() after all windows have been drawn following a pulse.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8210547

Warning

 ⚠️ Found leading lowercase letter in issue title for 8210547: synchronise with OpenGL after the last window is presented

Issue

• JDK-8210547: [Linux] Uncontrolled framerate (Bug - P5) ⚠️ Title mismatch between PR and JBS.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1981/head:pull/1981
$ git checkout pull/1981

Update a local copy of the PR:
$ git checkout pull/1981
$ git pull https://git.openjdk.org/jfx.git pull/1981/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1981

View PR using the GUI difftool:
$ git pr show -t 1981

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1981.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back notzed! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 01: Full - Incremental (293c5712)
• 00: Full (34a06fdc)

[kevinrushforth]

This will need careful review.

Reviewers: At least two of: @lukostyra @arapte @kevinrushforth

/reviewers 2

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[kevinrushforth]

@notzed Please enable GHA testing for your repo. Go you your "Actions" tab in your repo and enable them with the green "enable workflows" button. Then push an empty commit to trigger a test run.

[kevinrushforth]

@notzed As with the fix @arapte did for PR #1978, you will need to qualify this with PrismSettings.isVsyncEnabled.

My quick take on your PR is that the changes are pretty intrusive and not limited to the ES2 pipeline. I recommend exploring whether the changes can be more targeted.

[openjdk]

@notzed Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[notzed]

prism.vsync=false behaves visually the same as it currently does because double buffering isn't enabled via glXSwapIntervalSGI(). i.e it runs at roughly 62.5fps by default. One caveat is that glFinish() will pause the CPU to wait for the GPU and that may have throughput impacts, or it may be desirable to avoid over-buffering.

glFinish() is the only way to sync the cpu with the video card[1] and honour the expectations of prism (i.e. that presentation with 'vsync supported' is throttled by the frame-rate) in the same way directx is. And the only place it makes sense is immediately after glSwapBuffers(). But it can't be done per-window otherwise all windows are displayed round-robin with the frame rate.

The problematic point is the QuantumTooklit.vsyncHint() call which itself causes unthrottled pulses - they are only throttled in practice by Presentable.present(). vsyncHint() also seems to not be used anywhere to actually indicate vsync, it just fires another pulse after rendering is finish which just upsets the pulse regularity.

I tried a number of variations over a couple of weeks and this was by far the simplest I could come up with but i'm open to suggestions.

Note [1] also suggests a 'high precision timer' should be used for frame timing instead of vsync, and gdk_timeout used by the x11 backend is definitely not suitable for this purpose, even a simple ScheduledExecutorService (as used by monocle and headless) is far more accurate. But discovering accurate frame-rates is problematic (they should be rational but the gtk api's don't support that, and gtk4 and wayland has it's whole own idea of animation timing) and they can change at runtime or even across screens. I do have another patch to enable nanosecond (or it could be microsecond) frame times and a floating point animation rate ... but that is far far more intrusive.

[1] https://wikis.khronos.org/opengl/Swap_Interval#GPU_vs_CPU_synchronization

[lukostyra]

I tested your changes but they do not fix the original problem which #1929 addresses. I just tested this on my Intel UHD machine and the framerate is not tied to VSync (FPS meter from the original issue is around 1000fps instead of 60fps).

I think it's getting pretty confusing regarding both this change and Thiago's PR, so I would like to figure some things out before we progress. I looked back into the mailing list discussion but I'm still not fully clear on everything.

Is this change actually supposed to address the problem from JDK-8210547? If so, is this change in place of #1929 or does it depend on it?

[notzed]

This is for JDK-8210547.

1929 is separate, it doesn't work for me when you have more than 1 window open. It's effectively just the same as calling glFinish() on every glxSwapBuffers() which just tanks the framerate (and for me whether it uses *SGI or *EXT makes no difference).

I suppose I could see if using glXSwapIntervalEXT() makes a difference here.

FWIW this is what i'm using to test, it intentionally advances on animation pulses rather than using the the timestamp to visualise spurious pulses. With javafx-25 it goes mental but with the given patch it runs at the right rate but with spurious pulses from the vsyncHint stuff.

BEWARE! It uses Robot to take over the mouse, ESC/Q or alt-f4 quits.

Latency.java

[lukostyra]

I suppose I could see if using glXSwapIntervalEXT() makes a difference here.

It might be better to switch to glXSwapIntervalEXT() then. It seems to be more consistent across all platforms (ie. works on Intel where the others don't for some reason). I'll be waiting for your updates, and I think we can close #1929 then. Thanks for attaching the test program too, it'll come in handy.

Even though these changes are simple, they do go quite deep so we'll need to spend some extra time reviewing and testing them.

A couple of formalities as well:

• PR title must begin with issue number without JDK- and followed by a colon
• Text following the issue number must be the same as the JBS issue counterpart

So in this case, PR should be named exactly like #1929 for Skara to properly connect it.

Last but not least, when you update this branch please don't use git rebase and force pushes, as it breaks the review tool and makes it harder for us to iteratively review your changes. If you ever need to catch up to master or to resolve any conflicts, use git merge instead.

[notzed]

Sorry I made a mistake with the push, Kevin directed me to send an "empty commit" to trigger automation and I couldn't work out how to do that otherwise at the time - until just after I'd done it and it was too late then.

I'll look at merging in bits of 1929, sans the visual stuff which seems unrelated.