Benoît Cortier (@CBenoit): picking up the invitation from #1309.
Background
#1309's reply on the bounded-channel discussion left the snapshot-only-keeps-latest improvement for a future PR, with the constraint that the latest snapshot must properly reflect correct state. This issue proposes a shape and requests maintainer signal before any code lands.
The current output_event_sender: mpsc::Sender<RdpOutputEvent> in crates/ironrdp-client/src/rdp.rs uses bounded capacity with .await backpressure on every send (post-#1309). That preserves correctness but pays full backpressure cost on every framebuffer update, even when intermediate updates would be subsumed by the next one.
Proposal
Tag each RdpOutputEvent variant with a DropPolicy and dispatch sends through a wrapper that consults the policy:
| Policy |
Behavior |
Candidate variants |
MustDeliver |
.await always |
ConnectionFailure, Terminated |
DropOldest |
try_send; if full, discard oldest queued instance of the same variant, then send |
PointerPosition, PointerDefault, PointerHidden |
Coalesce |
Receiver-side accumulator merges queued + incoming into a single dirty-region snapshot |
Image |
Replace |
try_send; if full, replace the previous queued instance of the same variant outright |
PointerBitmap |
The load-bearing case is Coalesce on Image. Naive drop-oldest on framebuffer regions corrupts the destination because region updates are stateful relative to the previous frame. Coalesce maintains a "current dirty region" accumulator (union of dirty rects, latest pixel values per region) that gets delivered as one snapshot when the consumer wakes. The accumulator design is what the correctness constraint on the original invitation requires.
Open questions
- Coalesce shape. Receiver-side accumulator vs sender-side merge. Sender-side is simpler (no separate receiver state) but requires the sender to know the previous queued payload. Receiver-side keeps the channel event-typed but adds an accumulator owned by the consumer. Maintainer preference?
PointerBitmap policy. Arc<DecodedPointer> is cheap to clone. Replace versus a fixed-slot coalesce. Either works; preference?
PointerDefault / PointerHidden. These are cursor-state transitions, conceptually stateful, but the latest one is what matters. DropOldest covers them. Acceptable, or should they be MustDeliver?
- Library boundary. New module
crates/ironrdp-client/src/output_channel.rs (or similar), wrapper type around mpsc::Sender<RdpOutputEvent>, public API exposed via the library-side surface rather than the raw channel handle. Acceptable shape?
DropPolicy placement. A method RdpOutputEvent::drop_policy(&self) -> DropPolicy keeps the classification next to the variant definition. A const table at the wrapper side keeps the enum pure. Either works; preference?
Measurement plan (prerequisite for the implementation PR)
The implementation PR will include data; this issue commits to that.
- Workload: chatty server emitting fast framebuffer updates (rapid scrolling, animated content, or video playback). Either a real Windows host or a server-side IronRDP instance running on hardware sufficient to saturate the channel.
- Client side:
ironrdp-viewer instrumented to record per-event send timestamps, per-event consumer-receive timestamps, queue-depth at send time, and dropped/coalesced event counts.
- Metrics: P50 / P99 / P99.9 end-to-end framebuffer latency (server frame produced to viewer rendered). Cursor position end-to-end latency for
PointerPosition traffic. Total event count delivered vs total event count produced. CPU and memory deltas.
- Comparator: current
.await backpressure (master) vs proposed DropPolicy wrapper, same workload, same hardware.
- Acceptance bar: implementation PR ships only if P99 framebuffer latency improves materially under burst load AND no rendering correctness regression appears in the coalesce path.
If the preferred sequence is data-before-code, the implementation PR holds until the baseline is recorded. If code-first-with-data-following is preferred, the wrapper can ship behind a feature flag or as an off-by-default opt-in path with data following.
Out of scope for this issue
- Multi-monitor framebuffer state semantics under coalesce. The accumulator design here is single-buffer; multi-monitor needs a separate pass once the single-buffer shape settles.
- Workloads beyond framebuffer + cursor. Typing latency, RDP audio sync, clipboard latency can become follow-up workloads if the initial measurement raises specific concerns.
RdpInputEvent direction. The current input channel is mpsc::UnboundedSender<RdpInputEvent>; whether it deserves the same treatment is a separate discussion.
Refs #1309.
Benoît Cortier (@CBenoit): picking up the invitation from #1309.
Background
#1309's reply on the bounded-channel discussion left the snapshot-only-keeps-latest improvement for a future PR, with the constraint that the latest snapshot must properly reflect correct state. This issue proposes a shape and requests maintainer signal before any code lands.
The current
output_event_sender: mpsc::Sender<RdpOutputEvent>incrates/ironrdp-client/src/rdp.rsuses bounded capacity with.awaitbackpressure on every send (post-#1309). That preserves correctness but pays full backpressure cost on every framebuffer update, even when intermediate updates would be subsumed by the next one.Proposal
Tag each
RdpOutputEventvariant with aDropPolicyand dispatch sends through a wrapper that consults the policy:MustDeliver.awaitalwaysConnectionFailure,TerminatedDropOldesttry_send; if full, discard oldest queued instance of the same variant, then sendPointerPosition,PointerDefault,PointerHiddenCoalesceImageReplacetry_send; if full, replace the previous queued instance of the same variant outrightPointerBitmapThe load-bearing case is
CoalesceonImage. Naive drop-oldest on framebuffer regions corrupts the destination because region updates are stateful relative to the previous frame. Coalesce maintains a "current dirty region" accumulator (union of dirty rects, latest pixel values per region) that gets delivered as one snapshot when the consumer wakes. The accumulator design is what the correctness constraint on the original invitation requires.Open questions
PointerBitmappolicy.Arc<DecodedPointer>is cheap to clone.Replaceversus a fixed-slot coalesce. Either works; preference?PointerDefault/PointerHidden. These are cursor-state transitions, conceptually stateful, but the latest one is what matters.DropOldestcovers them. Acceptable, or should they beMustDeliver?crates/ironrdp-client/src/output_channel.rs(or similar), wrapper type aroundmpsc::Sender<RdpOutputEvent>, public API exposed via the library-side surface rather than the raw channel handle. Acceptable shape?DropPolicyplacement. A methodRdpOutputEvent::drop_policy(&self) -> DropPolicykeeps the classification next to the variant definition. A const table at the wrapper side keeps the enum pure. Either works; preference?Measurement plan (prerequisite for the implementation PR)
The implementation PR will include data; this issue commits to that.
ironrdp-viewerinstrumented to record per-event send timestamps, per-event consumer-receive timestamps, queue-depth at send time, and dropped/coalesced event counts.PointerPositiontraffic. Total event count delivered vs total event count produced. CPU and memory deltas..awaitbackpressure (master) vs proposedDropPolicywrapper, same workload, same hardware.If the preferred sequence is data-before-code, the implementation PR holds until the baseline is recorded. If code-first-with-data-following is preferred, the wrapper can ship behind a feature flag or as an off-by-default opt-in path with data following.
Out of scope for this issue
RdpInputEventdirection. The current input channel ismpsc::UnboundedSender<RdpInputEvent>; whether it deserves the same treatment is a separate discussion.Refs #1309.