From df3c1f9c66a97207039fc900a1aaa5d7548ccc9d Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Fri, 25 Sep 2026 05:14:06 -0500 Subject: [PATCH 1/2] fix(egfx): check mixed-frame tiles before queuing the frame send_mixed_frame emitted every tile unchecked: AVC420 tiles went out even with AVC disabled, and empty or out-of-surface rectangles reached the client. FreeRDP 3.32.0 rejects those before decoding the frame, which leaves the H.264 reference chain broken. Validate every tile first, queue nothing if one fails, and log the reason at trace level. A mix of AVC420 with another codec now needs the capability versions that promise it (10.4 or later, MS-RDPEGFX 2.2.3.7), reported by the new CodecCapabilities::avc420_in_mixed_frames. QuantQuality::encode returns an error for a QP above 63 instead of panicking. Also record backpressure in the mixed and Progressive senders like the others, and replace an unsourced doc claim with what the spec guarantees. --- crates/ironrdp-egfx/src/pdu/avc.rs | 13 +- crates/ironrdp-egfx/src/server.rs | 120 ++++++- .../ironrdp-testsuite-core/tests/egfx/avc.rs | 42 ++- .../tests/egfx/server.rs | 295 +++++++++++++++++- 4 files changed, 459 insertions(+), 11 deletions(-) diff --git a/crates/ironrdp-egfx/src/pdu/avc.rs b/crates/ironrdp-egfx/src/pdu/avc.rs index 67088c2d04..84077e1dc1 100644 --- a/crates/ironrdp-egfx/src/pdu/avc.rs +++ b/crates/ironrdp-egfx/src/pdu/avc.rs @@ -17,9 +17,9 @@ pub struct QuantQuality { } // Manual `Arbitrary` impl: the encoder packs `quantization_parameter` into bits 0..6 -// via `set_bits`, which panics when the value exceeds 6 bits. Mask the field to its -// wire-allowed range so fuzz inputs always round-trip through `Encode`. The other -// fields use their full type range. +// and fails when the value exceeds 6 bits. Mask the field to its wire-allowed range +// so fuzz inputs always round-trip through `Encode`. The other fields use their full +// type range. #[cfg(feature = "arbitrary")] impl<'a> arbitrary::Arbitrary<'a> for QuantQuality { fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { @@ -41,6 +41,10 @@ impl Encode for QuantQuality { fn encode(&self, dst: &mut WriteCursor<'_>) -> EncodeResult<()> { ensure_fixed_part_size!(in: dst); + if 0x3F < self.quantization_parameter { + return Err(invalid_field_err!("quantization_parameter", "must fit in 6 bits", in: dst)); + } + let mut data = 0u8; data.set_bits(0..6, self.quantization_parameter); data.set_bit(7, self.progressive); @@ -565,7 +569,8 @@ pub const fn align_to_16(dimension: u32) -> u32 { /// /// # Panics /// -/// Panics if internal encoding fails (should not happen with valid inputs). +/// Panics if a region's quantization parameter is above 63, the most the 6-bit +/// `qp` field holds. #[must_use] pub fn encode_avc420_bitmap_stream(regions: &[Avc420Region], h264_data: &[u8]) -> Vec { let rectangles: Vec = regions.iter().map(Avc420Region::to_rectangle).collect(); diff --git a/crates/ironrdp-egfx/src/server.rs b/crates/ironrdp-egfx/src/server.rs index e8b60d8ad2..ed46af0144 100644 --- a/crates/ironrdp-egfx/src/server.rs +++ b/crates/ironrdp-egfx/src/server.rs @@ -86,6 +86,18 @@ const DEFAULT_MAX_FRAMES_IN_FLIGHT: u32 = 3; /// Special queue depth value indicating client has disabled acknowledgments const SUSPEND_FRAME_ACK_QUEUE_DEPTH: u32 = 0xFFFFFFFF; +/// Highest QP in the H.264 range that MS-RDPEGFX [2.2.4.4.2] +/// (`RDPGFX_AVC420_QUANT_QUALITY`) requires for the `qp` field of an AVC420 +/// region, for 8-bit video. +/// +/// [2.2.4.4.2]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/bc54993f-2e3c-4285-b32b-58a4bf4cb02e +const MAX_AVC_QP: u8 = 51; + +/// Highest `qualityVal` of an AVC420 region (MS-RDPEGFX [2.2.4.4.2]). +/// +/// [2.2.4.4.2]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/bc54993f-2e3c-4285-b32b-58a4bf4cb02e +const MAX_AVC_QUALITY: u8 = 100; + /// Pre-encoded ZGFX-wrapped bytes for DVC transmission. /// /// `Encode::encode()` takes `&self`, but ZGFX wrapping is done in `drain_output()` @@ -719,6 +731,18 @@ pub struct CodecCapabilities { pub avc420: bool, /// AVC444 (H.264 4:4:4) is available pub avc444: bool, + /// The negotiated capability set promises AVC420 in the same frame as other codecs + /// + /// MS-RDPEGFX [2.2.3.7] (`RDPGFX_CAPSET_VERSION104`) requires that of a + /// client that did not set `AVC_DISABLED` at capability version 10.4, and + /// [2.2.3.8], [2.2.3.9] and [2.2.3.10] carry it to 10.5, 10.6 and 10.7. + /// Earlier versions make no such promise. + /// + /// [2.2.3.7]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/be5ea8da-44db-478d-b55c-d42d82f11d26 + /// [2.2.3.8]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/8fc20f1e-e63e-4b13-a546-22fba213ad83 + /// [2.2.3.9]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/8d489900-e903-4778-bb83-691c5ab719d5 + /// [2.2.3.10]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/ba94595b-04de-4fbd-8ee4-89d8ff8f5cf1 + pub avc420_in_mixed_frames: bool, /// Small cache mode pub small_cache: bool, /// Thin client mode @@ -732,24 +756,28 @@ impl CodecCapabilities { CapabilitySet::V8 { flags } => Self { avc420: false, avc444: false, + avc420_in_mixed_frames: false, small_cache: flags.contains(CapabilitiesV8Flags::SMALL_CACHE), thin_client: flags.contains(CapabilitiesV8Flags::THIN_CLIENT), }, CapabilitySet::V8_1 { flags } => Self { avc420: flags.contains(CapabilitiesV81Flags::AVC420_ENABLED), avc444: false, + avc420_in_mixed_frames: false, small_cache: flags.contains(CapabilitiesV81Flags::SMALL_CACHE), thin_client: flags.contains(CapabilitiesV81Flags::THIN_CLIENT), }, CapabilitySet::V10 { flags } | CapabilitySet::V10_2 { flags } => Self { avc420: !flags.contains(CapabilitiesV10Flags::AVC_DISABLED), avc444: !flags.contains(CapabilitiesV10Flags::AVC_DISABLED), + avc420_in_mixed_frames: false, small_cache: flags.contains(CapabilitiesV10Flags::SMALL_CACHE), thin_client: false, }, CapabilitySet::V10_1 => Self { avc420: true, avc444: true, + avc420_in_mixed_frames: false, small_cache: false, thin_client: false, }, @@ -757,6 +785,7 @@ impl CodecCapabilities { // V10.3 lacks SMALL_CACHE flag avc420: !flags.contains(CapabilitiesV103Flags::AVC_DISABLED), avc444: !flags.contains(CapabilitiesV103Flags::AVC_DISABLED), + avc420_in_mixed_frames: false, small_cache: false, thin_client: flags.contains(CapabilitiesV103Flags::AVC_THIN_CLIENT), }, @@ -766,12 +795,14 @@ impl CodecCapabilities { | CapabilitySet::V10_6Err { flags } => Self { avc420: !flags.contains(CapabilitiesV104Flags::AVC_DISABLED), avc444: !flags.contains(CapabilitiesV104Flags::AVC_DISABLED), + avc420_in_mixed_frames: !flags.contains(CapabilitiesV104Flags::AVC_DISABLED), small_cache: flags.contains(CapabilitiesV104Flags::SMALL_CACHE), thin_client: flags.contains(CapabilitiesV104Flags::AVC_THIN_CLIENT), }, CapabilitySet::V10_7 { flags } => Self { avc420: !flags.contains(CapabilitiesV107Flags::AVC_DISABLED), avc444: !flags.contains(CapabilitiesV107Flags::AVC_DISABLED), + avc420_in_mixed_frames: !flags.contains(CapabilitiesV107Flags::AVC_DISABLED), small_cache: flags.contains(CapabilitiesV107Flags::SMALL_CACHE), thin_client: flags.contains(CapabilitiesV107Flags::AVC_THIN_CLIENT), }, @@ -1432,6 +1463,11 @@ impl GraphicsPipelineServer { } } + /// Whether an exclusive rectangle is non-empty and lies inside the surface. + fn rect_fits_surface(surface: &Surface, rect: &ExclusiveRectangle) -> bool { + rect.left < rect.right && rect.top < rect.bottom && rect.right <= surface.width && rect.bottom <= surface.height + } + /// Queue an H.264 AVC420 frame for transmission /// /// Returns `Some(frame_id)` if queued, `None` if backpressure is active, @@ -1789,6 +1825,7 @@ impl GraphicsPipelineServer { return None; } if self.should_backpressure() { + self.qoe.record_backpressure(); return None; } @@ -1869,10 +1906,39 @@ impl GraphicsPipelineServer { /// ClearCodec tiles (lossless text), Progressive tiles (photos), and H.264 /// tiles (video), all sent between one `StartFrame`/`EndFrame` pair. /// - /// This matches how Azure VDI achieves its visual quality — each tile uses - /// the codec best suited to its content type. + /// MS-RDPEGFX [3.3.5.1] (processing an `RDPGFX_WIRE_TO_SURFACE_PDU_1` + /// message) has the client copy a decoded ClearCodec or AVC420 tile to the + /// surface when it processes the PDU, so where two such tiles overlap the + /// one sent later is what remains. A Progressive tile follows [3.3.5.2] + /// instead, which only says it SHOULD be copied once enough of it is + /// decoded and updated as later PDUs arrive, so the spec doesn't order it + /// against an overlapping tile. MS-RDPEGFX [2.2.3.7] requires a client that + /// did not set `AVC_DISABLED` at capability version 10.4 or later to process + /// AVC420 in the same frame as other codecs; earlier capability versions + /// make no such promise. [`CodecCapabilities::avc420_in_mixed_frames`] + /// reports whether the negotiated set makes it. /// - /// Returns `Some(frame_id)` if queued, `None` if not ready or backpressured. + /// Every tile is checked before the frame is queued. AVC420 tiles need + /// AVC420 support in the negotiated capabilities, and a frame that mixes an + /// AVC420 tile with another codec needs capability version 10.4 or later, + /// the versions that make that promise. A frame of AVC420 tiles alone is + /// not covered by it. Each AVC420 tile needs at least one region, unlike + /// [`Self::send_avc420_frame()`], which reads an empty list as the whole + /// surface: a whole-surface tile would cover the ClearCodec and AVC420 + /// tiles sent before it. Each AVC420 region and ClearCodec destination must + /// be non-empty and inside the surface. + /// AVC420 regions also need a QP of at most 51 (for 8-bit video) and a + /// quality of at most 100 (MS-RDPEGFX [2.2.4.4.2]). If any check fails + /// nothing is queued and no frame is tracked. + /// + /// Returns `Some(frame_id)` if queued, `None` if not ready, backpressured, + /// or a tile failed its checks. A refusal because of the tiles or the + /// negotiated capabilities logs its reason at trace level. + /// + /// [3.3.5.1]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/3fdfcd8d-ec4f-4b7a-8e0d-0385a840ec81 + /// [3.3.5.2]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/9791fc34-7644-4279-844f-7728ae9959c2 + /// [2.2.3.7]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/be5ea8da-44db-478d-b55c-d42d82f11d26 + /// [2.2.4.4.2]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/bc54993f-2e3c-4285-b32b-58a4bf4cb02e pub fn send_mixed_frame( &mut self, surface_id: u16, @@ -1883,15 +1949,63 @@ impl GraphicsPipelineServer { return None; } if self.should_backpressure() { + self.qoe.record_backpressure(); return None; } if tiles.is_empty() { return None; } + let is_avc420 = |tile: &MixedTilePayload| matches!(tile, MixedTilePayload::Avc420 { .. }); + if tiles.iter().any(is_avc420) { + if !self.supports_avc420() { + trace!( + reason = "AVC420 is not supported by the negotiated capabilities", + "Mixed frame refused" + ); + return None; + } + if !self.codec_caps.avc420_in_mixed_frames && tiles.iter().any(|tile| !is_avc420(tile)) { + trace!( + reason = "AVC420 beside another codec is not promised by the negotiated capabilities", + "Mixed frame refused" + ); + return None; + } + } + let surface = self.surfaces.get(surface_id)?; let pixel_format = surface.pixel_format; + let refusal = tiles.iter().find_map(|tile| match tile { + MixedTilePayload::ClearCodec { destination, .. } => (!Self::rect_fits_surface(surface, destination)) + .then_some("ClearCodec destination is empty or outside the surface"), + MixedTilePayload::RemoteFxProgressive { .. } => None, + MixedTilePayload::Avc420 { regions, .. } => { + if regions.is_empty() { + return Some("AVC420 tile has no regions"); + } + if regions + .iter() + .any(|region| !Self::rect_fits_surface(surface, ®ion.to_rectangle())) + { + return Some("AVC420 region is empty or outside the surface"); + } + if regions + .iter() + .any(|region| MAX_AVC_QP < region.quantization_parameter || MAX_AVC_QUALITY < region.quality) + { + return Some("AVC420 QP or quality is out of range"); + } + + None + } + }); + if let Some(reason) = refusal { + trace!(reason, "Mixed frame refused"); + return None; + } + let timestamp = Self::make_timestamp(timestamp_ms); let frame_id = self.frames.begin_frame(timestamp); diff --git a/crates/ironrdp-testsuite-core/tests/egfx/avc.rs b/crates/ironrdp-testsuite-core/tests/egfx/avc.rs index 6540b1f4f5..6b17c5fb80 100644 --- a/crates/ironrdp-testsuite-core/tests/egfx/avc.rs +++ b/crates/ironrdp-testsuite-core/tests/egfx/avc.rs @@ -1,5 +1,8 @@ -use ironrdp_core::{Decode as _, ReadCursor}; -use ironrdp_egfx::pdu::{Avc420BitmapStream, Avc420Region, align_to_16, annex_b_to_avc, encode_avc420_bitmap_stream}; +use ironrdp_core::{Decode as _, EncodeErrorKind, ReadCursor, encode_vec}; +use ironrdp_egfx::pdu::{ + Avc420BitmapStream, Avc420Region, QuantQuality, align_to_16, annex_b_to_avc, encode_avc420_bitmap_stream, +}; +use rstest::rstest; #[test] fn avc420_region_full_frame() { @@ -84,3 +87,38 @@ fn encode_avc420_bitmap_stream_round_trips_through_decode() { assert_eq!(decoded.quant_qual_vals.len(), 1); assert_eq!(decoded.data, &h264_data); } + +#[test] +fn quant_quality_encodes_the_largest_qp_the_field_holds() { + let quant_quality = QuantQuality { + quantization_parameter: 63, + progressive: true, + quality: 255, + }; + + let encoded = encode_vec(&quant_quality).expect("encode QuantQuality"); + + // Progressive flag in bit 7, reserved bit 6 clear, QP in bits 0 to 5. + assert_eq!(encoded, [0b1011_1111, 255]); +} + +#[rstest] +#[case::first_value_past_the_field(64)] +#[case::largest_value_of_the_type(255)] +fn quant_quality_encode_rejects_a_qp_that_does_not_fit_the_field(#[case] quantization_parameter: u8) { + let quant_quality = QuantQuality { + quantization_parameter, + progressive: false, + quality: 100, + }; + + let error = encode_vec(&quant_quality).expect_err("a QP above 63 does not fit the 6-bit field"); + + assert!(matches!( + error.kind(), + EncodeErrorKind::InvalidField { + field: "quantization_parameter", + .. + } + )); +} diff --git a/crates/ironrdp-testsuite-core/tests/egfx/server.rs b/crates/ironrdp-testsuite-core/tests/egfx/server.rs index 6bf6f5a226..8e0f034e5a 100644 --- a/crates/ironrdp-testsuite-core/tests/egfx/server.rs +++ b/crates/ironrdp-testsuite-core/tests/egfx/server.rs @@ -2,10 +2,13 @@ use ironrdp_core::{Decode as _, Encode, ReadCursor, WriteCursor, encode_vec}; use ironrdp_dvc::DvcProcessor as _; use ironrdp_egfx::pdu::{ Avc420Region, Avc444BitmapStream, CapabilitiesAdvertisePdu, CapabilitiesV8Flags, CapabilitiesV10Flags, - CapabilitiesV81Flags, CapabilitySet, Codec1Type, Encoding, FrameAcknowledgePdu, GfxPdu, PixelFormat, QueueDepth, + CapabilitiesV81Flags, CapabilitiesV103Flags, CapabilitiesV104Flags, CapabilitiesV107Flags, CapabilitySet, + Codec1Type, Codec2Type, Encoding, FrameAcknowledgePdu, GfxPdu, PixelFormat, QueueDepth, }; -use ironrdp_egfx::server::{GraphicsPipelineHandler, GraphicsPipelineServer, QoeMetrics, Surface}; +use ironrdp_egfx::server::{GraphicsPipelineHandler, GraphicsPipelineServer, MixedTilePayload, QoeMetrics, Surface}; use ironrdp_graphics::zgfx::Decompressor; +use ironrdp_pdu::geometry::ExclusiveRectangle; +use rstest::rstest; // ============================================================================ // Test Handler @@ -335,6 +338,258 @@ fn avc444v2_sender_rejects_invalid_stream_shapes_without_queueing() { } } +// ============================================================================ +// Mixed-Codec Frame Tests +// ============================================================================ + +/// A server whose client negotiated `caps`. +/// +/// Mixed frames that carry AVC420 beside another codec need capability version +/// 10.4 or later, so the tests of the per-tile checks negotiate 10.4. +fn mixed_frame_server(caps: CapabilitySet) -> (GraphicsPipelineServer, u16) { + let handler = Box::new(TestHandler::new()); + let mut server = GraphicsPipelineServer::new(handler); + let client_caps_pdu = GfxPdu::CapabilitiesAdvertise(CapabilitiesAdvertisePdu::from_typed(&[caps])); + server + .process(0, &encode_pdu(&client_caps_pdu)) + .expect("process capabilities"); + let surface_id = server.create_surface(64, 64).expect("create surface"); + server.drain_output(); + (server, surface_id) +} + +fn decode_drained_pdus(server: &mut GraphicsPipelineServer) -> Vec { + let mut decompressor = Decompressor::new(); + server + .drain_output() + .iter() + .map(|message| { + let encoded = encode_vec(message.as_ref()).expect("encode DVC message"); + let mut decoded = Vec::new(); + decompressor.decompress(&encoded, &mut decoded).expect("decompress PDU"); + GfxPdu::decode(&mut ReadCursor::new(&decoded)).expect("decode PDU") + }) + .collect() +} + +fn clearcodec_tile(left: u16, top: u16, right: u16, bottom: u16) -> MixedTilePayload { + MixedTilePayload::ClearCodec { + destination: ExclusiveRectangle { + left, + top, + right, + bottom, + }, + bitmap_data: vec![0x00, 0x00, 0x00, 0x00], + } +} + +fn avc420_tile(region: Avc420Region) -> MixedTilePayload { + MixedTilePayload::Avc420 { + regions: vec![region], + h264_data: vec![0x00, 0x00, 0x00, 0x01, 0x67], + } +} + +#[test] +fn mixed_frame_sends_each_tile_in_order_inside_one_frame() { + let (mut server, surface_id) = mixed_frame_server(CapabilitySet::V10_4 { + flags: CapabilitiesV104Flags::SMALL_CACHE, + }); + + let tiles = vec![ + clearcodec_tile(0, 0, 16, 16), + MixedTilePayload::RemoteFxProgressive { + codec_context_id: 7, + progressive_data: vec![0xCC, 0xC0, 0x06, 0x00, 0x00, 0x00], + }, + avc420_tile(Avc420Region::new(16, 8, 48, 40, 22, 78)), + ]; + let frame_id = server + .send_mixed_frame(surface_id, tiles, 42) + .expect("queue mixed frame"); + + let pdus = decode_drained_pdus(&mut server); + assert_eq!(pdus.len(), 5); + let GfxPdu::StartFrame(start) = &pdus[0] else { + panic!("expected StartFrame, got {:?}", pdus[0]); + }; + assert_eq!(start.frame_id, frame_id); + + let GfxPdu::WireToSurface1(clearcodec) = &pdus[1] else { + panic!("expected ClearCodec WireToSurface1"); + }; + assert_eq!(clearcodec.codec_id, Codec1Type::ClearCodec); + assert_eq!(clearcodec.destination_rectangle.right, 16); + assert_eq!(clearcodec.destination_rectangle.bottom, 16); + + let GfxPdu::WireToSurface2(progressive) = &pdus[2] else { + panic!("expected Progressive WireToSurface2"); + }; + assert_eq!(progressive.codec_id, Codec2Type::RemoteFxProgressive); + assert_eq!(progressive.codec_context_id, 7); + + let GfxPdu::WireToSurface1(avc420) = &pdus[3] else { + panic!("expected AVC420 WireToSurface1"); + }; + assert_eq!(avc420.codec_id, Codec1Type::Avc420); + assert_eq!(avc420.destination_rectangle.left, 16); + assert_eq!(avc420.destination_rectangle.top, 8); + assert_eq!(avc420.destination_rectangle.right, 48); + assert_eq!(avc420.destination_rectangle.bottom, 40); + + let GfxPdu::EndFrame(end) = &pdus[4] else { + panic!("expected EndFrame, got {:?}", pdus[4]); + }; + assert_eq!(end.frame_id, frame_id); + assert_eq!(server.frames_in_flight(), 1); +} + +#[rstest] +#[case::empty_clearcodec_destination(clearcodec_tile(8, 8, 8, 16))] +#[case::clearcodec_destination_past_the_surface(clearcodec_tile(0, 0, 65, 16))] +#[case::empty_avc420_region(avc420_tile(Avc420Region::new(0, 8, 32, 8, 22, 78)))] +#[case::avc420_region_past_the_surface(avc420_tile(Avc420Region::new(0, 0, 32, 65, 22, 78)))] +#[case::avc420_tile_with_no_regions(MixedTilePayload::Avc420 { + regions: Vec::new(), + h264_data: vec![0x00, 0x00, 0x00, 0x01, 0x67], +})] +#[case::avc420_qp_above_51(avc420_tile(Avc420Region::new(0, 0, 32, 32, 52, 78)))] +#[case::avc420_qp_too_large_for_the_field(avc420_tile(Avc420Region::new(0, 0, 32, 32, 64, 78)))] +#[case::avc420_quality_above_100(avc420_tile(Avc420Region::new(0, 0, 32, 32, 22, 101)))] +fn mixed_frame_rejects_invalid_tiles_without_queueing(#[case] bad_tile: MixedTilePayload) { + let (mut server, surface_id) = mixed_frame_server(CapabilitySet::V10_4 { + flags: CapabilitiesV104Flags::SMALL_CACHE, + }); + let tiles = vec![clearcodec_tile(0, 0, 16, 16), bad_tile]; + + assert!(server.send_mixed_frame(surface_id, tiles, 42).is_none()); + assert!(!server.has_pending_output()); + assert_eq!(server.frames_in_flight(), 0); +} + +#[test] +fn mixed_frame_rejects_avc420_tiles_when_avc_is_disabled() { + let (mut server, surface_id) = mixed_frame_server(CapabilitySet::V10_4 { + flags: CapabilitiesV104Flags::AVC_DISABLED, + }); + assert!(!server.supports_avc420()); + + let tiles = vec![ + clearcodec_tile(0, 0, 16, 16), + avc420_tile(Avc420Region::new(0, 0, 32, 32, 22, 78)), + ]; + assert!(server.send_mixed_frame(surface_id, tiles, 42).is_none()); + assert!(!server.has_pending_output()); + assert_eq!(server.frames_in_flight(), 0); + + // With no other codec in the frame the codec mix rule is out of the way, so + // the AVC420 support check is what refuses it. + let tiles = vec![avc420_tile(Avc420Region::new(0, 0, 32, 32, 22, 78))]; + assert!(server.send_mixed_frame(surface_id, tiles, 42).is_none()); + assert!(!server.has_pending_output()); + assert_eq!(server.frames_in_flight(), 0); + + let tiles = vec![clearcodec_tile(0, 0, 16, 16)]; + assert!(server.send_mixed_frame(surface_id, tiles, 42).is_some()); +} + +/// MS-RDPEGFX 2.2.3.7 promises AVC420 in the same frame as other codecs only from +/// capability version 10.4, so an earlier client gets no such frame. Each case +/// allows AVC420, so it's refused for the reason under test and not because AVC is +/// off. +#[rstest] +#[case::v8_1(CapabilitySet::V8_1 { flags: CapabilitiesV81Flags::AVC420_ENABLED })] +#[case::v10(CapabilitySet::V10 { flags: CapabilitiesV10Flags::SMALL_CACHE })] +#[case::v10_1(CapabilitySet::V10_1)] +#[case::v10_2(CapabilitySet::V10_2 { flags: CapabilitiesV10Flags::SMALL_CACHE })] +#[case::v10_3(CapabilitySet::V10_3 { flags: CapabilitiesV103Flags::empty() })] +fn mixed_frame_with_avc420_and_another_codec_is_refused_before_10_4(#[case] caps: CapabilitySet) { + let (mut server, surface_id) = mixed_frame_server(caps); + assert!(server.supports_avc420()); + + let tiles = vec![ + clearcodec_tile(0, 0, 16, 16), + avc420_tile(Avc420Region::new(0, 0, 32, 32, 22, 78)), + ]; + assert!(server.send_mixed_frame(surface_id, tiles, 42).is_none()); + assert!(!server.has_pending_output()); + assert_eq!(server.frames_in_flight(), 0); +} + +/// From capability version 10.4 the same mix is queued. +#[test] +fn mixed_frame_with_avc420_and_another_codec_is_allowed_at_10_4() { + let (mut server, surface_id) = mixed_frame_server(CapabilitySet::V10_4 { + flags: CapabilitiesV104Flags::empty(), + }); + let tiles = vec![ + clearcodec_tile(0, 0, 16, 16), + avc420_tile(Avc420Region::new(0, 0, 32, 32, 22, 78)), + ]; + assert!(server.send_mixed_frame(surface_id, tiles, 42).is_some()); +} + +/// The promise is about AVC420 beside another codec, so tiles that are all AVC420, +/// or none of them AVC420, are not held to it on an earlier version. +#[rstest] +#[case::v8_1(CapabilitySet::V8_1 { flags: CapabilitiesV81Flags::AVC420_ENABLED })] +#[case::v10(CapabilitySet::V10 { flags: CapabilitiesV10Flags::SMALL_CACHE })] +#[case::v10_1(CapabilitySet::V10_1)] +#[case::v10_2(CapabilitySet::V10_2 { flags: CapabilitiesV10Flags::SMALL_CACHE })] +#[case::v10_3(CapabilitySet::V10_3 { flags: CapabilitiesV103Flags::empty() })] +fn mixed_frame_without_a_codec_mix_is_allowed_before_10_4(#[case] caps: CapabilitySet) { + let (mut server, surface_id) = mixed_frame_server(caps.clone()); + let tiles = vec![ + avc420_tile(Avc420Region::new(0, 0, 32, 32, 22, 78)), + avc420_tile(Avc420Region::new(32, 32, 64, 64, 22, 78)), + ]; + assert!(server.send_mixed_frame(surface_id, tiles, 42).is_some()); + + let (mut server, surface_id) = mixed_frame_server(caps); + let tiles = vec![clearcodec_tile(0, 0, 16, 16)]; + assert!(server.send_mixed_frame(surface_id, tiles, 42).is_some()); +} + +/// What the server reports for `caps`, after checking that it confirmed exactly them. +fn negotiated_avc420_in_mixed_frames(caps: CapabilitySet) -> bool { + let (server, _surface_id) = mixed_frame_server(caps.clone()); + assert_eq!(server.negotiated_capabilities(), Some(&caps)); + + server.codec_capabilities().avc420_in_mixed_frames +} + +#[rstest] +#[case::v8(CapabilitySet::V8 { flags: CapabilitiesV8Flags::empty() })] +#[case::v8_1(CapabilitySet::V8_1 { flags: CapabilitiesV81Flags::AVC420_ENABLED })] +#[case::v10(CapabilitySet::V10 { flags: CapabilitiesV10Flags::empty() })] +#[case::v10_1(CapabilitySet::V10_1)] +#[case::v10_2(CapabilitySet::V10_2 { flags: CapabilitiesV10Flags::empty() })] +#[case::v10_3(CapabilitySet::V10_3 { flags: CapabilitiesV103Flags::empty() })] +fn avc420_is_not_promised_beside_other_codecs_before_10_4(#[case] caps: CapabilitySet) { + assert!(!negotiated_avc420_in_mixed_frames(caps)); +} + +#[rstest] +#[case::v10_4(CapabilitySet::V10_4 { flags: CapabilitiesV104Flags::empty() })] +#[case::v10_5(CapabilitySet::V10_5 { flags: CapabilitiesV104Flags::empty() })] +#[case::v10_6(CapabilitySet::V10_6 { flags: CapabilitiesV104Flags::empty() })] +#[case::v10_6_err(CapabilitySet::V10_6Err { flags: CapabilitiesV104Flags::empty() })] +#[case::v10_7(CapabilitySet::V10_7 { flags: CapabilitiesV107Flags::empty() })] +fn avc420_is_promised_beside_other_codecs_from_10_4(#[case] caps: CapabilitySet) { + assert!(negotiated_avc420_in_mixed_frames(caps)); +} + +#[rstest] +#[case::v10_4(CapabilitySet::V10_4 { flags: CapabilitiesV104Flags::AVC_DISABLED })] +#[case::v10_5(CapabilitySet::V10_5 { flags: CapabilitiesV104Flags::AVC_DISABLED })] +#[case::v10_6(CapabilitySet::V10_6 { flags: CapabilitiesV104Flags::AVC_DISABLED })] +#[case::v10_6_err(CapabilitySet::V10_6Err { flags: CapabilitiesV104Flags::AVC_DISABLED })] +#[case::v10_7(CapabilitySet::V10_7 { flags: CapabilitiesV107Flags::AVC_DISABLED })] +fn avc420_is_not_promised_beside_other_codecs_when_the_client_disabled_avc(#[case] caps: CapabilitySet) { + assert!(!negotiated_avc420_in_mixed_frames(caps)); +} + // ============================================================================ // Planar Frame Tests // ============================================================================ @@ -645,6 +900,42 @@ fn test_qoe_reset() { assert!(server.qoe_snapshot().is_none()); } +#[test] +fn mixed_and_progressive_senders_count_backpressure_in_qoe() { + use ironrdp_egfx::pdu::QoeFrameAcknowledgePdu; + + let (mut server, surface_id) = mixed_frame_server(CapabilitySet::V10_4 { + flags: CapabilitiesV104Flags::empty(), + }); + server.set_max_frames_in_flight(1); + + // The snapshot only exists once there is a QoE report or an RTT sample. + let qoe_pdu = GfxPdu::QoeFrameAcknowledge(QoeFrameAcknowledgePdu { + frame_id: 0, + timestamp: 1000, + time_diff_se: 50, + time_diff_dr: 3000, + }); + server.process(0, &encode_pdu(&qoe_pdu)).expect("process QoE report"); + assert_eq!(server.qoe_snapshot().expect("QoE snapshot").backpressure_count, 0); + + // One frame in flight is the limit, so the senders refuse everything after it. + let tiles = vec![clearcodec_tile(0, 0, 16, 16)]; + assert!(server.send_mixed_frame(surface_id, tiles, 0).is_some()); + assert!(server.should_backpressure()); + + let tiles = vec![clearcodec_tile(0, 0, 16, 16)]; + assert!(server.send_mixed_frame(surface_id, tiles, 16).is_none()); + assert_eq!(server.qoe_snapshot().expect("QoE snapshot").backpressure_count, 1); + + assert!( + server + .send_remotefx_progressive_frame(surface_id, 1, vec![0; 4], 33) + .is_none() + ); + assert_eq!(server.qoe_snapshot().expect("QoE snapshot").backpressure_count, 2); +} + // ============================================================================ // Uncompressed Frame Tests // ============================================================================ From ceede21b704b9591f977ad5ed93e04ecd386584c Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Fri, 25 Sep 2026 05:57:52 -0500 Subject: [PATCH 2/2] feat(egfx): add AVC444 tiles to mixed-codec frames A server that negotiated AVC444 could not put an AVC444 update in the same frame as a lossless tile. Add Avc444 and Avc444v2 variants to MixedTilePayload with the fields of send_avc444v2_frame, check them with the other tiles before the frame is queued, and share the stream shape check and PDU construction with the single-codec AVC444 senders. --- crates/ironrdp-egfx/src/server.rs | 290 +++++++++++++----- .../tests/egfx/server.rs | 211 ++++++++++++- 2 files changed, 416 insertions(+), 85 deletions(-) diff --git a/crates/ironrdp-egfx/src/server.rs b/crates/ironrdp-egfx/src/server.rs index ed46af0144..5096607ffc 100644 --- a/crates/ironrdp-egfx/src/server.rs +++ b/crates/ironrdp-egfx/src/server.rs @@ -1068,9 +1068,9 @@ pub struct GraphicsPipelineServer { /// [`GraphicsPipelineServer::send_mixed_frame()`] to pack multiple codec /// types into a single `StartFrame`/`EndFrame` pair. /// -/// Marked `#[non_exhaustive]` so future EGFX codec additions (for example, -/// Avc444 or hardware-accelerated paths) can land without a SemVer break -/// for downstream consumers that pattern-match on this enum. +/// Marked `#[non_exhaustive]` so future EGFX codec additions can land +/// without a SemVer break for downstream consumers that pattern-match on +/// this enum. #[non_exhaustive] pub enum MixedTilePayload { /// Lossless ClearCodec tile (text, UI elements, icons). @@ -1093,6 +1093,45 @@ pub enum MixedTilePayload { regions: Vec, h264_data: Vec, }, + /// H.264 AVC444 tile (`RFX_AVC444_BITMAP_STREAM`, MS-RDPEGFX 2.2.4.5). + Avc444(Avc444Tile), + /// H.264 AVC444v2 tile (`RFX_AVC444V2_BITMAP_STREAM`, MS-RDPEGFX 2.2.4.6). + /// + /// Identical on the wire to [`MixedTilePayload::Avc444`] except for how + /// the client combines the two views, so it takes the same [`Avc444Tile`]. + Avc444v2(Avc444Tile), +} + +/// One H.264 sub-stream of an [`Avc444Tile`]: the regions it covers and its +/// data, which together are one `RFX_AVC420_BITMAP_STREAM`. +pub struct Avc444SubStream { + pub regions: Vec, + pub data: Vec, +} + +/// The fields of an AVC444 or AVC444v2 tile in a mixed-codec frame. +/// +/// They mirror [`GraphicsPipelineServer::send_avc444v2_frame()`]: `encoding` is +/// the LC value, and the second sub-stream, `stream2`, is present exactly when it +/// is `LUMA_AND_CHROMA`. The regions and the data of the second sub-stream travel +/// together in one [`Avc444SubStream`], so a tile can't hold one without the +/// other. Each sub-stream keeps its own regions because each is a full +/// `RFX_AVC420_BITMAP_STREAM`. +/// +/// MS-RDPEGFX 2.2.3.7 guarantees same-frame mixing only for AVC in YUV420 +/// mode, so no capability version promises that a client decodes AVC444 next to +/// other codecs. [`GraphicsPipelineServer::send_mixed_frame()`] still holds it +/// to the 10.4 floor that applies to AVC420, which you can check beforehand with +/// [`CodecCapabilities::avc420_in_mixed_frames`] from +/// [`GraphicsPipelineServer::codec_capabilities()`], and you should test the +/// clients you target. An LC `CHROMA` update is combined with previously decoded luma +/// (2.2.4.5), so it must not cover an area another codec has repainted since +/// that area's last luma update. +pub struct Avc444Tile { + pub encoding: Encoding, + pub stream1_regions: Vec, + pub stream1_data: Vec, + pub stream2: Option, } impl GraphicsPipelineServer { @@ -1520,6 +1559,12 @@ impl GraphicsPipelineServer { /// AVC444 uses two streams: luma (Y) and chroma (UV). Set `chroma_data` to /// `None` for luma-only transmission. /// + /// Unlike [`Self::send_mixed_frame()`], this does not check the regions. Each + /// must be non-empty and inside the surface, with a QP of at most 51 and a + /// quality of at most 100 (MS-RDPEGFX 2.2.4.4.2). A `None` from this sender has + /// only meant not ready, not supported or backpressure, and callers read it that + /// way. + /// /// Returns `Some(frame_id)` if queued, `None` if not supported or backpressured. pub fn send_avc444_frame( &mut self, @@ -1555,6 +1600,12 @@ impl GraphicsPipelineServer { /// stream 1 and chroma in stream 2, LC=1 carries luma in stream 1, and LC=2 /// carries chroma in stream 1. /// + /// Unlike [`Self::send_mixed_frame()`], this does not check the regions. Each + /// must be non-empty and inside the surface, with a QP of at most 51 and a + /// quality of at most 100 (MS-RDPEGFX 2.2.4.4.2). A `None` from this sender has + /// only meant not ready, not supported or backpressure, and callers read it that + /// way. + /// /// Returns `Some(frame_id)` if queued, `None` if the stream shape is /// invalid, AVC444 is not supported, or backpressure is active. /// @@ -1600,14 +1651,14 @@ impl GraphicsPipelineServer { stream2_regions: Option<&[Avc420Region]>, timestamp_ms: u32, ) -> Option { - let valid_stream_shape = if encoding == Encoding::LUMA_AND_CHROMA { - stream2_data.is_some() && stream2_regions.is_some() - } else if encoding == Encoding::LUMA || encoding == Encoding::CHROMA { - stream2_data.is_none() && stream2_regions.is_none() - } else { - false + // A second sub-stream is its regions and its data together; one without the + // other is never valid. + let stream2 = match (stream2_regions, stream2_data) { + (Some(regions), Some(data)) => Some((regions, data)), + (None, None) => None, + _ => return None, }; - if !valid_stream_shape { + if !Self::avc444_stream_shape_is_valid(encoding, stream2.is_some()) { return None; } @@ -1627,57 +1678,102 @@ impl GraphicsPipelineServer { let timestamp = Self::make_timestamp(timestamp_ms); let frame_id = self.frames.begin_frame(timestamp); - let stream1_rectangles: Vec<_> = stream1_regions.iter().map(Avc420Region::to_rectangle).collect(); - let stream1_quant_vals: Vec<_> = stream1_regions.iter().map(Avc420Region::to_quant_quality).collect(); + let wire_pdu = Self::avc444_wire_pdu(codec_id, surface, encoding, (stream1_regions, stream1_data), stream2); - let stream1 = Avc420BitmapStream { - rectangles: stream1_rectangles, - quant_qual_vals: stream1_quant_vals, - data: stream1_data, - }; + self.output_queue + .push_back(GfxPdu::StartFrame(StartFramePdu { timestamp, frame_id })); + self.output_queue.push_back(GfxPdu::WireToSurface1(wire_pdu)); + self.output_queue.push_back(GfxPdu::EndFrame(EndFramePdu { frame_id })); - let stream2 = if encoding == Encoding::LUMA_AND_CHROMA { - let stream2_data = stream2_data?; - let stream2_regions = stream2_regions?; - let stream2_rectangles: Vec<_> = stream2_regions.iter().map(Avc420Region::to_rectangle).collect(); - let stream2_quant_vals: Vec<_> = stream2_regions.iter().map(Avc420Region::to_quant_quality).collect(); + Some(frame_id) + } - Some(Avc420BitmapStream { - rectangles: stream2_rectangles, - quant_qual_vals: stream2_quant_vals, - data: stream2_data, - }) + /// Whether the second AVC444 sub-stream is present exactly when the LC + /// value calls for it; LC 3 is invalid (MS-RDPEGFX 2.2.4.5). + fn avc444_stream_shape_is_valid(encoding: Encoding, has_stream2: bool) -> bool { + if encoding == Encoding::LUMA_AND_CHROMA { + has_stream2 + } else if encoding == Encoding::LUMA || encoding == Encoding::CHROMA { + !has_stream2 } else { - None + false + } + } + + /// Build the `WireToSurface1` PDU for an AVC444 or AVC444v2 update whose + /// stream shape has already been checked. + fn avc444_wire_pdu<'a>( + codec_id: Codec1Type, + surface: &Surface, + encoding: Encoding, + stream1: (&[Avc420Region], &'a [u8]), + stream2: Option<(&[Avc420Region], &'a [u8])>, + ) -> WireToSurface1Pdu { + let substream = |(regions, data): (&[Avc420Region], &'a [u8])| Avc420BitmapStream { + rectangles: regions.iter().map(Avc420Region::to_rectangle).collect(), + quant_qual_vals: regions.iter().map(Avc420Region::to_quant_quality).collect(), + data, }; let avc444_stream = Avc444BitmapStream { encoding, - stream1, - stream2, + stream1: substream(stream1), + stream2: stream2.map(substream), }; - let encoded_stream = encode_avc444_bitmap_stream(&avc444_stream); - let target_rect = if let Some(stream2_regions) = stream2_regions { - Self::compute_dest_rect_for_streams(stream1_regions, stream2_regions, surface.width, surface.height) - } else { - Self::compute_dest_rect(stream1_regions, surface.width, surface.height) + // destRect is the bounding rectangle of both sub-streams (MS-RDPEGFX 2.2.2.1). + let destination_rectangle = match stream2 { + Some((stream2_regions, _)) => { + Self::compute_dest_rect_for_streams(stream1.0, stream2_regions, surface.width, surface.height) + } + None => Self::compute_dest_rect(stream1.0, surface.width, surface.height), }; - self.output_queue - .push_back(GfxPdu::StartFrame(StartFramePdu { timestamp, frame_id })); - - self.output_queue.push_back(GfxPdu::WireToSurface1(WireToSurface1Pdu { - surface_id, + WireToSurface1Pdu { + surface_id: surface.id, codec_id, pixel_format: surface.pixel_format, - destination_rectangle: target_rect, - bitmap_data: encoded_stream, - })); + destination_rectangle, + bitmap_data: encode_avc444_bitmap_stream(&avc444_stream), + } + } - self.output_queue.push_back(GfxPdu::EndFrame(EndFramePdu { frame_id })); + /// Build the `WireToSurface1` PDU for an AVC444 or AVC444v2 mixed-frame tile + /// whose shape has already been checked. + fn avc444_tile_pdu(codec_id: Codec1Type, surface: &Surface, tile: &Avc444Tile) -> WireToSurface1Pdu { + Self::avc444_wire_pdu( + codec_id, + surface, + tile.encoding, + (&tile.stream1_regions, &tile.stream1_data), + tile.stream2 + .as_ref() + .map(|stream2| (stream2.regions.as_slice(), stream2.data.as_slice())), + ) + } - Some(frame_id) + /// Why the regions of one AVC420 tile or one AVC444 sub-stream can't be sent + /// in a mixed frame, if they can't: the list is empty, a region is empty or + /// outside the surface, or a QP or quality is outside the ranges of + /// MS-RDPEGFX 2.2.4.4.2. + fn avc_regions_refusal(surface: &Surface, regions: &[Avc420Region]) -> Option<&'static str> { + if regions.is_empty() { + return Some("AVC tile has no regions"); + } + if regions + .iter() + .any(|region| !Self::rect_fits_surface(surface, ®ion.to_rectangle())) + { + return Some("AVC region is empty or outside the surface"); + } + if regions + .iter() + .any(|region| MAX_AVC_QP < region.quantization_parameter || MAX_AVC_QUALITY < region.quality) + { + return Some("AVC QP or quality is out of range"); + } + + None } fn compute_dest_rect_for_streams( @@ -1919,16 +2015,20 @@ impl GraphicsPipelineServer { /// reports whether the negotiated set makes it. /// /// Every tile is checked before the frame is queued. AVC420 tiles need - /// AVC420 support in the negotiated capabilities, and a frame that mixes an - /// AVC420 tile with another codec needs capability version 10.4 or later, - /// the versions that make that promise. A frame of AVC420 tiles alone is - /// not covered by it. Each AVC420 tile needs at least one region, unlike + /// AVC420 support in the negotiated capabilities and AVC444 tiles need + /// AVC444 support; an AVC444 tile's second sub-stream must be present + /// exactly when its LC is `LUMA_AND_CHROMA`. A frame that mixes an AVC tile + /// (AVC420, AVC444 or AVC444v2) with a tile of another codec needs + /// capability version 10.4 or later. The spec promises that only for + /// AVC420, and only from there; AVC444 beside another codec is held to the + /// same floor because no version promises it at all. A frame whose tiles + /// all use one codec is not covered by it. Each AVC tile needs at least one + /// region, and so does the second sub-stream of an AVC444 tile, unlike /// [`Self::send_avc420_frame()`], which reads an empty list as the whole - /// surface: a whole-surface tile would cover the ClearCodec and AVC420 - /// tiles sent before it. Each AVC420 region and ClearCodec destination must - /// be non-empty and inside the surface. - /// AVC420 regions also need a QP of at most 51 (for 8-bit video) and a - /// quality of at most 100 (MS-RDPEGFX [2.2.4.4.2]). If any check fails + /// surface: a whole-surface tile would cover the tiles sent before it. Each + /// AVC region and ClearCodec destination must be non-empty and inside the + /// surface. AVC regions also need a QP of at most 51 (for 8-bit video) and + /// a quality of at most 100 (MS-RDPEGFX [2.2.4.4.2]). If any check fails /// nothing is queued and no frame is tracked. /// /// Returns `Some(frame_id)` if queued, `None` if not ready, backpressured, @@ -1957,23 +2057,33 @@ impl GraphicsPipelineServer { } let is_avc420 = |tile: &MixedTilePayload| matches!(tile, MixedTilePayload::Avc420 { .. }); - if tiles.iter().any(is_avc420) { - if !self.supports_avc420() { - trace!( - reason = "AVC420 is not supported by the negotiated capabilities", - "Mixed frame refused" - ); - return None; - } - if !self.codec_caps.avc420_in_mixed_frames && tiles.iter().any(|tile| !is_avc420(tile)) { - trace!( - reason = "AVC420 beside another codec is not promised by the negotiated capabilities", - "Mixed frame refused" - ); - return None; - } + if tiles.iter().any(is_avc420) && !self.supports_avc420() { + trace!( + reason = "AVC420 is not supported by the negotiated capabilities", + "Mixed frame refused" + ); + return None; } + // Tiles of one codec family are never a mix; an AVC tile beside a tile of + // another family needs the 10.4 floor (AVC444 and AVC444v2 count as one family). + let family = |tile: &MixedTilePayload| match tile { + MixedTilePayload::ClearCodec { .. } => 0, + MixedTilePayload::RemoteFxProgressive { .. } => 1, + MixedTilePayload::Avc420 { .. } => 2, + MixedTilePayload::Avc444(_) | MixedTilePayload::Avc444v2(_) => 3, + }; + let has_avc = tiles.iter().any(|tile| 2 <= family(tile)); + let mixes_codecs = tiles.iter().any(|tile| family(tile) != family(&tiles[0])); + if has_avc && mixes_codecs && !self.codec_caps.avc420_in_mixed_frames { + trace!( + reason = "AVC beside another codec is not promised by the negotiated capabilities", + "Mixed frame refused" + ); + return None; + } + + let supports_avc444 = self.supports_avc444(); let surface = self.surfaces.get(surface_id)?; let pixel_format = surface.pixel_format; @@ -1981,24 +2091,20 @@ impl GraphicsPipelineServer { MixedTilePayload::ClearCodec { destination, .. } => (!Self::rect_fits_surface(surface, destination)) .then_some("ClearCodec destination is empty or outside the surface"), MixedTilePayload::RemoteFxProgressive { .. } => None, - MixedTilePayload::Avc420 { regions, .. } => { - if regions.is_empty() { - return Some("AVC420 tile has no regions"); - } - if regions - .iter() - .any(|region| !Self::rect_fits_surface(surface, ®ion.to_rectangle())) - { - return Some("AVC420 region is empty or outside the surface"); + MixedTilePayload::Avc420 { regions, .. } => Self::avc_regions_refusal(surface, regions), + MixedTilePayload::Avc444(tile) | MixedTilePayload::Avc444v2(tile) => { + if !supports_avc444 { + return Some("AVC444 is not supported by the negotiated capabilities"); } - if regions - .iter() - .any(|region| MAX_AVC_QP < region.quantization_parameter || MAX_AVC_QUALITY < region.quality) - { - return Some("AVC420 QP or quality is out of range"); + if !Self::avc444_stream_shape_is_valid(tile.encoding, tile.stream2.is_some()) { + return Some("AVC444 second sub-stream does not match the LC value"); } - None + Self::avc_regions_refusal(surface, &tile.stream1_regions).or_else(|| { + tile.stream2 + .as_ref() + .and_then(|stream2| Self::avc_regions_refusal(surface, &stream2.regions)) + }) } }); if let Some(reason) = refusal { @@ -2050,6 +2156,22 @@ impl GraphicsPipelineServer { bitmap_data: encoded_stream, })); } + MixedTilePayload::Avc444(tile) => { + self.output_queue + .push_back(GfxPdu::WireToSurface1(Self::avc444_tile_pdu( + Codec1Type::Avc444, + surface, + &tile, + ))); + } + MixedTilePayload::Avc444v2(tile) => { + self.output_queue + .push_back(GfxPdu::WireToSurface1(Self::avc444_tile_pdu( + Codec1Type::Avc444v2, + surface, + &tile, + ))); + } } } diff --git a/crates/ironrdp-testsuite-core/tests/egfx/server.rs b/crates/ironrdp-testsuite-core/tests/egfx/server.rs index 8e0f034e5a..91da952d4b 100644 --- a/crates/ironrdp-testsuite-core/tests/egfx/server.rs +++ b/crates/ironrdp-testsuite-core/tests/egfx/server.rs @@ -5,7 +5,9 @@ use ironrdp_egfx::pdu::{ CapabilitiesV81Flags, CapabilitiesV103Flags, CapabilitiesV104Flags, CapabilitiesV107Flags, CapabilitySet, Codec1Type, Codec2Type, Encoding, FrameAcknowledgePdu, GfxPdu, PixelFormat, QueueDepth, }; -use ironrdp_egfx::server::{GraphicsPipelineHandler, GraphicsPipelineServer, MixedTilePayload, QoeMetrics, Surface}; +use ironrdp_egfx::server::{ + Avc444SubStream, Avc444Tile, GraphicsPipelineHandler, GraphicsPipelineServer, MixedTilePayload, QoeMetrics, Surface, +}; use ironrdp_graphics::zgfx::Decompressor; use ironrdp_pdu::geometry::ExclusiveRectangle; use rstest::rstest; @@ -326,6 +328,8 @@ fn avc444v2_sender_rejects_invalid_stream_shapes_without_queueing() { (Encoding::LUMA_AND_CHROMA, None, None), (Encoding::LUMA, Some(data.as_slice()), Some(regions.as_slice())), (Encoding::CHROMA, Some(data.as_slice()), Some(regions.as_slice())), + (Encoding::LUMA, Some(data.as_slice()), None), + (Encoding::LUMA, None, Some(regions.as_slice())), (Encoding::from_bits_retain(3), None, None), ] { assert!( @@ -590,6 +594,211 @@ fn avc420_is_not_promised_beside_other_codecs_when_the_client_disabled_avc(#[cas assert!(!negotiated_avc420_in_mixed_frames(caps)); } +fn avc444_tile( + v2: bool, + encoding: Encoding, + stream1_regions: Vec, + stream2_regions: Option>, +) -> MixedTilePayload { + let stream1_data = vec![0x00, 0x00, 0x00, 0x01, 0x67]; + let stream2 = stream2_regions.map(|regions| Avc444SubStream { + regions, + data: vec![0x00, 0x00, 0x00, 0x01, 0x68], + }); + let tile = Avc444Tile { + encoding, + stream1_regions, + stream1_data, + stream2, + }; + if v2 { + MixedTilePayload::Avc444v2(tile) + } else { + MixedTilePayload::Avc444(tile) + } +} + +fn avc444_region() -> Vec { + vec![Avc420Region::new(0, 0, 32, 32, 22, 78)] +} + +/// A server whose client negotiated 10.4, where an AVC tile may share a frame with another codec. +fn server_at_10_4() -> (GraphicsPipelineServer, u16) { + mixed_frame_server(CapabilitySet::V10_4 { + flags: CapabilitiesV104Flags::empty(), + }) +} + +#[rstest] +fn mixed_frame_carries_avc444_tiles_next_to_clearcodec( + #[values(false, true)] v2: bool, + #[values(Encoding::LUMA_AND_CHROMA, Encoding::LUMA, Encoding::CHROMA)] encoding: Encoding, +) { + use ironrdp_graphics::clearcodec::{ClearCodecDecoder, ClearCodecEncoder}; + + let pixels: Vec = (0..=u8::MAX) + .flat_map(|i| [0x10 ^ i, 0x20 ^ i, 0x30 ^ i, 0xFF]) + .collect(); + let clearcodec_data = ClearCodecEncoder::new().encode(&pixels, 16, 16); + + let (mut server, surface_id) = server_at_10_4(); + let stream2_regions = + (encoding == Encoding::LUMA_AND_CHROMA).then(|| vec![Avc420Region::new(16, 8, 64, 48, 24, 76)]); + + let tiles = vec![ + avc444_tile(v2, encoding, avc444_region(), stream2_regions), + MixedTilePayload::ClearCodec { + destination: ExclusiveRectangle { + left: 48, + top: 48, + right: 64, + bottom: 64, + }, + bitmap_data: clearcodec_data, + }, + ]; + server + .send_mixed_frame(surface_id, tiles, 42) + .expect("queue mixed frame"); + + let pdus = decode_drained_pdus(&mut server); + assert_eq!(pdus.len(), 4); + assert!(matches!(pdus[0], GfxPdu::StartFrame(_))); + assert!(matches!(pdus[3], GfxPdu::EndFrame(_))); + + let GfxPdu::WireToSurface1(avc444) = &pdus[1] else { + panic!("expected AVC444 WireToSurface1"); + }; + let expected_codec = if v2 { Codec1Type::Avc444v2 } else { Codec1Type::Avc444 }; + assert_eq!(avc444.codec_id, expected_codec); + + let stream = Avc444BitmapStream::decode(&mut ReadCursor::new(&avc444.bitmap_data)).expect("decode AVC444"); + assert_eq!(stream.encoding, encoding); + assert_eq!(stream.stream1.rectangles[0].right, 32); + assert_eq!(stream.stream1.rectangles[0].bottom, 32); + if encoding == Encoding::LUMA_AND_CHROMA { + let stream2 = stream.stream2.expect("second sub-stream"); + assert_eq!(stream2.rectangles[0].left, 16); + assert_eq!(stream2.rectangles[0].bottom, 48); + // destRect is the bounding box of both sub-streams. + assert_eq!(avc444.destination_rectangle.right, 64); + assert_eq!(avc444.destination_rectangle.bottom, 48); + } else { + assert!(stream.stream2.is_none()); + assert_eq!(avc444.destination_rectangle.right, 32); + assert_eq!(avc444.destination_rectangle.bottom, 32); + } + + let GfxPdu::WireToSurface1(clearcodec) = &pdus[2] else { + panic!("expected ClearCodec WireToSurface1"); + }; + assert_eq!(clearcodec.codec_id, Codec1Type::ClearCodec); + let decoded = ClearCodecDecoder::new() + .decode(&clearcodec.bitmap_data, 16, 16) + .expect("decode ClearCodec tile"); + assert_eq!(decoded.len(), 16 * 16 * 4); +} + +#[rstest] +#[case::lc0_without_a_second_sub_stream(avc444_tile(false, Encoding::LUMA_AND_CHROMA, avc444_region(), None))] +#[case::lc1_with_a_second_sub_stream(avc444_tile(true, Encoding::LUMA, avc444_region(), Some(avc444_region())))] +#[case::lc2_with_a_second_sub_stream(avc444_tile(false, Encoding::CHROMA, avc444_region(), Some(avc444_region())))] +#[case::lc3(avc444_tile(true, Encoding::from_bits_retain(3), avc444_region(), None))] +#[case::second_sub_stream_region_past_the_surface(avc444_tile( + true, + Encoding::LUMA_AND_CHROMA, + avc444_region(), + Some(vec![Avc420Region::new(0, 0, 80, 32, 22, 78)]), +))] +#[case::degenerate_first_sub_stream_region(avc444_tile( + false, + Encoding::LUMA, + vec![Avc420Region::new(4, 4, 4, 32, 22, 78)], + None, +))] +#[case::no_first_sub_stream_regions(avc444_tile(false, Encoding::LUMA, Vec::new(), None))] +#[case::no_second_sub_stream_regions(avc444_tile(true, Encoding::LUMA_AND_CHROMA, avc444_region(), Some(Vec::new())))] +#[case::qp_above_51_in_the_second_sub_stream(avc444_tile( + false, + Encoding::LUMA_AND_CHROMA, + avc444_region(), + Some(vec![Avc420Region::new(0, 0, 32, 32, 52, 78)]), +))] +#[case::quality_above_100_in_the_first_sub_stream(avc444_tile( + false, + Encoding::LUMA, + vec![Avc420Region::new(0, 0, 32, 32, 22, 101)], + None, +))] +fn mixed_frame_rejects_invalid_avc444_tiles_without_queueing(#[case] bad_tile: MixedTilePayload) { + let (mut server, surface_id) = server_at_10_4(); + let tiles = vec![clearcodec_tile(0, 0, 16, 16), bad_tile]; + + assert!(server.send_mixed_frame(surface_id, tiles, 42).is_none()); + assert!(!server.has_pending_output()); + assert_eq!(server.frames_in_flight(), 0); +} + +#[test] +fn mixed_frame_rejects_avc444_tiles_without_avc444_support() { + let (mut server, surface_id) = mixed_frame_server(CapabilitySet::V8_1 { + flags: CapabilitiesV81Flags::AVC420_ENABLED, + }); + assert!(server.supports_avc420()); + assert!(!server.supports_avc444()); + + let tiles = vec![avc444_tile(true, Encoding::LUMA, avc444_region(), None)]; + assert!(server.send_mixed_frame(surface_id, tiles, 42).is_none()); + assert!(!server.has_pending_output()); + assert_eq!(server.frames_in_flight(), 0); +} + +/// MS-RDPEGFX 2.2.3.7 promises no version at which AVC444 decodes next to other codecs, so +/// the mixed path holds it to the 10.4 floor it applies to AVC420. Each case allows AVC444 +/// and AVC420, so the refusal is for the version and not because AVC is off. +#[rstest] +#[case::v10(CapabilitySet::V10 { flags: CapabilitiesV10Flags::SMALL_CACHE })] +#[case::v10_1(CapabilitySet::V10_1)] +#[case::v10_2(CapabilitySet::V10_2 { flags: CapabilitiesV10Flags::SMALL_CACHE })] +#[case::v10_3(CapabilitySet::V10_3 { flags: CapabilitiesV103Flags::empty() })] +fn mixed_frame_with_avc444_and_another_codec_is_refused_before_10_4( + #[case] caps: CapabilitySet, + #[values(false, true)] v2: bool, +) { + for other_tile in [ + clearcodec_tile(48, 48, 64, 64), + avc420_tile(Avc420Region::new(32, 32, 64, 64, 22, 78)), + ] { + let (mut server, surface_id) = mixed_frame_server(caps.clone()); + assert!(server.supports_avc444()); + assert!(server.supports_avc420()); + + let tiles = vec![avc444_tile(v2, Encoding::LUMA, avc444_region(), None), other_tile]; + assert!(server.send_mixed_frame(surface_id, tiles, 42).is_none()); + assert!(!server.has_pending_output()); + assert_eq!(server.frames_in_flight(), 0); + } +} + +/// Tiles that all use one codec are no mix, so on an earlier version a frame of AVC444 tiles +/// is queued. +#[test] +fn mixed_frame_of_avc444_tiles_alone_is_allowed_before_10_4() { + let (mut server, surface_id) = mixed_frame_server(CapabilitySet::V10 { + flags: CapabilitiesV10Flags::SMALL_CACHE, + }); + let tiles = vec![ + avc444_tile(false, Encoding::LUMA, avc444_region(), None), + avc444_tile( + true, + Encoding::LUMA, + vec![Avc420Region::new(32, 32, 64, 64, 22, 78)], + None, + ), + ]; + assert!(server.send_mixed_frame(surface_id, tiles, 42).is_some()); +} + // ============================================================================ // Planar Frame Tests // ============================================================================