From df3c1f9c66a97207039fc900a1aaa5d7548ccc9d Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Fri, 25 Sep 2026 05:14:06 -0500 Subject: [PATCH] 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 // ============================================================================