The open source OpenXR runtime
st/oxr: Refactor wait frame function to avoid setting state before we should
We would leak frame_id and active_wait_frames increment that would cause bad
states where we hadn't fully waited but would allow xrBeginFrame to complete.
Also improve error handling so that on error conditions the semaphore is
properly decremented and the application can call xrWaitFrame again.
This was caused by unreal doing something like this:
* xrBeginFrame <-- Error
* xrWaitFrame
* xrBeginFrame
* xrEndFrame
* Called at the same time:
* xrWaitFrame
* xrBeginFrame <-- Would get state from non-completed xrWaitFrame
+76
-22
+76
-22
src/xrt/state_trackers/oxr/oxr_session.c
+76
-22
src/xrt/state_trackers/oxr/oxr_session.c
···
461
461
return ns_to_ms(monotonic);
462
462
}
463
463
464
+
static XrResult
465
+
do_wait_frame_and_checks(struct oxr_logger *log,
466
+
struct oxr_session *sess,
467
+
int64_t *out_frame_id,
468
+
uint64_t *out_predicted_display_time,
469
+
uint64_t *out_predicted_display_period,
470
+
XrTime *out_converted_time)
471
+
{
472
+
assert(sess->compositor != NULL);
473
+
474
+
int64_t frame_id = -1;
475
+
uint64_t predicted_display_time = 0;
476
+
uint64_t predicted_display_period = 0;
477
+
478
+
xrt_result_t xret = xrt_comp_wait_frame( //
479
+
sess->compositor, // compositor
480
+
&frame_id, // out_frame_id
481
+
&predicted_display_time, // out_predicted_display_time
482
+
&predicted_display_period); // out_predicted_display_period
483
+
OXR_CHECK_XRET(log, sess, xret, xrt_comp_wait_frame);
484
+
485
+
if (frame_id < 0) {
486
+
return oxr_error(log, XR_ERROR_RUNTIME_FAILURE, "Got a negative frame id '%" PRIi64 "'", frame_id);
487
+
}
488
+
489
+
if ((int64_t)predicted_display_time <= 0) {
490
+
return oxr_error(log, XR_ERROR_RUNTIME_FAILURE, "Got a negative display time '%" PRIi64 "'",
491
+
(int64_t)predicted_display_time);
492
+
}
493
+
494
+
XrTime converted_time = time_state_monotonic_to_ts_ns(sess->sys->inst->timekeeping, predicted_display_time);
495
+
if (converted_time <= 0) {
496
+
return oxr_error(log, XR_ERROR_RUNTIME_FAILURE, "Got '%" PRIi64 "' from time_state_monotonic_to_ts_ns",
497
+
converted_time);
498
+
}
499
+
500
+
*out_frame_id = frame_id;
501
+
*out_predicted_display_time = predicted_display_time;
502
+
*out_predicted_display_period = predicted_display_period;
503
+
*out_converted_time = converted_time;
504
+
505
+
return XR_SUCCESS;
506
+
}
507
+
464
508
XrResult
465
509
oxr_session_frame_wait(struct oxr_logger *log, struct oxr_session *sess, XrFrameState *frameState)
466
510
{
···
474
518
return oxr_session_success_result(sess);
475
519
}
476
520
477
-
478
521
if (sess->frame_timing_spew) {
479
522
oxr_log(log, "Called at %8.3fms", ts_ms(sess));
480
523
}
481
524
482
-
// A subsequent xrWaitFrame call must: block until the previous frame
483
-
// has been begun
525
+
/*
526
+
* A subsequent xrWaitFrame call must: block until the previous frame
527
+
* has been begun. It's extremely forbidden to call xrWaitFrame from
528
+
* multiple threads. We do this before so we call predicted after any
529
+
* waiting for xrBeginFrame has happened, for better timing information.
530
+
*/
484
531
os_semaphore_wait(&sess->sem, 0);
485
532
486
-
os_mutex_lock(&sess->active_wait_frames_lock);
487
-
sess->active_wait_frames++;
488
-
os_mutex_unlock(&sess->active_wait_frames_lock);
489
-
490
533
if (sess->frame_timing_spew) {
491
534
oxr_log(log, "Finished waiting for previous frame begin at %8.3fms", ts_ms(sess));
492
535
}
493
536
537
+
int64_t frame_id = -1;
494
538
uint64_t predicted_display_time;
495
539
uint64_t predicted_display_period;
496
-
xrt_result_t xret = xrt_comp_wait_frame( //
497
-
xc, // compositor
498
-
&sess->frame_id.waited, // out_frame_id
540
+
XrTime converted_time;
541
+
542
+
XrResult ret = do_wait_frame_and_checks( //
543
+
log, // log
544
+
sess, // sess
545
+
&frame_id, // out_frame_id
499
546
&predicted_display_time, // out_predicted_display_time
500
-
&predicted_display_period); // out_predicted_display_period
501
-
OXR_CHECK_XRET(log, sess, xret, xrt_comp_wait_frame);
547
+
&predicted_display_period, // out_predicted_display_period
548
+
&converted_time); // out_converted_time
549
+
if (ret != XR_SUCCESS) {
550
+
// On error we need to release the semaphore ourselves as xrBeginFrame won't do it.
551
+
os_semaphore_release(&sess->sem);
502
552
503
-
if ((int64_t)predicted_display_time <= 0) {
504
-
return oxr_error(log, XR_ERROR_RUNTIME_FAILURE, "Got a negative display time '%" PRIi64 "'",
505
-
(int64_t)predicted_display_time);
553
+
// Error already logged.
554
+
return ret;
506
555
}
507
556
557
+
/*
558
+
* We set the frame_id along with the number of active waited frames to
559
+
* avoid races with xrBeginFrame. The function xrBeginFrame will only
560
+
* allow xrWaitFrame to continue from the semaphore above once it has
561
+
* cleared the `sess->frame_id.waited`.
562
+
*/
563
+
os_mutex_lock(&sess->active_wait_frames_lock);
564
+
sess->active_wait_frames++;
565
+
sess->frame_id.waited = frame_id;
566
+
os_mutex_unlock(&sess->active_wait_frames_lock);
567
+
508
568
frameState->shouldRender = should_render(sess->state);
509
569
frameState->predictedDisplayPeriod = predicted_display_period;
510
-
frameState->predictedDisplayTime =
511
-
time_state_monotonic_to_ts_ns(sess->sys->inst->timekeeping, predicted_display_time);
512
-
513
-
if (frameState->predictedDisplayTime <= 0) {
514
-
return oxr_error(log, XR_ERROR_RUNTIME_FAILURE, "Time_state_monotonic_to_ts_ns returned '%" PRIi64 "'",
515
-
frameState->predictedDisplayTime);
516
-
}
570
+
frameState->predictedDisplayTime = converted_time;
517
571
518
572
if (sess->frame_timing_spew) {
519
573
oxr_log(log,