diff --git a/linux/runner/CMakeLists.txt b/linux/runner/CMakeLists.txt index e41c9daf8..31a45469c 100644 --- a/linux/runner/CMakeLists.txt +++ b/linux/runner/CMakeLists.txt @@ -6,6 +6,7 @@ add_executable(${BINARY_NAME} "my_application.cc" "mpv/mpv_player.cc" "mpv/mpv_plugin.cc" + "mpv/plane_render_executor.cc" "mpv/wayland_video_surface.cc" "${FLUTTER_MANAGED_DIR}/generated_plugin_registrant.cc" ) @@ -218,4 +219,17 @@ if(PLEZY_BUILD_MPV_RELIABILITY_TESTS) target_link_libraries(video_params_test PRIVATE PkgConfig::MPV) apply_mpv_reliability_sanitizer(video_params_test) add_test(NAME video_params_test COMMAND video_params_test) + + # The video plane's render worker (issue #2057). GLib for the completion + # context, Threads for the worker; no GL, no Wayland - jobs are opaque. + add_executable(plane_render_executor_test + "mpv/plane_render_executor.cc" + "mpv/plane_render_executor_test.cc" + ) + apply_standard_settings(plane_render_executor_test) + target_compile_features(plane_render_executor_test PRIVATE cxx_std_14) + target_include_directories(plane_render_executor_test PRIVATE "mpv") + target_link_libraries(plane_render_executor_test PRIVATE PkgConfig::GTK Threads::Threads) + apply_mpv_reliability_sanitizer(plane_render_executor_test) + add_test(NAME plane_render_executor_test COMMAND plane_render_executor_test) endif() diff --git a/linux/runner/mpv/mpv_player.cc b/linux/runner/mpv/mpv_player.cc index 1abb66438..f7933504f 100644 --- a/linux/runner/mpv/mpv_player.cc +++ b/linux/runner/mpv/mpv_player.cc @@ -617,16 +617,42 @@ bool MpvPlayer::InitRenderContextForSurface(EGLDisplay display, EGLConfig config surface_depth_bits_ = depth_bits > 0 ? depth_bits : 8; mpv_gl_ = candidate_gl; mpv_render_context_set_update_callback(mpv_gl_, OnMpvRenderUpdate, callback_context_.get()); + // Hand the context over unbound: from here on only the plane render thread + // makes it current (RenderToSurface), and an EGLContext can be current on at + // most one thread. Creation itself had to happen with it current here - mpv + // probes GL inside mpv_render_context_create. + if (!eglMakeCurrent(display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT)) { + g_warning("MPV: could not unbind the video-plane EGL context after creation: 0x%x", eglGetError()); + } g_message("MPV: Render context created on the Wayland video plane"); return true; } bool MpvPlayer::RenderToSurface(EGLSurface surface, int width, int height) { - std::lock_guard lock(native_mutex_); - if (disposed_ || !mpv_gl_ || egl_context_ == EGL_NO_CONTEXT || surface == EGL_NO_SURFACE) return false; - if (width < 1 || height < 1) return false; + // Runs on the plane render thread. Deliberately not holding native_mutex_ + // across the render: a 4K HDR tone-map takes tens of milliseconds, and the + // mutex has main-thread callers on every seek (ReadSourceHdrMetadata, + // UpdateSourceHdrMetadata) - holding it here would rebuild the very stall + // this thread exists to remove, behind a lock instead of a thread. The + // snapshot below is safe without it because of ordering, not locking: the + // plugin drains the render thread (release_video_resources) before + // Dispose() hands mpv_gl_ and the EGL context to the teardown queue, so + // neither can be freed while a job is running. + mpv_render_context* render_context = nullptr; + EGLDisplay display = EGL_NO_DISPLAY; + EGLContext context = EGL_NO_CONTEXT; + int depth_bits = 8; + { + std::lock_guard lock(native_mutex_); + if (disposed_ || !mpv_gl_ || egl_context_ == EGL_NO_CONTEXT || surface == EGL_NO_SURFACE) return false; + if (width < 1 || height < 1) return false; + render_context = mpv_gl_; + display = egl_display_; + context = egl_context_; + depth_bits = surface_depth_bits_; + } - if (!eglBindAPI(EGL_OPENGL_ES_API) || !eglMakeCurrent(egl_display_, surface, surface, egl_context_)) { + if (!eglBindAPI(EGL_OPENGL_ES_API) || !eglMakeCurrent(display, surface, surface, context)) { g_warning("MPV: Failed to activate the video-plane EGL context for render: 0x%x", eglGetError()); return false; } @@ -642,21 +668,21 @@ bool MpvPlayer::RenderToSurface(EGLSurface surface, int width, int height) { // Ignored by the render API's OpenGL backend, which reads the depth param // instead, but it is what mpv#16818's gpu-next backend will read, so state // it truthfully rather than leave a lie in place for that day. - mpv_fbo.internal_format = surface_depth_bits_ >= 16 ? GL_RGBA16F : surface_depth_bits_ >= 10 ? GL_RGB10_A2 : GL_RGBA8; + mpv_fbo.internal_format = depth_bits >= 16 ? GL_RGBA16F : depth_bits >= 10 ? GL_RGB10_A2 : GL_RGBA8; // The default framebuffer is bottom-up relative to mpv's image orientation, // so this flips. int flip_y = 1; // Without this mpv assumes 8 bits and dithers a 10-bit PQ plane down to 8, // which bands precisely in the dark ramp PQ spends most of its code space on. - int depth = surface_depth_bits_; + int depth = depth_bits; mpv_render_param params[] = { {MPV_RENDER_PARAM_OPENGL_FBO, &mpv_fbo}, {MPV_RENDER_PARAM_FLIP_Y, &flip_y}, {MPV_RENDER_PARAM_DEPTH, &depth}, {MPV_RENDER_PARAM_INVALID, nullptr}, }; - mpv_render_context_render(mpv_gl_, params); + mpv_render_context_render(render_context, params); return true; } @@ -702,11 +728,15 @@ void MpvPlayer::Dispose() { RemoveTrackedSources(); - // The plane's context is left current on this thread by RenderToSurface and - // nothing else releases it before the video surface is destroyed - which the - // plugin does *after* this call. An EGLContext can be current to at most one - // thread, so handing it to the teardown worker while it is still bound here - // makes the worker's eglMakeCurrent fail with EGL_BAD_ACCESS; the pair is then + // The plane's context is normally already unbound by the time this runs: the + // plane render thread makes it current (RenderToSurface) and the plugin's + // teardown posts an unbind there before draining the thread ahead of this + // call. This main-thread release covers the two paths that still bind it + // here - the PLEZY_PLANE_RENDER_MAIN_THREAD inline fallback, and a context + // created but never rendered with, where InitRenderContextForSurface's own + // unbind failed. An EGLContext can be current to at most one thread, so + // handing it to the teardown worker while it is still bound here makes the + // worker's eglMakeCurrent fail with EGL_BAD_ACCESS; the pair is then // retained, and by the note below the mpv handle cannot be terminated until // every pair drains. Repeated open/close would carry a whole stale mpv core // across each gap. Only our own context is released: Flutter's must be left diff --git a/linux/runner/mpv/mpv_player.h b/linux/runner/mpv/mpv_player.h index 39c6fec56..4e71c5380 100644 --- a/linux/runner/mpv/mpv_player.h +++ b/linux/runner/mpv/mpv_player.h @@ -100,6 +100,11 @@ class MpvPlayer { /// Renders one frame into |surface|'s default framebuffer. The caller /// presents it (eglSwapBuffers) once this returns. + /// + /// Runs on the plane render thread and deliberately holds no lock across + /// the render; the caller guarantees by ordering (drain the render thread, + /// then Dispose) that the render context outlives every call. The EGL + /// context becomes current on the calling thread and stays there. /// @return true if the frame was rendered. bool RenderToSurface(EGLSurface surface, int width, int height); diff --git a/linux/runner/mpv/mpv_plugin.cc b/linux/runner/mpv/mpv_plugin.cc index dfdf6fc27..bfd24b981 100644 --- a/linux/runner/mpv/mpv_plugin.cc +++ b/linux/runner/mpv/mpv_plugin.cc @@ -4,13 +4,16 @@ #include #include #include +#include #include #include +#include "plane_render_executor.h" #include "wayland_video_surface.h" using PlayerPtr = std::unique_ptr; using VideoSurfacePtr = std::unique_ptr; +using ExecutorPtr = std::unique_ptr; // One queued HDR transaction: what to apply, and who to tell when it settles. // @@ -44,6 +47,24 @@ struct _MpvPlugin { // stale or absent. Sticky, because the render it asks for may first have to // wait out an unacknowledged frame. gboolean plane_needs_render; + // The worker that runs mpv's render + the plane's eglSwapBuffers off the + // GTK main thread (issue #2057: an expensive per-frame render - a 4K HDR + // tone-map - on the main thread starves input dispatch and Flutter's + // raster). Created with the plane; drained and shut down in + // release_video_resources *before* the player is disposed, which is the + // ordering RenderToSurface's lock-free render relies on. Null when + // PLEZY_PLANE_RENDER_MAIN_THREAD selects the inline fallback. + ExecutorPtr render_executor; + // A render job is somewhere between PreparePresent() and CompletePresent(). + // Gates render_video_plane - the flight owns the plane's EGL surface - and + // defers rect application and HDR transaction starts to the completion. + gboolean render_in_flight; + // A rect arrived while a job was in flight. Applied at completion, because + // wl_egl_window_resize must not race the swap. + gboolean rect_apply_deferred; + // An HDR transaction was ready to start while a job was in flight. Started + // at completion; see run_next_hdr_transaction for why it must wait. + gboolean hdr_start_deferred; gboolean visible; gboolean initialized; gboolean audio_only; @@ -213,6 +234,46 @@ static void release_video_resources(MpvPlugin* self) { for (auto& request : queued) { if (request.done) request.done(MPV_ERROR_UNINITIALIZED); } + // Drain the render thread before anything a job touches is torn down: + // RenderToSurface's lock-free render is safe only because mpv_gl_, the EGL + // context and the plane's EGL surface outlive every job, and this is where + // that ordering is enforced. The final job unbinds the EGL context on the + // worker - the one thread it is current on - so the teardown queue's worker + // can bind it (an EGLContext can be current on at most one thread). + bool render_thread_wedged = false; + if (self->render_executor) { + const EGLDisplay unbind_display = self->video_surface ? self->video_surface->egl_display() : EGL_NO_DISPLAY; + self->render_executor->Post( + [unbind_display]() -> bool { + if (unbind_display != EGL_NO_DISPLAY) { + eglMakeCurrent(unbind_display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); + } + return true; + }, + nullptr); + render_thread_wedged = !self->render_executor->ShutdownAndJoin(5000); + self->render_executor.reset(); + } + self->render_in_flight = FALSE; + self->rect_apply_deferred = FALSE; + self->hdr_start_deferred = FALSE; + if (render_thread_wedged) { + // A job is stuck inside a driver call. Disposing the player frees the + // render context under it and destroying the plane frees the EGL surface + // it is drawing into - either is a guaranteed crash. Leaking one + // session's core and plane keeps the process alive; stop is best-effort, + // because the mpv client API is thread-safe and the wedged job holds only + // the render API. + g_warning("MPV video plane: render thread did not drain; leaking this session's player and plane"); + if (self->player) { + self->player->SetRedrawCallback(nullptr); + self->player->SetSourceMetadataCallback(nullptr); + self->player->SetEventCallback(nullptr); + self->player->Command({"stop"}); + self->player.release(); + } + if (self->video_surface) self->video_surface.release(); + } if (self->player) { // The plane is a raw callback target. Revoke every callback path before // tearing it down; Dispose then drains any callback already holding a @@ -271,6 +332,15 @@ static void apply_pending_rect(MpvPlugin* self) { self->video_surface->SetRect(rect.x, rect.y, rect.width, rect.height, rect.scale); } +// Runs |job| on the plane render thread and |completion| back on the main +// thread with the job's result. The inline fallback (PLEZY_PLANE_RENDER_MAIN_THREAD) +// runs both synchronously, preserving the pre-#2057 single-threaded behaviour. +static bool post_render_job(MpvPlugin* self, std::function job, std::function completion) { + if (self->render_executor) return self->render_executor->Post(std::move(job), std::move(completion)); + completion(job()); + return true; +} + // Renders and presents one frame on the native video plane. Skipped while the // plane is hidden or has not been given a rect yet; both of those paths render // explicitly once the condition clears, because mpv's redraw latch stays set @@ -278,11 +348,22 @@ static void apply_pending_rect(MpvPlugin* self) { // // |force| is for the callers who need pixels regardless of whether mpv has // produced a new frame: a resize, or the plane becoming visible again. +// +// The render and the swap themselves run on the plane render thread (issue +// #2057): a frame render that costs a real fraction of the frame budget - a +// 4K HDR tone-map, say - would otherwise starve input dispatch and Flutter's +// raster, which share the GTK main thread. Everything up to PreparePresent() +// and everything from the completion on stays here on the main thread. static void render_video_plane(MpvPlugin* self, gboolean force) { if (force) self->plane_needs_render = TRUE; if (!self->player || !self->video_surface || !self->video_surface->valid()) return; + // One job at a time. The flight owns the plane's EGL surface, and a second + // prepare would arm a frame callback over a commit that has not happened. + // Nothing arriving meanwhile is lost: the completion re-runs this function, + // and plane_needs_render and mpv's redraw latch both hold their edge. + if (self->render_in_flight) return; if (!self->video_surface->visible() || !self->video_surface->has_size()) return; - // Present() is held while a colour transition is staged, so a render now would + // Presents are held while a colour transition is staged, so a render now would // be discarded. The forced render after CommitHdrTransition is what resumes; // plane_needs_render stays set meanwhile, so nothing is lost. if (self->video_surface->hdr_transition_staged()) return; @@ -306,15 +387,61 @@ static void render_video_plane(MpvPlugin* self, gboolean force) { // 60fps content on a 120Hz output, half of them redrawing the same picture. // mpv's redraw latch is what says a new frame actually exists. if (!self->plane_needs_render && !self->player->NeedsRedraw()) return; - if (self->player->RenderToSurface( - self->video_surface->egl_surface(), self->video_surface->width(), self->video_surface->height())) { - // Only once a frame has actually been published. Present() returns false on - // an eglSwapBuffers failure having already destroyed its frame callback, so - // clearing the flag first would drop both the retry and the thing that would - // have rescheduled it, and the plane would sit on a stale buffer until an - // unrelated event arrived. Its other false returns are all re-tested above - // on this same thread, so a swap failure is the only way to get here. - if (self->video_surface->Present()) self->plane_needs_render = FALSE; + if (!self->video_surface->PreparePresent()) return; + + // Everything the job touches is snapshotted now and stays alive for the + // flight's duration: release_video_resources drains the render thread + // before the player or the plane is torn down. + mpv::MpvPlayer* player = self->player.get(); + EGLDisplay display = self->video_surface->egl_display(); + EGLSurface egl_surface = self->video_surface->egl_surface(); + const int width = self->video_surface->width(); + const int height = self->video_surface->height(); + const guint64 generation = self->generation; + self->render_in_flight = TRUE; + const bool posted = post_render_job( + self, + [player, display, egl_surface, width, height]() -> bool { + if (!player->RenderToSurface(egl_surface, width, height)) return false; + // The swap is the child surface's commit. Non-throttled + // (eglSwapInterval 0), so it never blocks on the compositor; its cost + // is the render's, which is exactly what this thread is for. + if (eglSwapBuffers(display, egl_surface) != EGL_TRUE) { + g_warning("MPV video plane: eglSwapBuffers failed: 0x%x", eglGetError()); + return false; + } + return true; + }, + [self, generation](bool swapped) { + // The plane this job rendered for may be gone: release_video_resources + // bumps the generation, and completions outlive the executor. + if (self->generation != generation) return; + self->render_in_flight = FALSE; + if (self->video_surface == nullptr) return; + // A swap failure leaves plane_needs_render set, so the retry - and the + // frame callback CompletePresent just cleared - are both owed to the + // next event that moves the plane, exactly as before the split. + if (self->video_surface->CompletePresent(swapped)) self->plane_needs_render = FALSE; + // Work that had to wait out the flight, in dependency order: geometry + // first (wl_egl_window_resize must not race a swap), then the HDR + // transaction pump (a transition staged mid-flight would pair an + // old-colour buffer with a new description), then the render either + // may have asked for. + if (self->rect_apply_deferred) { + self->rect_apply_deferred = FALSE; + apply_pending_rect(self); + } + if (self->hdr_start_deferred) { + self->hdr_start_deferred = FALSE; + run_next_hdr_transaction(self); + } + render_video_plane(self, FALSE); + }); + if (!posted) { + // Shutdown has begun; the job will never run. Undo the prepare so the + // frame callback does not wait forever on a commit that is not coming. + self->render_in_flight = FALSE; + self->video_surface->CompletePresent(false); } } @@ -679,6 +806,19 @@ static void run_next_hdr_transaction(MpvPlugin* self) { } return; } + // A render job in flight was prepared before this transaction's turn came; + // let it land first. Staging now would raise the present hold *after* that + // job's prepare, so the commit its swap performs would pair an old-colour + // buffer with whatever the transaction stages - the exact mismatch the + // two-phase dance exists to avoid - and the mpv leg would move the output + // properties under a frame mid-render. Setting the in-flight flag keeps the + // pump's invariant (a non-empty queue implies a transaction in flight); the + // render completion re-enters here. + if (self->render_in_flight) { + self->hdr_transaction_in_flight = true; + self->hdr_start_deferred = TRUE; + return; + } PendingHdrRequest request = std::move(self->hdr_queue.front()); self->hdr_queue.pop_front(); self->hdr_transaction_in_flight = true; @@ -830,6 +970,15 @@ static gboolean start_video_plane(MpvPlugin* self, FlView* view, std::string* er // coalesces the two into one transaction when they arrive together, and a late // parse converges rather than leaving a wrong description standing. self->player->SetSourceMetadataCallback([self]() { request_hdr_reapply(self); }); + // The worker that runs mpv's render + the plane's eglSwapBuffers off the + // GTK main thread (issue #2057). The env var is a temporary escape hatch + // for driver surprises: the inline fallback preserves the old + // single-threaded behaviour through the same code path. + if (g_getenv("PLEZY_PLANE_RENDER_MAIN_THREAD") != nullptr) { + g_message("MPV video plane: rendering on the GTK main thread (PLEZY_PLANE_RENDER_MAIN_THREAD)"); + } else { + self->render_executor = std::make_unique(); + } // A rect that arrived before this plane existed is the only one Dart may ever // offer, since it re-sends solely on change. Hand it over now, before the // first frame, so the plane is never left sizeless and blank. @@ -858,6 +1007,7 @@ static void mpv_plugin_dispose(GObject* object) { static void mpv_plugin_finalize(GObject* object) { MpvPlugin* self = MPV_PLUGIN(object); self->hdr_queue.~HdrQueue(); + self->render_executor.~ExecutorPtr(); self->video_surface.~VideoSurfacePtr(); self->player.~PlayerPtr(); G_OBJECT_CLASS(mpv_plugin_parent_class)->finalize(object); @@ -1332,10 +1482,18 @@ static void mpv_plugin_handle_method_call(FlMethodChannel* channel, FlMethodCall self->pending_rect.scale = scale; self->has_pending_rect = TRUE; if (self->video_surface) { - apply_pending_rect(self); - // Re-render at the new size straight away; waiting for the next mpv - // frame would leave a stale buffer stretched across the new rect. - render_video_plane(self, TRUE); + if (self->render_in_flight) { + // wl_egl_window_resize must not race the in-flight swap; the + // render completion applies the rect. The sticky flag makes it + // re-render at the new size, exactly like the immediate path. + self->rect_apply_deferred = TRUE; + self->plane_needs_render = TRUE; + } else { + apply_pending_rect(self); + // Re-render at the new size straight away; waiting for the next mpv + // frame would leave a stale buffer stretched across the new rect. + render_video_plane(self, TRUE); + } } response = FL_METHOD_RESPONSE(fl_method_success_response_new(nullptr)); } diff --git a/linux/runner/mpv/plane_render_executor.cc b/linux/runner/mpv/plane_render_executor.cc new file mode 100644 index 000000000..154b59491 --- /dev/null +++ b/linux/runner/mpv/plane_render_executor.cc @@ -0,0 +1,100 @@ +#include "plane_render_executor.h" + +#include + +namespace mpv { + +namespace { + +struct CompletionInvocation { + PlaneRenderExecutor::Completion completion; + bool result; +}; + +gboolean InvokeCompletion(gpointer data) { + auto* invocation = static_cast(data); + invocation->completion(invocation->result); + return G_SOURCE_REMOVE; +} + +void DestroyCompletionInvocation(gpointer data) { delete static_cast(data); } + +} // namespace + +PlaneRenderExecutor::Shared::~Shared() { + if (completion_context != nullptr) g_main_context_unref(completion_context); +} + +PlaneRenderExecutor::PlaneRenderExecutor() : shared_(std::make_shared()) { + shared_->completion_context = g_main_context_ref_thread_default(); + thread_ = std::thread(&PlaneRenderExecutor::Run, shared_); +} + +PlaneRenderExecutor::~PlaneRenderExecutor() { ShutdownAndJoin(5000); } + +bool PlaneRenderExecutor::Post(Job job, Completion completion) { + { + std::lock_guard lock(shared_->mutex); + if (shared_->quitting) return false; + shared_->jobs.emplace_back(std::move(job), std::move(completion)); + } + shared_->wake.notify_one(); + return true; +} + +bool PlaneRenderExecutor::ShutdownAndJoin(unsigned int timeout_ms) { + if (!thread_.joinable()) return !abandoned_; + { + std::unique_lock lock(shared_->mutex); + shared_->quitting = true; + shared_->wake.notify_all(); + if (!shared_->idle.wait_for(lock, std::chrono::milliseconds(timeout_ms), [this] { + return shared_->jobs.empty() && !shared_->running_job; + })) { + abandoned_ = true; + } + } + if (abandoned_) { + // The job is wedged inside a call that cannot be interrupted. Joining + // would hang the caller forever; the thread is cut loose instead, and the + // caller is told so it can leak, rather than free, whatever the job may + // still be touching. The worker keeps the shared state alive on its own. + thread_.detach(); + return false; + } + thread_.join(); + return true; +} + +void PlaneRenderExecutor::Run(const std::shared_ptr& shared) { + for (;;) { + Job job; + Completion completion; + { + std::unique_lock lock(shared->mutex); + shared->wake.wait(lock, [&shared] { return !shared->jobs.empty() || shared->quitting; }); + // Quitting drains: queued jobs still run, so a shutdown-time job (the + // EGL unbind) can be posted and then waited for. + if (shared->jobs.empty()) return; + job = std::move(shared->jobs.front().first); + completion = std::move(shared->jobs.front().second); + shared->jobs.pop_front(); + shared->running_job = true; + } + + const bool result = job ? job() : false; + if (completion) { + g_main_context_invoke_full( + shared->completion_context, G_PRIORITY_DEFAULT, InvokeCompletion, + new CompletionInvocation{std::move(completion), result}, DestroyCompletionInvocation); + } + + { + std::lock_guard lock(shared->mutex); + shared->running_job = false; + } + shared->idle.notify_all(); + } +} + +} // namespace mpv diff --git a/linux/runner/mpv/plane_render_executor.h b/linux/runner/mpv/plane_render_executor.h new file mode 100644 index 000000000..607dd9def --- /dev/null +++ b/linux/runner/mpv/plane_render_executor.h @@ -0,0 +1,86 @@ +#ifndef PLEZY_LINUX_MPV_PLANE_RENDER_EXECUTOR_H_ +#define PLEZY_LINUX_MPV_PLANE_RENDER_EXECUTOR_H_ + +#include + +#include +#include +#include +#include +#include +#include +#include + +namespace mpv { + +// The video plane's render worker (issue #2057). +// +// mpv's render + the plane's eglSwapBuffers used to run on the GTK main +// thread, which also rasters Flutter's UI and dispatches input. A cheap frame +// hides that; a 4K HDR tone-map does not - input reads batch at video-render +// boundaries and every UI repaint waits out the render. This worker exists to +// take exactly that GL work off the main thread. Everything else about the +// plane - Wayland protocol state, watchdogs, colour transitions, geometry - +// deliberately stays on the main thread; see WaylandVideoSurface. +// +// Contract: +// - Jobs run in order on one worker thread and return a bool. +// - Completions run on the GLib main context that was thread-default when +// the executor was constructed, carrying the job's result. They are +// delivered even after ShutdownAndJoin, so they must guard against state +// that has since been torn down (the plugin's generation counter). +// - ShutdownAndJoin drains queued jobs first. A job wedged inside a driver +// call cannot be interrupted; past the timeout the thread is abandoned +// (detached) and false is returned so the caller can leak, rather than +// free, what the job may still touch. The worker owns its state through a +// shared_ptr, never through the executor object, so abandonment leaks that +// state instead of leaving the thread on freed memory. +class PlaneRenderExecutor { + public: + // Runs on the worker thread; the result is handed to the completion. + using Job = std::function; + // Runs on the construction-time GLib main context. + using Completion = std::function; + + PlaneRenderExecutor(); + ~PlaneRenderExecutor(); + + PlaneRenderExecutor(const PlaneRenderExecutor&) = delete; + PlaneRenderExecutor& operator=(const PlaneRenderExecutor&) = delete; + + /// Queues |job|. Returns false only after shutdown has begun, in which case + /// neither the job nor the completion will run. + bool Post(Job job, Completion completion); + + /// Stops accepting jobs, waits up to |timeout_ms| for queued jobs to drain, + /// and joins the worker. Returns false when the worker had to be abandoned + /// instead - see the class comment. Idempotent. + bool ShutdownAndJoin(unsigned int timeout_ms); + + private: + // Everything the worker touches. Held by shared_ptr from both the executor + // and the worker thread's closure, so an abandoned worker still stands on + // live memory. The completion context reference is owned here and released + // by the destructor - i.e. by whichever side lets go last. + struct Shared { + ~Shared(); + + GMainContext* completion_context = nullptr; + std::mutex mutex; + std::condition_variable wake; + std::condition_variable idle; + std::deque> jobs; + bool running_job = false; + bool quitting = false; + }; + + static void Run(const std::shared_ptr& shared); + + std::shared_ptr shared_; + std::thread thread_; + bool abandoned_ = false; +}; + +} // namespace mpv + +#endif // PLEZY_LINUX_MPV_PLANE_RENDER_EXECUTOR_H_ diff --git a/linux/runner/mpv/plane_render_executor_test.cc b/linux/runner/mpv/plane_render_executor_test.cc new file mode 100644 index 000000000..68eb09f9d --- /dev/null +++ b/linux/runner/mpv/plane_render_executor_test.cc @@ -0,0 +1,225 @@ +#include "plane_render_executor.h" + +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +int failures = 0; + +void Expect(bool condition, const char* expression, int line) { + if (condition) return; + std::cerr << "line " << line << ": check failed: " << expression << '\n'; + ++failures; +} + +#define EXPECT(condition) Expect(static_cast(condition), #condition, __LINE__) + +// Each test runs against its own GLib context installed as the thread default, +// which is what the executor captures for completion delivery - exactly how +// the plugin's main context reaches it in production. +class ScopedMainContext { + public: + ScopedMainContext() : context_(g_main_context_new()) { g_main_context_push_thread_default(context_); } + ~ScopedMainContext() { + g_main_context_pop_thread_default(context_); + g_main_context_unref(context_); + } + + GMainContext* get() const { return context_; } + + // Iterates the context until |done| holds or the deadline passes. Completions + // are GSources on this context, so this is what "the main thread ran" means. + bool IterateUntil(const std::function& done, int timeout_ms = 5000) { + const auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(timeout_ms); + while (!done()) { + if (std::chrono::steady_clock::now() > deadline) return false; + g_main_context_iteration(context_, FALSE); + } + return true; + } + + private: + GMainContext* context_; +}; + +// Jobs must run off the constructing thread; completions must run on the +// constructing thread's context, carrying the job's result. +void TestJobRunsOffThreadAndCompletesOnContext() { + ScopedMainContext context; + mpv::PlaneRenderExecutor executor; + + std::atomic job_ran{false}; + std::thread::id job_thread; + std::atomic completed{false}; + bool completion_result = false; + std::thread::id completion_thread; + + EXPECT(executor.Post( + [&]() { + job_thread = std::this_thread::get_id(); + job_ran = true; + return true; + }, + [&](bool result) { + completion_result = result; + completion_thread = std::this_thread::get_id(); + completed = true; + })); + + EXPECT(context.IterateUntil([&] { return completed.load(); })); + EXPECT(job_ran.load()); + EXPECT(job_thread != std::this_thread::get_id()); + EXPECT(completion_thread == std::this_thread::get_id()); + EXPECT(completion_result); + EXPECT(executor.ShutdownAndJoin(5000)); +} + +// A job's false lands in its completion untouched: the plugin's completion +// distinguishes a presented frame from a failed render/swap by exactly this. +void TestFailedJobReportsFalse() { + ScopedMainContext context; + mpv::PlaneRenderExecutor executor; + + std::atomic completed{false}; + bool completion_result = true; + EXPECT(executor.Post([] { return false; }, + [&](bool result) { + completion_result = result; + completed = true; + })); + EXPECT(context.IterateUntil([&] { return completed.load(); })); + EXPECT(!completion_result); + EXPECT(executor.ShutdownAndJoin(5000)); +} + +// Jobs and their completions keep their queue order. The plugin only ever has +// one job in flight, but the shutdown-time EGL unbind queues behind a live +// render, and it must not overtake it. +void TestJobsRunInOrder() { + ScopedMainContext context; + mpv::PlaneRenderExecutor executor; + + std::vector job_order; + std::mutex order_mutex; + std::vector completion_order; + std::atomic completions{0}; + + for (int i = 0; i < 3; ++i) { + EXPECT(executor.Post( + [&, i] { + std::lock_guard lock(order_mutex); + job_order.push_back(i); + return true; + }, + [&, i](bool) { + completion_order.push_back(i); + ++completions; + })); + } + + EXPECT(context.IterateUntil([&] { return completions.load() == 3; })); + EXPECT((job_order == std::vector{0, 1, 2})); + EXPECT((completion_order == std::vector{0, 1, 2})); + EXPECT(executor.ShutdownAndJoin(5000)); +} + +// Shutdown drains what was already queued - that is what makes posting the +// EGL unbind ahead of ShutdownAndJoin sufficient - and refuses anything after. +void TestShutdownDrainsQueuedJobsAndRefusesNewOnes() { + ScopedMainContext context; + mpv::PlaneRenderExecutor executor; + + // Hold the worker inside the first job so the second is provably still + // queued when shutdown begins. + std::mutex gate_mutex; + std::condition_variable gate_cv; + bool gate_open = false; + std::atomic second_ran{false}; + + EXPECT(executor.Post( + [&] { + std::unique_lock lock(gate_mutex); + gate_cv.wait(lock, [&] { return gate_open; }); + return true; + }, + nullptr)); + EXPECT(executor.Post([&] { + second_ran = true; + return true; + }, + nullptr)); + + std::thread opener([&] { + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + { + std::lock_guard lock(gate_mutex); + gate_open = true; + } + gate_cv.notify_all(); + }); + EXPECT(executor.ShutdownAndJoin(5000)); + opener.join(); + EXPECT(second_ran.load()); + EXPECT(!executor.Post([] { return true; }, nullptr)); +} + +// A worker wedged inside a driver call cannot be joined; the executor must +// abandon it within the timeout and say so, because the caller's next move - +// leak instead of free - depends on the answer. The late completion must still +// be deliverable without touching freed executor state. +void TestWedgedJobIsAbandonedNotJoined() { + ScopedMainContext context; + + std::mutex gate_mutex; + std::condition_variable gate_cv; + bool gate_open = false; + std::atomic completed{false}; + + auto executor = std::make_unique(); + EXPECT(executor->Post( + [&] { + std::unique_lock lock(gate_mutex); + gate_cv.wait(lock, [&] { return gate_open; }); + return true; + }, + [&](bool) { completed = true; })); + + const auto start = std::chrono::steady_clock::now(); + EXPECT(!executor->ShutdownAndJoin(100)); + EXPECT(std::chrono::steady_clock::now() - start < std::chrono::seconds(4)); + // Destruction after abandonment must not hang or double-join. + executor.reset(); + + // Un-wedge the abandoned thread; its completion should still arrive on the + // context, proving the thread's own context reference outlived the executor. + { + std::lock_guard lock(gate_mutex); + gate_open = true; + } + gate_cv.notify_all(); + EXPECT(context.IterateUntil([&] { return completed.load(); })); +} + +} // namespace + +int main() { + TestJobRunsOffThreadAndCompletesOnContext(); + TestFailedJobReportsFalse(); + TestJobsRunInOrder(); + TestShutdownDrainsQueuedJobsAndRefusesNewOnes(); + TestWedgedJobIsAbandonedNotJoined(); + + if (failures != 0) { + std::cerr << "plane_render_executor_test: " << failures << " failure(s)\n"; + return 1; + } + std::cout << "plane_render_executor_test: PASS\n"; + return 0; +} diff --git a/linux/runner/mpv/wayland_video_surface.cc b/linux/runner/mpv/wayland_video_surface.cc index 01ebcd317..c6cf82759 100644 --- a/linux/runner/mpv/wayland_video_surface.cc +++ b/linux/runner/mpv/wayland_video_surface.cc @@ -663,7 +663,7 @@ void WaylandVideoSurface::ClearStagedDescription() { void WaylandVideoSurface::SettleTransition(bool ok) { if (!transition_staged_) return; // Re-armed, not cancelled. The compositor answering only ends the *first* of - // two waits: the plane stays staged - and Present() stays held - until the + // two waits: the plane stays staged - and presents stay held - until the // caller's mpv leg commits or aborts, which is a longer wait than this one and // has no timeout of its own. Cancelling here left exactly that window // unbounded, so a silent mpv froze the plane for good. @@ -722,7 +722,7 @@ void WaylandVideoSurface::BeginHdrTransition( } // Metadata matters only while described; otherwise every SDR source change - // would stage a no-op transition that holds Present() and forces a render. + // would stage a no-op transition that holds presents and forces a render. if (describe == hdr_active_ && (!describe || metadata_ == metadata)) { if (on_settled) on_settled(0, true); return; @@ -745,7 +745,7 @@ void WaylandVideoSurface::BeginHdrTransition( BuildImageDescription(); } -// Present() and the plugin's render path are both held while a transition is +// PreparePresent() and the plugin's render path are both held while a transition is // staged, so a compositor that accepts create() and then answers with neither // ready nor failed freezes the plane on its last buffer for good, and every // queued HDR method call behind it never answers. Everything else in this file @@ -792,7 +792,7 @@ void WaylandVideoSurface::ArmTransitionWatchdog() { "resuming presentation on the description already in force after %d seconds", kTransitionTimeoutSeconds); self->DiscardTransition(); - // Present() was held for the whole staged window, so nothing else will + // Presents were held for the whole staged window, so nothing else will // start it again: no frame callback is outstanding, and mpv - which by // definition has gone quiet - will not raise its redraw latch either. // That rules out the ordinary frame callback, whose handler skips unless @@ -983,7 +983,7 @@ void WaylandVideoSurface::BuildImageDescription() { } void WaylandVideoSurface::ArmFrameAckWatchdog() { - // One watchdog per outstanding callback. Present() re-arms after each + // One watchdog per outstanding callback. CompletePresent() re-arms after each // acknowledgement or timeout, so a source that is already set means the // previous timer is still waiting on a callback that has not been answered. if (frame_ack_source_ != 0 || !frame_pending_ || !visible_) return; @@ -1067,7 +1067,7 @@ void WaylandVideoSurface::ClearFrameCallback() { void WaylandVideoSurface::HandleFrameDone(void* data, wl_callback* callback, uint32_t time) { (void)time; auto* self = static_cast(data); - // Always the callback we hold: Present() is the only place one is created and + // Always the callback we hold: PreparePresent() is the only place one is created and // it early-returns while frame_pending_, so a second is never armed over a // live one, and libwayland delivers nothing for a proxy we already destroyed. if (self->frame_callback_ == callback) { @@ -1251,7 +1251,7 @@ void WaylandVideoSurface::SetRect(int32_t x, int32_t y, int32_t width, int32_t h // presented: mesa commits the EGL surface's pre-allocated 1x1 back buffer // on the first swap regardless of wl_egl_window_resize, and a 1x1 buffer at // scale > 1 is a fatal protocol error (the compositor disconnects us). - // Present() flushes the deferred scale right after that first commit. The + // CompletePresent() flushes the deferred scale right after that first commit. The // gate is the first-frame latch rather than buffer attachment: the committed // scale survives a detach, so once a frame has been presented the scale must // be updatable with no buffer attached. @@ -1287,11 +1287,11 @@ void WaylandVideoSurface::SetVisible(bool visible) { visible_ = visible; if (surface_ == nullptr) return; if (!visible) DetachBuffer(); - // Becoming visible needs no action here: the next Present() attaches a buffer. + // Becoming visible needs no action here: the next present attaches a buffer. RequestParentCommit(); } -bool WaylandVideoSurface::Present() { +bool WaylandVideoSurface::PreparePresent() { if (!visible_ || egl_surface_ == EGL_NO_SURFACE || frame_pending_) return false; // Held while a colour transition is staged. eglSwapBuffers is the child // surface's commit, so presenting now would publish a buffer paired with a @@ -1301,17 +1301,32 @@ bool WaylandVideoSurface::Present() { if (transition_staged_) return false; // Ask for the acknowledgement before the commit that eglSwapBuffers performs, - // so the callback belongs to this frame. + // so the callback belongs to this frame. The request is issued here, on the + // main thread, *before* the render job is posted: libwayland serializes + // requests across threads in call order, so the happens-before of the job + // handoff is what keeps this frame request ahead of the worker's commit. static const wl_callback_listener kFrameListener = {HandleFrameDone}; frame_callback_ = wl_surface_frame(surface_); if (frame_callback_ != nullptr) { wl_callback_add_listener(frame_callback_, &kFrameListener, this); frame_pending_ = true; } + return true; +} - if (eglSwapBuffers(egl_display_, egl_surface_) != EGL_TRUE) { +bool WaylandVideoSurface::CompletePresent(bool swapped) { + if (!swapped) { + // The render job logged why. The frame callback belongs to a commit that + // never happened; without this it would wait forever and frame_pending_ + // would hold off every later present. ClearFrameCallback(); - g_warning("MPV video plane: eglSwapBuffers failed: 0x%x", eglGetError()); + return false; + } + // A hide or a rect loss that landed while the swap was in flight already + // detached the buffer - and the swap just re-attached one behind its back. + // Detach again; the plane is not showing this frame. + if (!visible_ || !rect_valid_) { + DetachBuffer(); return false; } if (!first_frame_presented_) { @@ -1332,7 +1347,8 @@ bool WaylandVideoSurface::Present() { } // The acknowledgement for this commit is now owed; bound the wait so a // compositor that never pays it cannot freeze the plane (see - // ArmFrameAckWatchdog). + // ArmFrameAckWatchdog). If the compositor already acknowledged between the + // swap and this call, frame_pending_ is false again and the arm is a no-op. ArmFrameAckWatchdog(); return true; } diff --git a/linux/runner/mpv/wayland_video_surface.h b/linux/runner/mpv/wayland_video_surface.h index fe72591b6..682cd2984 100644 --- a/linux/runner/mpv/wayland_video_surface.h +++ b/linux/runner/mpv/wayland_video_surface.h @@ -57,7 +57,12 @@ struct PreferredColorDescription { // whole window surface per presented frame (see gdk_cairo_draw_from_gl's // alpha path), previously paid once per *video* frame. // -// Everything here runs on the GTK main thread. The subsurface is desynchronized +// Everything here runs on the GTK main thread, with one deliberate exception: +// the commit itself. PreparePresent()/CompletePresent() bracket an +// eglSwapBuffers that the plugin performs on the plane render thread (issue +// #2057: an expensive render on the main thread starves input dispatch and +// Flutter's raster). All Wayland protocol state stays main-thread; the worker +// only ever touches the EGL surface. The subsurface is desynchronized // so its commits are independent of the parent's frame loop; position and // stacking, however, are *parent* state and only take effect on a parent // commit, which is why SetRect() asks the view to redraw. @@ -108,7 +113,7 @@ class WaylandVideoSurface { // toplevel's frame, matching what the Dart side sends via setVideoRect. void SetRect(int32_t x, int32_t y, int32_t width, int32_t height, int32_t scale); - // Hides the plane by attaching a null buffer. The next Present() re-shows it. + // Hides the plane by attaching a null buffer. The next present re-shows it. void SetVisible(bool visible); bool visible() const { return visible_; } @@ -141,11 +146,23 @@ class WaylandVideoSurface { // it real. void SetForcedRenderCallback(std::function callback) { on_forced_render_ = std::move(callback); } - // Presents whatever was rendered into the EGL surface. No-op while hidden, - // while a frame is still pending, or while a colour transition is staged — - // the last being the one case a caller cannot read off the plane's visible - // state, so see hdr_transition_staged(). - bool Present(); + // First half of a present: the gates, and the frame-callback request that + // must precede the commit eglSwapBuffers performs so the callback belongs to + // this frame. Returns false while the plane must not present - hidden, no + // EGL surface, a frame still unacknowledged, or a colour transition staged + // (the last being the one case a caller cannot read off the plane's visible + // state, so see hdr_transition_staged()). + // + // On true, the plane is reserved for the caller's render + eglSwapBuffers - + // frame_pending_ holds every later prepare off - and CompletePresent() must + // follow on the main thread once the swap's result is known. + bool PreparePresent(); + + // Second half, on the main thread, with |swapped| = the eglSwapBuffers + // result. Owns the first-frame scale flush and the ack watchdog, and + // re-detaches the buffer when the plane was hidden or lost its rect while + // the swap was in flight. Returns whether the frame truly presented. + bool CompletePresent(bool swapped); // True when this plane can be described as HDR at all: the compositor offers // a parametric image-description creator, accepts the perceptual render @@ -183,7 +200,7 @@ class WaylandVideoSurface { // Stages a colour change. It has to be two-phase; apply_hdr_state in // mpv_plugin.cc tells that story in full. In outline: BeginHdrTransition - // stages and validates the description and holds Present() while it does, the + // stages and validates the description and holds presents while it does, the // caller switches mpv once it settles, and CommitHdrTransition attaches the // state and releases the hold so the first buffer rendered in the new colour // space is the one that carries it. Abort backs out and changes nothing. @@ -211,7 +228,7 @@ class WaylandVideoSurface { // colour state is left exactly as it was. Ignores a stale token. void AbortHdrTransition(uint64_t token); - // True while a transition is staged, i.e. while Present() is being held. + // True while a transition is staged, i.e. while presents are being held. bool hdr_transition_staged() const { return transition_staged_; } // Drops any staged transition and unsets the description immediately. @@ -236,7 +253,7 @@ class WaylandVideoSurface { // Bounds each half of a staged transition: first the compositor's verdict on // the image description, then the caller's mpv leg deciding to commit or - // abort. Present() and the plugin's render path are held across *both*, so it + // abort. PreparePresent() and the plugin's render path are held across *both*, so it // is re-armed rather than cancelled when the compositor answers - the second // wait is the longer one and has no timeout of its own. Public because the // plugin's own mpv-leg timeout (mpv_plugin.cc) shares this horizon: the two @@ -286,7 +303,7 @@ class WaylandVideoSurface { // Bounds the frame-acknowledgement wait. A compositor is entitled to stop // acknowledging frames for an occluded or minimized surface - wlroots // lineage compositors (Hyprland) and KWin do exactly that - and - // frame_pending_ is the only latch between Present() and the frame callback. + // frame_pending_ is the only latch between a present and the frame callback. // Without a bound, one missed wl_callback freezes the plane on its last // buffer for good: every later render bails on frame_pending(), and nothing // else clears it. The watchdog withdraws the dead callback and asks for a @@ -417,7 +434,7 @@ class WaylandVideoSurface { // one: set_image_description copies, so the object is destroyed immediately // after it is handed over. wp_image_description_v1* staged_description_ = nullptr; - // A transition is staged: Present() is held, and Commit or Abort will release + // A transition is staged: presents are held, and Commit or Abort will release // it. `staged_describe_` is what Commit will apply, and `transition_token_` is // what Commit and Abort must match to act on it. bool transition_staged_ = false;