From 703e60c85ad54356399df4c851ec6ac563fc89df Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Sat, 18 Jul 2026 20:45:01 +0200 Subject: [PATCH] Review: identify and resolve problem with sync logic ''Review supported by Claude Code'' Control flow logic was scrutinised, and a race identified in the solution to pass on the display action to the GUI thread. Reconsidering the design, cross-thread synchronisation is now hinged rather on a single atomic buffer pointer, where the GUI thread notifies back completion of display work by setting that pointer to NULL. Overall this simplifies the control logic ---- As an aside, I setup an outline of topics for documentation in `DemoSpec.txt` and then instructed Claude to fill in the paragraphs, based on insight gained as part of the code validation / review. Furthermore, I requested to generate some diagrams that could be useful later when preparting the presentation --- FrOSCon26/FrOSCon.26.mm | 143 ++++++++++++++- FrOSCon26/examples/demo-uring.cpp | 120 ++++++------ FrOSCon26/research/DemoSpec.txt | 173 +++++++++++++++++- FrOSCon26/research/diagrams/ring-buffer.svg | 111 +++++++++++ FrOSCon26/research/diagrams/state-engine.dot | 34 ++++ FrOSCon26/research/diagrams/state-engine.svg | 83 +++++++++ .../research/diagrams/thread-handoff.dot | 56 ++++++ .../research/diagrams/thread-handoff.svg | 123 +++++++++++++ 8 files changed, 772 insertions(+), 71 deletions(-) create mode 100644 FrOSCon26/research/diagrams/ring-buffer.svg create mode 100644 FrOSCon26/research/diagrams/state-engine.dot create mode 100644 FrOSCon26/research/diagrams/state-engine.svg create mode 100644 FrOSCon26/research/diagrams/thread-handoff.dot create mode 100644 FrOSCon26/research/diagrams/thread-handoff.svg diff --git a/FrOSCon26/FrOSCon.26.mm b/FrOSCon26/FrOSCon.26.mm index 8b5da0f..a8611dc 100644 --- a/FrOSCon26/FrOSCon.26.mm +++ b/FrOSCon26/FrOSCon.26.mm @@ -1984,7 +1984,10 @@ - + + + + @@ -2007,8 +2010,12 @@

- - + + + + + + @@ -2477,7 +2484,7 @@ - + @@ -2488,8 +2495,10 @@ + + @@ -2832,6 +2841,132 @@ + + + + + + + + + + + + + + + + +

+ rein zufällig funktioniert es aber trotzdem +

+ + + + + +

+ und zwar, wie Claudes ausführt, weil unter Linux die Lese / Schreib-Kernel-Calls einen Syncrhonisationspunkt darstellen. Da der Glib::Dispatcher über einen Pipe-Write/Read implementiert ist, führt bereits das Dispatch zu einer vollen Barrier, und daher ist es unter Linux ausgeschlossen, daß der GUI-Thread noch einen alten, verschleppten Cursor-State sieht. +

+

+ +

+

+ Das gilt aber nicht allgemein und für andere Betriebssysteme und ist mithin nicht durch den C++-Standard gedeckt. Deshalb beherrscht diese Lösung nicht das Problem, und die dargestellte Begründung ist nicht stichhaltig. +

+ + + + + + + + + + + + + + + + + + + + + + + +

+ weiterer theoretischer Race: die playing-Steuervariable +

+ + + + + + + +

+ dazu müßte das GUI sofort terminieren, während der Proactor-Thread grade erst startet ⟹ dann würde dort in der Loop ohne weiteren Check playing_ = true gesetzt, und damit ginge die Shutdown-Signalisierung verloren +

+ + + + + + + + + + + + + +

+ Hinweis: in Prod-Code sollte man jedes io_uring_get_sqe() != nullptr  prüfen +

+ + + + + +

+ Begründung: im Moment kann das nicht passieren, da die Queue ausreichend dimensioniert ist auf RING_SIZ + 1.  Sollte sich sowas aber unter Maintainnance ändern, dann könnte man in einen Segfault laufen! (Ja das sehe ich genauso und hätte es in Prod-Code tatsächlich in eine Hilfsfunktion gepackt; hier aber habe ich bewußt aus didaktischen Gründen darauf verzichtet) +

+ + + + + + + + +

+ außerdem habe ich vergessen: io_uring_queue_exit(&uRing); +

+ + + + + + + + + + + +

+ ...da der Konversations-Status ein komplettes Bild der Steuerlogik enthält, bietet sich das gradezu an; stilistisch überarbeiten kann ich den Text später immer noch +

+ + + + + + + + diff --git a/FrOSCon26/examples/demo-uring.cpp b/FrOSCon26/examples/demo-uring.cpp index c7291a8..61177d4 100644 --- a/FrOSCon26/examples/demo-uring.cpp +++ b/FrOSCon26/examples/demo-uring.cpp @@ -56,8 +56,11 @@ namespace { * and uses them cyclically, first adding new tasks to read a frame-block * from the file, receiving completed tasks to hand-over the next frame * driven by a frame-beat timer, that is also based on IO_URING. - * Each buffer »slot« is associated with a #BuffState flag, implemented - * as `std::atomic` to also serve for cross-thread visibility. + * The lifecycle of each buffer »slot« is tracked by a #BuffState flag, + * used solely within the »Proactor« thread. A single [atomic pointer](\ref toDisplay_) + * is used as baton to hand the current frame to the GUI and to signal back + * when the display was published, thereby also establishing cross-thread + * visibility barriers for the buffer contents. * @note UringVideoPlayer must be constructed in the GTK UI thread, * yet will immediately launch the »Proactor« thread from the ctor. * @remarks @@ -75,10 +78,9 @@ class UringVideoPlayer using Buffer = VideoDisplay::Buffer; enum BuffState { FREE, READ, FEED, SHOW, STOP }; - using AtomicBuffState = std::atomic; using BufferAlloc = unique_ptr; - using BuffStateAlloc = unique_ptr; + using BuffStateAlloc = unique_ptr; /** * Connector to the VideoDisplay for dispatching video data. @@ -98,18 +100,14 @@ class UringVideoPlayer BufferDispatcher (UringVideoPlayer& player) : player_{player} { } - - private: - Buffer& getCurrentFrame(); - AtomicBuffState& getCurrentFrameState(); }; size_t nextRead_{0}; size_t nextShow_{0}; - size_t currShow_{0}; BufferAlloc buffer_{}; BuffStateAlloc buffState_{}; - std::atomic_bool playing_{false}; + std::atomic toDisplay_{nullptr}; + std::atomic_bool playing_{true}; BufferDispatcher dispatchFrame_; VideoDisplay videoDisplayer_; std::thread proactorThread_; @@ -147,12 +145,12 @@ UringVideoPlayer::playVideo() __FAIL ("open file \""+RAW_VIDEO_INPUT_FILE+"\""); // Allocate several data buffers (used as ring) - buffer_ = make_unique (RING_SIZ); - buffState_ = make_unique (RING_SIZ); + buffer_ = make_unique (RING_SIZ); + buffState_ = make_unique (RING_SIZ); // initialise uRing Kernel interface io_uring uRing; - io_uring_queue_init (RING_SIZ+1, &uRing, /*flags*/ 0 ); // Note: one additional »slot« for the timer event(s) + io_uring_queue_init (RING_SIZ+1, &uRing, /*flags*/ 0 ); // Note: allocate one additional »slot« for the timer event(s) // Setup periodic frame beat, firing infinitely const auto TICK_TAG = UINT64_MAX; // special marker for timer events (differs from possible frame numbers) @@ -165,7 +163,6 @@ UringVideoPlayer::playVideo() io_uring_submit (&uRing); /* =============== proactor-loop =============== */ - playing_ = true; while (playing_.load (std::memory_order_acquire)) { io_uring_cqe* completion; @@ -188,6 +185,7 @@ UringVideoPlayer::playVideo() io_uring_cqe_seen (&uRing, completion); } close (inputFD); + io_uring_queue_exit (&uRing); } @@ -212,7 +210,7 @@ UringVideoPlayer::addNextReadSubmissions (io_uring& uRing, int inputFD) ,BUFFSIZ * srcOffset ); io_uring_sqe_set_data64 (readSubmission, nextRead_); - buffState_[nextRead_ % RING_SIZ].store (READ, std::memory_order_relaxed); + buffState_[nextRead_ % RING_SIZ] = READ; timings.markStart (nextRead_); ++nextRead_; } @@ -232,84 +230,74 @@ UringVideoPlayer::handleCompletedRead (io_uring_cqe& completion) timings.markStop(frameNr); if (completion.res != sizeof(Buffer)) - state.store(STOP, std::memory_order_relaxed); // Insufficient read, possibly end-of-file => signal stop of playback (to ourselves) + state = STOP; // Insufficient read, possibly end-of-file => signal stop of playback (to ourselves) else - state.store(FEED, std::memory_order_relaxed); // Success; frame data is in the buffer => mark ready for use at the next tick event + state = FEED; // Success; frame data is in the buffer => mark ready for use at the next tick event } /** * At each timer tick: see if the next frame can be displayed. - * @remark determine the buffer slot where the frame data was placed, - * based on the cursor for the next frame to display, wrapping - * through the ring of buffers. Depending on the state marked - * for this buffer, the frame can be displayed if the data - * was already loaded, or otherwise the frame will slip - * to the next regular time tick. + * @remark the cursor #nextShow_ walks the ring of buffers. It rests on the slot + * currently lent to the GUI-thread (marked #SHOW) until the latter did signal + * completion by clearing the atomic #toDisplay_. Only then is the slot reclaimed + * (#FREE) and the cursor advanced. The next slot reached thusly is displayed if + * its data was already loaded (#FEED); otherwise the frame slips forward to + * the next regular time tick. + * Thus #toDisplay_ also provides the back-pressure: a new frame is published + * only once the previous one was displayed and released. */ void UringVideoPlayer::maybeDisplayFrame() { - auto prevState = buffState_[currShow_ % RING_SIZ].load (std::memory_order_acquire); - auto nextState = buffState_[nextShow_ % RING_SIZ].load (std::memory_order_relaxed); - - if (nextState == FEED and prevState != SHOW) - { // can hand over this next frame to the VideoDisplay in the GUI-thread - currShow_ = nextShow_; + // check and possibly reclaim previously displayed buffer + if (SHOW == buffState_[nextShow_ % RING_SIZ] and + nullptr == toDisplay_.load (std::memory_order_acquire)) // read barrier + { // GUI thread did mark completion + buffState_[nextShow_ % RING_SIZ] = FREE; ++nextShow_; - // mark state to wait for display completion; this also creates a write barrier - buffState_[currShow_ % RING_SIZ].store (SHOW, std::memory_order_release); - // Notify GUI thread via Glib::Dispatcher - dispatchFrame_(); - - if (0 == currShow_ % EMA_PERIOD) - std::cout << "Frame #"< +toDisplay_` — which _is_ the payload rather than a key to look it up: + +[cols="1,3"] +|=== +| `toDisplay_ != nullptr` | a frame is currently lent to the GUI thread +| `toDisplay_ == nullptr` | the Proactor owns the slot again and may reuse it +|=== + +The Proactor publishes with a *release*-store; the GUI picks the pointer up with an *acquire*-load +and thereby is guaranteed to see the buffer contents written before the store. When finished, the +GUI writes `nullptr` with a *release*-store, which the Proactor observes with an *acquire*-load +before reclaiming the slot. This is the classic single-slot, single-producer/single-consumer +_mailbox_ handoff: the two release/acquire pairs establish the _happens-before_ relations that carry +buffer-content visibility in both directions. Everything else — the `BuffState` array and the frame +cursors — is confined to the Proactor thread alone and therefore needs no synchronisation at all. + +Buffer data structure +~~~~~~~~~~~~~~~~~~~~~ +A fixed set of `RING_SIZ` frame-sized buffers is held in a `unique_ptr` and used as a +_ring_: the slot for a frame is simply `frameNr % RING_SIZ`. Frame numbers themselves are monotonic, +unbounded counters — `nextRead_` for the production side and `nextShow_` for the consumption side — +so the modulo maps an ever-growing frame index onto the small cyclic set of buffers. The same running +frame number does triple duty: as the SQE `user_data` tag that later identifies the completed read, as +the key of the per-slot IO timer, and (via modulo) as the slot index. The io_uring queues are sized +`RING_SIZ+1`, the one extra entry accommodating the always-present timer event. + +.Snapshot of the ring: the two cursors chase each other, the gap holds the in-flight frames +image::diagrams/ring-buffer.svg[Ring-buffer snapshot with production and consumption cursors,align="center"] + +Tracking state of a buffer slot +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Each slot carries a `BuffState` describing where in its life-cycle it currently is: + + FREE -> READ -> FEED -> SHOW -> FREE (with STOP as terminal state) + +[cols="1,4"] +|=== +| `FREE` | available; `addNextReadSubmissions()` may submit a read into it +| `READ` | a read has been submitted and is in flight +| `FEED` | the read completed successfully; frame data is ready to be shown +| `SHOW` | the frame has been handed to the GUI thread (the single slot currently lent out) +| `STOP` | the read fell short (end-of-file); signals playback to stop +|=== + +Because this array is written and read _only_ from the Proactor thread, it is a plain, non-atomic +array. The `SHOW` marker records _which_ slot is presently on loan to the GUI, complementing the +`toDisplay_` baton which records _whether_ it has come back yet. + +.The life-cycle of a single buffer slot (all transitions driven by the Proactor thread) +image::diagrams/state-engine.svg[Buffer-slot state engine,align="center"] + +Control flow and backpressure +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Production and consumption advance independently, each with its own cursor. Production +(`addNextReadSubmissions()`) greedily fills _every_ `FREE` slot ahead with read submissions, tagging +each with its frame number. Consumption happens once per timer tick in `maybeDisplayFrame()`: first +a completed frame is _reclaimed_, then the next frame is _published_ if its data has arrived. + +The essential back-pressure is that the single show-cursor does not advance while a frame is still out +with the GUI (`toDisplay_` non-null). No new frame is published until the previous one has been +displayed and released, and — since reads only ever refill `FREE` slots — a lagging consumer naturally +throttles production. A frame whose read has not completed in time, or whose predecessor the GUI has +not yet released, simply _slips_ to the next tick. + +[source,cpp] +-------------------------------------------------------------------------- +void +UringVideoPlayer::maybeDisplayFrame() +{ + // reclaim previously displayed buffer + if (SHOW == buffState_[nextShow_ % RING_SIZ] and + nullptr == toDisplay_.load (memory_order_acquire)) // <1> + { + buffState_[nextShow_ % RING_SIZ] = FREE; + ++nextShow_; // <2> + } + + switch (buffState_[nextShow_ % RING_SIZ]) + { + case FEED: // <3> + buffState_[nextShow_ % RING_SIZ] = SHOW; + toDisplay_.store (& buffer_[nextShow_ % RING_SIZ] + , memory_order_release); // <4> + dispatchFrame_(); + break; + case STOP: + playing_ = false; + break; + default: // <5> + std::cout << "WARNING: Frame slipped" << std::endl; + } +} +-------------------------------------------------------------------------- +<1> reclaim only the slot lent to the GUI, and only once it cleared the baton +<2> advance the single show-cursor — this is where the back-pressure lives +<3> the frame data has arrived (`FEED`): hand it over +<4> the release-store publishes the buffer and makes its contents visible to the GUI +<5> `READ` (IO still pending) or `SHOW` (GUI not yet caught up) ⇒ the frame slips to the next tick diff --git a/FrOSCon26/research/diagrams/ring-buffer.svg b/FrOSCon26/research/diagrams/ring-buffer.svg new file mode 100644 index 0000000..305f285 --- /dev/null +++ b/FrOSCon26/research/diagrams/ring-buffer.svg @@ -0,0 +1,111 @@ + + + + + + + + + + + + + Buffers used as a ring: slot = frameNr % RING_SIZ + + + + + advances + + + + + + SHOW + frame 40 + slot 0 + + + + + FEED + frame 41 + slot 1 + + + + + READ + frame 42 + slot 2 + + + + + READ + frame 43 + slot 3 + + + + + FREE + — + slot 4 + + + + buffer_[ ] + RING_SIZ = 5 + + + + + + + nextShow_ = 40 + + + + + + + + nextRead_ = 44 + + + + + + + States: + + FREE + + READ + + FEED + + SHOW + + STOP + + + Between the two cursors lie the frames currently in flight (READ) or ready (FEED); + + + the gap can never exceed RING_SIZ — that is where the back-pressure lives. + + diff --git a/FrOSCon26/research/diagrams/state-engine.dot b/FrOSCon26/research/diagrams/state-engine.dot new file mode 100644 index 0000000..2520ca2 --- /dev/null +++ b/FrOSCon26/research/diagrams/state-engine.dot @@ -0,0 +1,34 @@ +// Buffer-slot state engine of the io_uring video demo. +// Render: dot -Tsvg state-engine.dot -o state-engine.svg +// +// The BuffState of each ring slot is written and read ONLY by the Proactor +// thread; the single SHOW->FREE transition is triggered by the GUI thread +// clearing the atomic baton (toDisplay_), but observed and enacted here. + +digraph BuffState { + rankdir=LR; + bgcolor="transparent"; + labelloc="t"; + fontname="sans-serif"; + fontsize=14; + label="Life-cycle of one buffer »slot«"; + + node [shape=circle, fixedsize=true, width=0.9, + fontname="sans-serif", fontsize=12, style=filled, penwidth=1.4]; + edge [fontname="sans-serif", fontsize=10]; + + FREE [fillcolor="#dbe9ff"]; + READ [fillcolor="#ffedcc"]; + FEED [fillcolor="#d8f0d8"]; + SHOW [fillcolor="#e7d8ff"]; + STOP [shape=doublecircle, width=0.85, fillcolor="#ffd6d6"]; + + FREE -> READ [label=" submit read\l addNextReadSubmissions()\l"]; + READ -> FEED [label=" read completed OK\l handleCompletedRead()\l"]; + READ -> STOP [label=" short read / EOF ", fontcolor="#a00000", color="#a00000"]; + FEED -> SHOW [label=" publish frame\l maybeDisplayFrame()\l"]; + SHOW -> FREE [label=" GUI released frame\l (toDisplay_ == nullptr)\l", + fontcolor="#5a2ca0", color="#5a2ca0", constraint=false]; + + { rank=same; FREE; } +} diff --git a/FrOSCon26/research/diagrams/state-engine.svg b/FrOSCon26/research/diagrams/state-engine.svg new file mode 100644 index 0000000..ac4135b --- /dev/null +++ b/FrOSCon26/research/diagrams/state-engine.svg @@ -0,0 +1,83 @@ + + + + + + +BuffState +Life-cycle of one buffer »slot« + + +FREE + +FREE + + + +READ + +READ + + + +FREE->READ + + +  submit read +  addNextReadSubmissions() + + + +FEED + +FEED + + + +READ->FEED + + +  read completed OK +  handleCompletedRead() + + + +STOP + + +STOP + + + +READ->STOP + + +  short read / EOF   + + + +SHOW + +SHOW + + + +FEED->SHOW + + +  publish frame +  maybeDisplayFrame() + + + +SHOW->FREE + + +  GUI released frame +  (toDisplay_ == nullptr) + + + diff --git a/FrOSCon26/research/diagrams/thread-handoff.dot b/FrOSCon26/research/diagrams/thread-handoff.dot new file mode 100644 index 0000000..ad35e87 --- /dev/null +++ b/FrOSCon26/research/diagrams/thread-handoff.dot @@ -0,0 +1,56 @@ +// Cross-thread architecture and data-visibility handoff. +// Render: dot -Tsvg thread-handoff.dot -o thread-handoff.svg +// +// One direction of data flow: the Proactor thread reads file blocks into the +// ring of buffers and publishes a ready frame through the single atomic baton +// `toDisplay_`; the GUI thread picks it up, uploads it to the GPU, and hands +// the slot back. The release/acquire pairs on the baton carry buffer-content +// visibility in both directions. + +digraph handoff { + rankdir=LR; + bgcolor="transparent"; + compound=true; + fontname="sans-serif"; + node [fontname="sans-serif", fontsize=11]; + edge [fontname="sans-serif", fontsize=10]; + + file [shape=cylinder, fillcolor="#eeeeee", style=filled, label="demovideo.raw"]; + + subgraph cluster_proactor { + label=" »Proactor« thread — playVideo() loop "; + labeljust="l"; + style="filled,rounded"; fillcolor="#eef4ff"; color="#7092c0"; fontsize=12; + + wait [shape=box, style="rounded,filled", fillcolor="#ffffff", + label="io_uring_wait_cqe()\n(blocking)"]; + timer [shape=box, style="rounded,filled", fillcolor="#ffedcc", + label="frame-beat timer\n20 ms · multishot"]; + ring [shape=record, style=filled, fillcolor="#ffffff", + label="ring buffers\ (RING_SIZ)|slot|slot|slot|…"]; + } + + subgraph cluster_gui { + label=" GTK / GUI thread "; + labeljust="l"; + style="filled,rounded"; fillcolor="#eefaee"; color="#6bab6b"; fontsize=12; + + disp [shape=box, style="rounded,filled", fillcolor="#ffffff", + label="EglDisplay::displayFrame()\nretrieveFrame() / completedFrame()"]; + gpu [shape=box, style="rounded,filled", fillcolor="#ffffff", + label="GPU texture\n(glTexImage2D → draw)"]; + } + + baton [shape=box, style="filled", fillcolor="#e7d8ff", penwidth=1.6, + label="toDisplay_\nstd::atomic"]; + + file -> ring [label="io_uring read", lhead=cluster_proactor]; + timer -> wait [style=dotted, label="CQE (TICK_TAG)"]; + ring -> baton [label="publish\nstore(release)", color="#5a2ca0", fontcolor="#5a2ca0"]; + baton -> disp [label="retrieveFrame\nload(acquire)", color="#5a2ca0", fontcolor="#5a2ca0"]; + disp -> gpu [label="upload + draw"]; + disp -> baton [label="completedFrame\nstore(nullptr, release)", + style=dashed, color="#3a7a3a", fontcolor="#3a7a3a"]; + + { rank=same; baton; } +} diff --git a/FrOSCon26/research/diagrams/thread-handoff.svg b/FrOSCon26/research/diagrams/thread-handoff.svg new file mode 100644 index 0000000..b2726c1 --- /dev/null +++ b/FrOSCon26/research/diagrams/thread-handoff.svg @@ -0,0 +1,123 @@ + + + + + + +handoff + +cluster_proactor + +  »Proactor« thread  —  playVideo() loop   + + +cluster_gui + +  GTK / GUI thread   + + + +file + + +demovideo.raw + + + +ring + +ring buffers (RING_SIZ) + +slot + +slot + +slot + +… + + + +file->ring + + +io_uring read + + + +wait + +io_uring_wait_cqe() +(blocking) + + + +timer + +frame-beat timer +20 ms · multishot + + + +timer->wait + + +CQE (TICK_TAG) + + + +baton + +toDisplay_ +std::atomic<Buffer*> + + + +ring->baton + + +publish +store(release) + + + +disp + +EglDisplay::displayFrame() +retrieveFrame() / completedFrame() + + + +gpu + +GPU texture +(glTexImage2D → draw) + + + +disp->gpu + + +upload + draw + + + +disp->baton + + +completedFrame +store(nullptr, release) + + + +baton->disp + + +retrieveFrame +load(acquire) + + +