Skip to content

fix(graphics): read the DWT variant from REGION flags #2085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Ki Hyun Park (kihyun1998) wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(graphics): read the DWT variant from REGION flags #2085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
395c0a5
fix(graphics): read the DWT variant from REGION flags
kihyun1998 Oct 7, 2026
8f76971
refactor(graphics): stop writing the surface DWT flag per REGION
kihyun1998 Oct 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 45 additions & 82 deletions crates/ironrdp-egfx/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,9 +712,8 @@ impl GraphicsPipelineClient {
}
// The Progressive decoder is deliberately NOT reset here either. Its context lifetime
// is driven by DeleteEncodingContext and DeleteSurface; MS-RDPEGFX 3.3.5.14 only
// resizes the Graphics Output Buffer. Windows establishes a codec context once and
// never re-sends SYNC + CONTEXT afterwards, so dropping it here makes every later
// payload fail with MissingBlock("CONTEXT").
// resizes the Graphics Output Buffer. Dropping it here would discard the tile state
// that later upgrade passes refine.
// The ClearCodec decoder is deliberately NOT reset here. MS-RDPEGFX 3.3.5.14 only
// resizes the Graphics Output Buffer; cache lifetime is driven by the stream instead,
// through CLEARCODEC_FLAG_CACHE_RESET (2.2.4.1), which ClearCodecDecoder::decode
Expand Down Expand Up @@ -1433,6 +1432,8 @@ fn copy_frame_region(frame_data: &[u8], frame_width: u32, region: &ExclusiveRect
mod tests {
use std::sync::{Arc, Mutex};

use ironrdp_pdu::codecs::rfx::progressive::TILE_FLAG_DIFFERENCE;

use super::*;

struct TestHandler;
Expand Down Expand Up @@ -1984,45 +1985,7 @@ mod tests {
}))
}

fn progressive_context_stream(with_context: bool) -> Vec<u8> {
use ironrdp_pdu::codecs::rfx::RfxRectangle;
use ironrdp_pdu::codecs::rfx::progressive::{
ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, ProgressiveFrameEndPdu,
ProgressiveRegion, ProgressiveSyncPdu, encode_progressive_stream,
};

let mut blocks = Vec::new();
if with_context {
blocks.push(ProgressiveBlock::Sync(ProgressiveSyncPdu));
blocks.push(ProgressiveBlock::Context(ProgressiveContextPdu {
context_id: 0,
tile_size: 0x0040,
flags: 0,
}));
}
blocks.push(ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu {
frame_index: 0,
region_count: 1,
}));
blocks.push(ProgressiveBlock::Region(ProgressiveRegion {
tile_size: 0x40,
rects: vec![RfxRectangle {
x: 0,
y: 0,
width: 64,
height: 64,
}],
quant_vals: vec![],
quant_prog_vals: vec![],
flags: 0,
tiles: vec![],
}));
blocks.push(ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu));

encode_progressive_stream(&blocks).unwrap()
}

fn progressive_tile_stream(tile_x: u16, tile_y: u16, rect_width: u16, rect_height: u16) -> Vec<u8> {
fn progressive_tile_stream(tile_x: u16, tile_y: u16, rect_width: u16, rect_height: u16, tile_flags: u8) -> Vec<u8> {
use ironrdp_graphics::progressive::{COEFFICIENTS_PER_COMPONENT, encode_first_pass};
use ironrdp_pdu::codecs::rfx::RfxRectangle;
use ironrdp_pdu::codecs::rfx::progressive::{
Expand Down Expand Up @@ -2072,7 +2035,7 @@ mod tests {
quant_idx_cr: 0,
x_idx: tile_x,
y_idx: tile_y,
flags: 0,
flags: tile_flags,
y_data: component_data,
cb_data: component_data,
cr_data: component_data,
Expand Down Expand Up @@ -2143,7 +2106,7 @@ mod tests {
frame_id: 2,
}))
.unwrap();
wire_progressive(&mut client, progressive_tile_stream(1, 1, 36, 36)).unwrap();
wire_progressive(&mut client, progressive_tile_stream(1, 1, 36, 36, 0)).unwrap();
client
.handle_pdu(GfxPdu::EndFrame(crate::pdu::EndFramePdu { frame_id: 2 }))
.unwrap();
Expand Down Expand Up @@ -2327,59 +2290,57 @@ mod tests {
assert!(client.drain_output().is_empty());
}

fn assert_progressive_context_is_deleted(clear: impl FnOnce(&mut GraphicsPipelineClient)) {
/// Sends a tile, runs `step`, then sends a difference tile, which needs the reference the first
/// tile left on its surface.
fn wire_difference_tile_after(step: impl FnOnce(&mut GraphicsPipelineClient)) -> PduResult<Vec<DvcMessage>> {
let mut client = progressive_client();
wire_progressive(&mut client, progressive_context_stream(true)).unwrap();
clear(&mut client);
assert!(wire_progressive(&mut client, progressive_context_stream(false)).is_err());
wire_progressive(&mut client, progressive_tile_stream(0, 0, 64, 64, 0)).unwrap();
step(&mut client);
wire_progressive(&mut client, progressive_tile_stream(0, 0, 64, 64, TILE_FLAG_DIFFERENCE))
}

#[test]
fn progressive_context_survives_graphics_reset() {
let mut client = progressive_client();
wire_progressive(&mut client, progressive_context_stream(true)).unwrap();

client
.handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu {
width: 64,
height: 64,
monitors: vec![],
}))
.unwrap();
client
.handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu {
surface_id: 1,
width: 64,
height: 64,
pixel_format: PixelFormat::XRgb,
}))
.unwrap();
let result = wire_difference_tile_after(|client| {
client
.handle_pdu(GfxPdu::ResetGraphics(crate::pdu::ResetGraphicsPdu {
width: 64,
height: 64,
monitors: vec![],
}))
.unwrap();
client
.handle_pdu(GfxPdu::CreateSurface(crate::pdu::CreateSurfacePdu {
surface_id: 1,
width: 64,
height: 64,
pixel_format: PixelFormat::XRgb,
}))
.unwrap();
});

// Windows never re-sends SYNC + CONTEXT after a reset, so a CONTEXT-less
// continuation has to keep decoding.
assert!(wire_progressive(&mut client, progressive_context_stream(false)).is_ok());
assert!(result.is_ok());
}

#[test]
fn progressive_context_is_deleted_with_encoding_context() {
let mut client = progressive_client();
wire_progressive(&mut client, progressive_context_stream(true)).unwrap();
client
.handle_pdu(GfxPdu::DeleteEncodingContext(DeleteEncodingContextPdu {
surface_id: 1,
codec_context_id: 7,
}))
.unwrap();
// The context's tiles are gone, but its surface keeps the sub-band reference. Windows
// deletes a codec context as it opens the next one.
let result = wire_difference_tile_after(|client| {
client
.handle_pdu(GfxPdu::DeleteEncodingContext(DeleteEncodingContextPdu {
surface_id: 1,
codec_context_id: 7,
}))
.unwrap();
});

// The context's tiles are gone, but the surface survives and keeps the band layout it
// was given, so a payload reusing the id decodes from scratch. Windows deletes a codec
// context as it opens the next one and never repeats SYNC + CONTEXT.
assert!(wire_progressive(&mut client, progressive_context_stream(false)).is_ok());
assert!(result.is_ok());
}

#[test]
fn progressive_context_is_deleted_with_surface() {
assert_progressive_context_is_deleted(|client| {
let result = wire_difference_tile_after(|client| {
client
.handle_pdu(GfxPdu::DeleteSurface(crate::pdu::DeleteSurfacePdu { surface_id: 1 }))
.unwrap();
Expand All @@ -2392,6 +2353,8 @@ mod tests {
}))
.unwrap();
});

assert!(result.is_err());
}

// ========================================================================
Expand Down
63 changes: 13 additions & 50 deletions crates/ironrdp-graphics/src/progressive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1063,7 +1063,7 @@ pub struct SurfaceTiles {
pub tiles_wide: u16,
/// Height of the surface in tiles.
pub tiles_high: u16,
/// Whether the associated context uses reduce-extrapolate DWT.
/// DWT variant given to tiles that [`Self::get_or_create`] allocates; a first pass sets each tile's own.
pub use_reduce_extrapolate: bool,
/// Tile storage, indexed by `y_idx * tiles_wide + x_idx`.
/// `None` entries haven't received any progressive data yet.
Expand Down Expand Up @@ -1289,7 +1289,6 @@ pub struct ProgressiveDecoder {
references: BTreeMap<SubBandDiffingTileKey, DecDwtQ>,
frame_tiles: BTreeMap<(u16, u32), BTreeSet<(u16, u16)>>,
frame_active: bool,
surface_context_flags: BTreeMap<u16, bool>,
}

impl ProgressiveDecoder {
Expand All @@ -1300,7 +1299,6 @@ impl ProgressiveDecoder {
references: BTreeMap::new(),
frame_tiles: BTreeMap::new(),
frame_active: false,
surface_context_flags: BTreeMap::new(),
}
}

Expand Down Expand Up @@ -1339,38 +1337,6 @@ impl ProgressiveDecoder {

let blocks = decode_progressive_stream(bitmap_data)?;

// Extract the band-layout flag from the CONTEXT block when present.
// Per MS-RDPEGFX 2.2.4.2 the SYNC + CONTEXT blocks establish a codec
// context once (keyed by `(surface_id, codec_context_id)`) and are not
// required to be
// repeated on subsequent frames that reference the same context.
// Real-world servers (xrdp, GNOME Remote Desktop) omit the CONTEXT
// block on every frame after the first one that established the
// context. The strict requirement rejected each of those frames with
// `MissingBlock("CONTEXT")`, freezing the image on the coarse first
// pass.
//
// Fall back to the value stored when the context was first created, then to the last
// one this surface described: Windows opens a new codec context id mid-session,
// deletes the previous one, and never repeats SYNC + CONTEXT, so a per-context lookup
// alone rejects the new context. The retained value is scoped to its surface and
// released with it. Only error when no source is available at all.
let signalled = blocks.iter().find_map(|block| match block {
ProgressiveBlock::Context(ctx) => Some(ctx.uses_reduce_extrapolate()),
_ => None,
});
if let Some(flag) = signalled {
self.surface_context_flags.insert(surface_id, flag);
}
let use_reduce_extrapolate = signalled
.or_else(|| {
self.contexts
.get(&(surface_id, codec_context_id))
.map(|c| c.surface.use_reduce_extrapolate)
})
.or_else(|| self.surface_context_flags.get(&surface_id).copied())
.ok_or(ProgressiveDecodeError::MissingBlock("CONTEXT"))?;

// Direct users of the decoder get one self-contained frame per call.
// The EGFX client brackets multiple payloads with begin_frame/end_frame.
if !self.frame_active {
Expand All @@ -1383,7 +1349,7 @@ impl ProgressiveDecoder {
let context = match contexts.entry((surface_id, codec_context_id)) {
Entry::Occupied(e) => e.into_mut(),
Entry::Vacant(e) => {
let surface = SurfaceTiles::new(surface_width, surface_height, use_reduce_extrapolate)?;
let surface = SurfaceTiles::new(surface_width, surface_height, false)?;
e.insert(ProgressiveContext { surface })
}
};
Expand All @@ -1394,9 +1360,8 @@ impl ProgressiveDecoder {
let surface_resized =
context.surface.tiles_wide != expected_wide || context.surface.tiles_high != expected_high;
if surface_resized {
context.surface = SurfaceTiles::new(surface_width, surface_height, use_reduce_extrapolate)?;
context.surface = SurfaceTiles::new(surface_width, surface_height, false)?;
}
context.surface.use_reduce_extrapolate = use_reduce_extrapolate;

let frame_tiles = all_frame_tiles.entry((surface_id, codec_context_id)).or_default();
if surface_resized {
Expand Down Expand Up @@ -1426,6 +1391,9 @@ impl ProgressiveDecoder {
_ => continue,
};

// Each REGION names its own DWT variant (MS-RDPEGFX 2.2.4.2.1.5).
let use_reduce_extrapolate = region.uses_reduce_extrapolate();

let mut region_tiles = BTreeMap::new();
for tile_block in &region.tiles {
let tiles = decode_tile_block(
Expand Down Expand Up @@ -1544,7 +1512,6 @@ impl ProgressiveDecoder {
.retain(|(reference_surface_id, _, _), _| *reference_surface_id != surface_id);
self.frame_tiles
.retain(|(context_surface_id, _), _| *context_surface_id != surface_id);
self.surface_context_flags.remove(&surface_id);
}

/// Reset codec-context state while retaining surface sub-band references.
Expand Down Expand Up @@ -2249,19 +2216,15 @@ mod tests {
}

#[test]
fn decoder_context_fallback_is_scoped_by_surface() {
fn decoder_does_not_require_a_context_block() {
// MS-RDPEGFX 2.2.4.2.1.4 makes RFX_PROGRESSIVE_CONTEXT optional.
let mut decoder = ProgressiveDecoder::new();
let stream_with_context = minimal_progressive_stream(true);
let stream_without_context = minimal_progressive_stream(false);

assert!(decoder.decode_bitmap(1, 0, 640, 480, &stream_with_context).is_ok());
assert!(matches!(
decoder.decode_bitmap(2, 0, 640, 480, &stream_without_context),
Err(ProgressiveDecodeError::MissingBlock("CONTEXT"))
));

assert!(decoder.decode_bitmap(2, 0, 640, 480, &stream_with_context).is_ok());
assert!(decoder.decode_bitmap(2, 0, 640, 480, &stream_without_context).is_ok());
assert!(
decoder
.decode_bitmap(1, 0, 640, 480, &minimal_progressive_stream(false))
.is_ok()
);
}

fn rect(x: u16, y: u16, width: u16, height: u16) -> ironrdp_pdu::codecs::rfx::RfxRectangle {
Expand Down
25 changes: 19 additions & 6 deletions crates/ironrdp-pdu/src/codecs/rfx/progressive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,9 +394,20 @@ pub struct ProgressiveContextPdu {
pub flags: u8,
}

/// Bit 0 of context flags: use reduce-extrapolate DWT.
/// RFX_DWT_REDUCE_EXTRAPOLATE in [2.2.4.2.1.5] RFX_PROGRESSIVE_REGION flags.
///
/// Indicates that the discrete wavelet transform (DWT) uses the "Reduce-Extrapolate" method.
///
/// [2.2.4.2.1.5]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/ffa9dcfc-610c-4cc5-86ba-d9435cfb37aa
pub const FLAG_DWT_REDUCE_EXTRAPOLATE: u8 = 0x01;

/// RFX_SUBBAND_DIFFING in [2.2.4.2.1.4] RFX_PROGRESSIVE_CONTEXT flags.
///
/// Indicates that sub-band diffing is enabled.
///
/// [2.2.4.2.1.4]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/89c2eaef-6bd1-4cb1-a80e-bfc161ccdbbf
pub const CONTEXT_FLAG_SUBBAND_DIFFING: u8 = 0x01;

/// RFX_TILE_DIFFERENCE in TILE_SIMPLE and TILE_FIRST flags.
///
/// The tile payload contains DWT coefficient deltas from the retained reference
Expand All @@ -407,9 +418,12 @@ impl ProgressiveContextPdu {
const NAME: &'static str = "ProgressiveContext";
const FIXED_PART_SIZE: usize = 1 /* ctxId */ + 2 /* tileSize */ + 1 /* flags */;

/// Whether the reduce-extrapolate DWT variant is selected.
/// Whether bit 0 of the CONTEXT flags is set.
///
/// That bit is RFX_SUBBAND_DIFFING, not the DWT variant.
#[deprecated(note = "the DWT variant is `ProgressiveRegion::uses_reduce_extrapolate`")]
pub fn uses_reduce_extrapolate(&self) -> bool {
self.flags & FLAG_DWT_REDUCE_EXTRAPOLATE != 0
self.flags & CONTEXT_FLAG_SUBBAND_DIFFING != 0
}
}

Expand Down Expand Up @@ -1151,9 +1165,8 @@ mod tests {
let original = ProgressiveContextPdu {
context_id: 0,
tile_size: 0x0040,
flags: FLAG_DWT_REDUCE_EXTRAPOLATE,
flags: CONTEXT_FLAG_SUBBAND_DIFFING,
};
assert!(original.uses_reduce_extrapolate());

let mut buf = [0u8; ProgressiveContextPdu::FIXED_PART_SIZE];
original.encode(&mut WriteCursor::new(&mut buf)).unwrap();
Expand Down Expand Up @@ -1290,7 +1303,7 @@ mod tests {
ProgressiveBlock::Context(ProgressiveContextPdu {
context_id: 0,
tile_size: 0x0040,
flags: FLAG_DWT_REDUCE_EXTRAPOLATE,
flags: CONTEXT_FLAG_SUBBAND_DIFFING,
}),
ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu {
frame_index: 0,
Expand Down
Loading
Loading