Skip to content

Call glFlush after eglCreateSync, not before #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kbrenneman wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Call glFlush after eglCreateSync, not before #15

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/x11/x11-window.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1611,15 +1611,15 @@ static EGLBoolean SyncRendering(EplDisplay *pdpy, EplSurface *surf, X11ColorBuff
return EGL_TRUE;
}

pwin->inst->platform->priv->egl.Flush();

sync = pwin->inst->platform->priv->egl.CreateSync(pwin->inst->internal_display->edpy,
EGL_SYNC_NATIVE_FENCE_ANDROID, NULL);
if (sync == EGL_NO_SYNC)
{
goto done;
}

pwin->inst->platform->priv->egl.Flush();
Copy link
Collaborator

@cubanismo cubanismo Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite what the spec claims, this isn't actually how our implementation behaves. It flushes internally during CreateSync() and attaches the fence created by that flush operation. I think this was and is still just perturbing some timing/race if it affects anything at all.

Copy link
Collaborator Author

@kbrenneman kbrenneman Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's possible -- I'm still trying to make sense of that myself.

But at the very least, this is the one sequence that the spec requires to work, so even if there's a driver bug lurking, this should continue to work once it's fixed, right?

Copy link
Collaborator

@cubanismo cubanismo Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that it won't make things worse, yes. I just worry there's a worse bug somewhere that this continues to paper over. This might actually break the papering over, in that the after-create flush will probably get reduced to a no-op by the driver logic.

Copy link
Collaborator Author

@kbrenneman kbrenneman Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thus far, I haven't managed to get an incorrect result, short of trying to draw a frame that takes long enough to cause timeouts (in which case the result is identical to the current code).

The fact that eglQuerySync without a glFlush gets signaled at the wrong point does suggest a driver bug, but matching the spec seems like the best hedge against whatever changes in driver behavior might happen in the future.

Strictly speaking, with the current code in egl-x11, the EGLSync isn't even required to be signaled at all.


syncFd = pwin->inst->platform->priv->egl.DupNativeFenceFDANDROID(pwin->inst->internal_display->edpy, sync);
if (syncFd < 0)
{
Expand Down