From 83713a38bb8ec4a5f7afcfcfb3d4dad86860559e Mon Sep 17 00:00:00 2001 From: Dor Askayo Date: Sun, 24 Sep 2023 18:09:44 +0300 Subject: [PATCH 1/5] wayland: Remove unreachable condition The value returned from clutter_frame_get_target_presentation_time() is always same as the value returned from clutter_frame_get_min_render_time_allowed() when they are called consecutively because both functions effectively return the value of frame->has_target_presentation_time. This is with the assumption that this variable is only ever modified by the same thread that also executes on_after_update(). As such, a case where the former returns FALSE after the latter returned TRUE is not possible, which means the line that sets "target_presentation_time_us = 0;" is effectively unreachable. Acknowledging this fact allows the call to clutter_frame_get_target_presentation_time() to be moved outside the "else" case and into the "if" condition itself. This is done in preparation for the next commits. No change in behavior. Part-of: --- src/wayland/meta-wayland.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/wayland/meta-wayland.c b/src/wayland/meta-wayland.c index ac7f0e250f6..59338cd0cb6 100644 --- a/src/wayland/meta-wayland.c +++ b/src/wayland/meta-wayland.c @@ -298,6 +298,7 @@ on_after_update (ClutterStage *stage, FrameCallbackSource *frame_callback_source; GSource *source; int64_t min_render_time_allowed_us; + int64_t target_presentation_time_us; if (!META_IS_BACKEND_NATIVE (backend)) { @@ -312,20 +313,17 @@ on_after_update (ClutterStage *stage, if (meta_frame_native_had_kms_update (frame_native) || !clutter_frame_get_min_render_time_allowed (frame, - &min_render_time_allowed_us)) + &min_render_time_allowed_us) || + !clutter_frame_get_target_presentation_time (frame, + &target_presentation_time_us)) { g_source_set_ready_time (source, -1); emit_frame_callbacks_for_stage_view (compositor, stage_view); } else { - int64_t target_presentation_time_us; int64_t source_ready_time_us; - if (!clutter_frame_get_target_presentation_time (frame, - &target_presentation_time_us)) - target_presentation_time_us = 0; - if (g_source_get_ready_time (source) != -1 && frame_callback_source->target_presentation_time_us < target_presentation_time_us) -- GitLab From 8c88dbfbe869e969f347f7f652c625e1160b3c3a Mon Sep 17 00:00:00 2001 From: Dor Askayo Date: Sun, 24 Sep 2023 18:58:37 +0300 Subject: [PATCH 2/5] wayland: Rename source_ready_time_us to frame_deadline_us The value of this variable represents the last point in time in which an update would be allowed to scheduled for the given frame. Rename it for clarity and in preparation for the next commits. No change in behavior. Part-of: --- src/wayland/meta-wayland.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/wayland/meta-wayland.c b/src/wayland/meta-wayland.c index 59338cd0cb6..4832d0ed7e6 100644 --- a/src/wayland/meta-wayland.c +++ b/src/wayland/meta-wayland.c @@ -322,17 +322,17 @@ on_after_update (ClutterStage *stage, } else { - int64_t source_ready_time_us; + int64_t frame_deadline_us; if (g_source_get_ready_time (source) != -1 && frame_callback_source->target_presentation_time_us < target_presentation_time_us) emit_frame_callbacks_for_stage_view (compositor, stage_view); - source_ready_time_us = target_presentation_time_us - - min_render_time_allowed_us; + frame_deadline_us = target_presentation_time_us - + min_render_time_allowed_us; - if (source_ready_time_us <= g_get_monotonic_time ()) + if (frame_deadline_us <= g_get_monotonic_time ()) { g_source_set_ready_time (source, -1); emit_frame_callbacks_for_stage_view (compositor, stage_view); @@ -341,7 +341,7 @@ on_after_update (ClutterStage *stage, { frame_callback_source->target_presentation_time_us = target_presentation_time_us; - g_source_set_ready_time (source, source_ready_time_us); + g_source_set_ready_time (source, frame_deadline_us); } } #else -- GitLab From 26d8b9c69b50ed959850b12e7c2a876e1756621d Mon Sep 17 00:00:00 2001 From: Dor Askayo Date: Mon, 25 Sep 2023 11:56:18 +0300 Subject: [PATCH 3/5] wayland: Remove unnecessary dispatch of frame callback source To avoid communicating lower frame rate to clients through frame callbacks, it is important to avoid delaying the source dispatch when a dispatch is already scheduled. To that end, the previous logic would emit pending frame callbacks immediately in case a source dispatch was still scheduled for the previous refresh cycle and then (potentially) schedule another source dispatch for the current refresh cycle. However, emitting pending frame callbacks immediately would send frame events for every pending frame callback, including for the current "empty" update. Scheduling another source dispatch for the current cycle was then unnecessary and potentially undesirable because there may not even be another "empty" update during the cycle. Instead, let the already-scheduled source dispatch handle emitting any pending frame callbacks, and do not schedule an additional source dispatch for the current cycle as it may not be needed. This approach is useful because it removes an implicit assumption that the refresh rate is fixed and that target presentation time remains constant within a refresh cycle. This assumption does not apply for VRR. Part-of: --- src/wayland/meta-wayland.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/wayland/meta-wayland.c b/src/wayland/meta-wayland.c index 4832d0ed7e6..a269462b5b9 100644 --- a/src/wayland/meta-wayland.c +++ b/src/wayland/meta-wayland.c @@ -97,7 +97,6 @@ typedef struct MetaWaylandCompositor *compositor; ClutterStageView *stage_view; - int64_t target_presentation_time_us; } FrameCallbackSource; static void meta_wayland_compositor_update_focus (MetaWaylandCompositor *compositor, @@ -295,7 +294,6 @@ on_after_update (ClutterStage *stage, MetaContext *context = meta_wayland_compositor_get_context (compositor); MetaBackend *backend = meta_context_get_backend (context); MetaFrameNative *frame_native; - FrameCallbackSource *frame_callback_source; GSource *source; int64_t min_render_time_allowed_us; int64_t target_presentation_time_us; @@ -309,7 +307,6 @@ on_after_update (ClutterStage *stage, frame_native = meta_frame_native_from_frame (frame); source = ensure_source_for_stage_view (compositor, stage_view); - frame_callback_source = (FrameCallbackSource *) source; if (meta_frame_native_had_kms_update (frame_native) || !clutter_frame_get_min_render_time_allowed (frame, @@ -324,11 +321,6 @@ on_after_update (ClutterStage *stage, { int64_t frame_deadline_us; - if (g_source_get_ready_time (source) != -1 && - frame_callback_source->target_presentation_time_us < - target_presentation_time_us) - emit_frame_callbacks_for_stage_view (compositor, stage_view); - frame_deadline_us = target_presentation_time_us - min_render_time_allowed_us; @@ -339,8 +331,9 @@ on_after_update (ClutterStage *stage, } else { - frame_callback_source->target_presentation_time_us = - target_presentation_time_us; + if (g_source_get_ready_time (source) != -1) + return; + g_source_set_ready_time (source, frame_deadline_us); } } -- GitLab From d5c2f20d55c45a70f4cf536b9f7441e1eb67c48e Mon Sep 17 00:00:00 2001 From: Dor Askayo Date: Mon, 25 Sep 2023 12:41:48 +0300 Subject: [PATCH 4/5] wayland: Flatten logic in on_after_update() Slightly improves the cognitive complexity of the function. No change in behavior. Part-of: --- src/wayland/meta-wayland.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/wayland/meta-wayland.c b/src/wayland/meta-wayland.c index a269462b5b9..3b76281beac 100644 --- a/src/wayland/meta-wayland.c +++ b/src/wayland/meta-wayland.c @@ -297,6 +297,7 @@ on_after_update (ClutterStage *stage, GSource *source; int64_t min_render_time_allowed_us; int64_t target_presentation_time_us; + int64_t frame_deadline_us; if (!META_IS_BACKEND_NATIVE (backend)) { @@ -316,27 +317,23 @@ on_after_update (ClutterStage *stage, { g_source_set_ready_time (source, -1); emit_frame_callbacks_for_stage_view (compositor, stage_view); + return; } - else - { - int64_t frame_deadline_us; - - frame_deadline_us = target_presentation_time_us - - min_render_time_allowed_us; - if (frame_deadline_us <= g_get_monotonic_time ()) - { - g_source_set_ready_time (source, -1); - emit_frame_callbacks_for_stage_view (compositor, stage_view); - } - else - { - if (g_source_get_ready_time (source) != -1) - return; + frame_deadline_us = target_presentation_time_us - + min_render_time_allowed_us; - g_source_set_ready_time (source, frame_deadline_us); - } + if (frame_deadline_us <= g_get_monotonic_time ()) + { + g_source_set_ready_time (source, -1); + emit_frame_callbacks_for_stage_view (compositor, stage_view); + return; } + + if (g_source_get_ready_time (source) != -1) + return; + + g_source_set_ready_time (source, frame_deadline_us); #else emit_frame_callbacks_for_stage_view (compositor, stage_view); #endif -- GitLab From 3e4a330ae7c4dbf946347028acfa7f3f037c3d31 Mon Sep 17 00:00:00 2001 From: Dor Askayo Date: Sun, 24 Sep 2023 16:18:42 +0300 Subject: [PATCH 5/5] clutter/frame-clock,wayland: Calculate frame deadline during scheduling Calculate the frame deadline in ClutterFrameClock's calculate_next_update_time_us() rather than in MetaWaylandCompositor's on_after_update(). The specifics of the deadline calculation for a given frame should be implementation detail of the frame clock and and remain internal to allow extensibility. This extensibility is specifically useful for scenarios where a different deadline calculation is needed due to alternative frame scheduling logic, such as for VRR. No change in behavior. Part-of: --- clutter/clutter/clutter-frame-clock.c | 21 ++++++++++++++------- clutter/clutter/clutter-frame-private.h | 4 +++- clutter/clutter/clutter-frame.c | 8 ++++---- clutter/clutter/clutter-frame.h | 4 ++-- src/wayland/meta-wayland.c | 11 ++--------- 5 files changed, 25 insertions(+), 23 deletions(-) diff --git a/clutter/clutter/clutter-frame-clock.c b/clutter/clutter/clutter-frame-clock.c index 054e08cc5f0..8aa8b2bb816 100644 --- a/clutter/clutter/clutter-frame-clock.c +++ b/clutter/clutter/clutter-frame-clock.c @@ -79,7 +79,9 @@ struct _ClutterFrameClock gboolean is_next_presentation_time_valid; int64_t next_presentation_time_us; - int64_t min_render_time_allowed_us; + + gboolean has_next_frame_deadline; + int64_t next_frame_deadline_us; /* Buffer must be submitted to KMS and GPU rendering must be finished * this amount of time before the next presentation time. @@ -451,7 +453,7 @@ static void calculate_next_update_time_us (ClutterFrameClock *frame_clock, int64_t *out_next_update_time_us, int64_t *out_next_presentation_time_us, - int64_t *out_min_render_time_allowed_us) + int64_t *out_next_frame_deadline_us) { int64_t last_presentation_time_us; int64_t now_us; @@ -474,7 +476,7 @@ calculate_next_update_time_us (ClutterFrameClock *frame_clock, now_us; *out_next_presentation_time_us = 0; - *out_min_render_time_allowed_us = 0; + *out_next_frame_deadline_us = 0; return; } @@ -583,7 +585,7 @@ calculate_next_update_time_us (ClutterFrameClock *frame_clock, } else { - while (next_presentation_time_us < now_us + min_render_time_allowed_us) + while (next_presentation_time_us - min_render_time_allowed_us < now_us) next_presentation_time_us += refresh_interval_us; next_update_time_us = next_presentation_time_us - max_render_time_allowed_us; @@ -593,7 +595,7 @@ calculate_next_update_time_us (ClutterFrameClock *frame_clock, *out_next_update_time_us = next_update_time_us; *out_next_presentation_time_us = next_presentation_time_us; - *out_min_render_time_allowed_us = min_render_time_allowed_us; + *out_next_frame_deadline_us = next_presentation_time_us - min_render_time_allowed_us; } void @@ -664,6 +666,7 @@ clutter_frame_clock_schedule_update_now (ClutterFrameClock *frame_clock) g_source_set_ready_time (frame_clock->source, next_update_time_us); frame_clock->state = CLUTTER_FRAME_CLOCK_STATE_SCHEDULED; frame_clock->is_next_presentation_time_valid = FALSE; + frame_clock->has_next_frame_deadline = FALSE; } void @@ -686,9 +689,11 @@ clutter_frame_clock_schedule_update (ClutterFrameClock *frame_clock) calculate_next_update_time_us (frame_clock, &next_update_time_us, &frame_clock->next_presentation_time_us, - &frame_clock->min_render_time_allowed_us); + &frame_clock->next_frame_deadline_us); frame_clock->is_next_presentation_time_valid = (frame_clock->next_presentation_time_us != 0); + frame_clock->has_next_frame_deadline = + (frame_clock->next_frame_deadline_us != 0); break; case CLUTTER_FRAME_CLOCK_STATE_SCHEDULED: return; @@ -770,7 +775,9 @@ clutter_frame_clock_dispatch (ClutterFrameClock *frame_clock, frame->frame_count = frame_count; frame->has_target_presentation_time = frame_clock->is_next_presentation_time_valid; frame->target_presentation_time_us = frame_clock->next_presentation_time_us; - frame->min_render_time_allowed_us = frame_clock->min_render_time_allowed_us; + + frame->has_frame_deadline = frame_clock->has_next_frame_deadline; + frame->frame_deadline_us = frame_clock->next_frame_deadline_us; COGL_TRACE_BEGIN_SCOPED (ClutterFrameClockEvents, "Clutter::FrameListener::before_frame()"); if (iface->before_frame) diff --git a/clutter/clutter/clutter-frame-private.h b/clutter/clutter/clutter-frame-private.h index 0a0226b0a54..cdda09c042c 100644 --- a/clutter/clutter/clutter-frame-private.h +++ b/clutter/clutter/clutter-frame-private.h @@ -30,7 +30,9 @@ struct _ClutterFrame gboolean has_target_presentation_time; int64_t target_presentation_time_us; - int64_t min_render_time_allowed_us; + + gboolean has_frame_deadline; + int64_t frame_deadline_us; gboolean has_result; ClutterFrameResult result; diff --git a/clutter/clutter/clutter-frame.c b/clutter/clutter/clutter-frame.c index 85baef274ea..db2ca55bd49 100644 --- a/clutter/clutter/clutter-frame.c +++ b/clutter/clutter/clutter-frame.c @@ -76,12 +76,12 @@ clutter_frame_get_target_presentation_time (ClutterFrame *frame, } gboolean -clutter_frame_get_min_render_time_allowed (ClutterFrame *frame, - int64_t *min_render_time_allowed_us) +clutter_frame_get_frame_deadline (ClutterFrame *frame, + int64_t *frame_deadline_us) { - if (frame->has_target_presentation_time) + if (frame->has_frame_deadline) { - *min_render_time_allowed_us = frame->min_render_time_allowed_us; + *frame_deadline_us = frame->frame_deadline_us; return TRUE; } else diff --git a/clutter/clutter/clutter-frame.h b/clutter/clutter/clutter-frame.h index 1d5660d685e..34f0770bd7d 100644 --- a/clutter/clutter/clutter-frame.h +++ b/clutter/clutter/clutter-frame.h @@ -44,8 +44,8 @@ gboolean clutter_frame_get_target_presentation_time (ClutterFrame *frame, int64_t *target_presentation_time_us); CLUTTER_EXPORT -gboolean clutter_frame_get_min_render_time_allowed (ClutterFrame *frame, - int64_t *min_render_time_allowed_us); +gboolean clutter_frame_get_frame_deadline (ClutterFrame *frame, + int64_t *frame_deadline_us); CLUTTER_EXPORT void clutter_frame_set_result (ClutterFrame *frame, diff --git a/src/wayland/meta-wayland.c b/src/wayland/meta-wayland.c index 3b76281beac..9b292ff6667 100644 --- a/src/wayland/meta-wayland.c +++ b/src/wayland/meta-wayland.c @@ -295,8 +295,6 @@ on_after_update (ClutterStage *stage, MetaBackend *backend = meta_context_get_backend (context); MetaFrameNative *frame_native; GSource *source; - int64_t min_render_time_allowed_us; - int64_t target_presentation_time_us; int64_t frame_deadline_us; if (!META_IS_BACKEND_NATIVE (backend)) @@ -310,19 +308,14 @@ on_after_update (ClutterStage *stage, source = ensure_source_for_stage_view (compositor, stage_view); if (meta_frame_native_had_kms_update (frame_native) || - !clutter_frame_get_min_render_time_allowed (frame, - &min_render_time_allowed_us) || - !clutter_frame_get_target_presentation_time (frame, - &target_presentation_time_us)) + !clutter_frame_get_frame_deadline (frame, + &frame_deadline_us)) { g_source_set_ready_time (source, -1); emit_frame_callbacks_for_stage_view (compositor, stage_view); return; } - frame_deadline_us = target_presentation_time_us - - min_render_time_allowed_us; - if (frame_deadline_us <= g_get_monotonic_time ()) { g_source_set_ready_time (source, -1); -- GitLab