fix(graphics): invalid YCoCg color conversion (#146)

Closes #141

crates/ironrdp-error/src/lib.rs

@@ -161,7 +161,7 @@ where
     }
 }
 
-/// Temporary compability traits to smooth transition from old style
+/// Temporary compatibility traits to smooth transition from old style
 #[cfg(feature = "std")]
 #[doc(hidden)]
 pub mod legacy {

crates/ironrdp-graphics/src/color_conversion.rs

@@ -18,10 +18,6 @@ fn clip(v: i32) -> u8 {
     min(max(v, 0), 255) as u8
 }
 
-fn clip_i16(v: i16) -> u8 {
-    min(max(v, 0), 255) as u8
-}
-
 #[derive(Debug)]
 pub struct YCbCrBuffer<'a> {
     pub y: &'a [i16],
@@ -56,13 +52,6 @@ pub struct YCbCr {
     pub cr: i16,
 }
 
-#[derive(Debug, Copy, Clone, PartialEq, Eq)]
-pub struct YCoCg {
-    pub y: u8,
-    pub co: i8,
-    pub cg: i8,
-}
-
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub struct Rgb {
     pub r: u8,
@@ -90,19 +79,3 @@ impl From<YCbCr> for Rgb {
         Self { r, g, b }
     }
 }
-
-impl From<YCoCg> for Rgb {
-    fn from(YCoCg { y, co, cg }: YCoCg) -> Self {
-        let y = i16::from(y);
-        let co = i16::from(co);
-        let cg = i16::from(cg);
-
-        let t = y - cg / 2;
-
-        let r = clip_i16(t + co / 2);
-        let g = clip_i16(y + cg / 2);
-        let b = clip_i16(t - co / 2);
-
-        Self { r, g, b }
-    }
-}

crates/ironrdp-graphics/src/rdp6/bitmap_stream.rs

@@ -2,7 +2,7 @@ use ironrdp_pdu::bitmap::rdp6::{BitmapStream as BitmapStreamPdu, ColorPlanes};
 use ironrdp_pdu::{decode, PduError};
 use thiserror::Error;
 
-use crate::color_conversion::{Rgb, YCoCg};
+use crate::color_conversion::Rgb;
 use crate::rdp6::rle::{decompress_8bpp_plane, RleError};
 
 #[derive(Debug, Error)]
@@ -158,8 +158,6 @@ impl<'a> BitmapStreamDecoderImpl<'a> {
     }
 
     fn write_aycocg_planes_to_rgb24(&self, params: AYCoCgParams, planes: &[u8], dst: &mut Vec<u8>) {
-        // For AYCoCg we need to take color loss level and subsampling into account
-        let chroma_shift = (params.color_loss_level - 1) as usize;
         let sample_shift = params.chroma_subsampling as usize;
 
         let (y_offset, co_offset, cg_offset) = (
@@ -177,10 +175,10 @@ impl<'a> BitmapStreamDecoderImpl<'a> {
             let chroma_col = (idx % self.image_width) >> sample_shift;
             let chroma_idx = chroma_row * self.chroma_width + chroma_col;
 
-            let co = (co_plane[chroma_idx] << chroma_shift) as i8;
-            let cg = (cg_plane[chroma_idx] << chroma_shift) as i8;
+            let co = co_plane[chroma_idx];
+            let cg = cg_plane[chroma_idx];
 
-            let Rgb { r, g, b } = YCoCg { y, co, cg }.into();
+            let Rgb { r, g, b } = ycocg_with_cll_to_rgb(params.color_loss_level, y, co, cg);
 
             // As described in 3.1.9.1.2 [MS-RDPEGDI], R and B channels are swapped for
             // AYCoCg when 24-bit image is used (no alpha). We swap them back here
@@ -206,7 +204,7 @@ impl<'a> BitmapStreamDecoderImpl<'a> {
                 use_chroma_subsampling,
                 ..
             } => {
-                let params = AYCoCgParams {
+                let params: AYCoCgParams = AYCoCgParams {
                     color_loss_level,
                     chroma_subsampling: use_chroma_subsampling,
                     alpha: self.bitmap.use_alpha,
@@ -220,6 +218,32 @@ impl<'a> BitmapStreamDecoderImpl<'a> {
     }
 }
 
+/// Perform YCoCg -> RGB conversion with color loss redution (CLL) correction.
+fn ycocg_with_cll_to_rgb(cll: u8, y: u8, co: u8, cg: u8) -> Rgb {
+    // We decrease CLL by 1 to skip division by 2 for co & cg components during computation of
+    // the following color conversion matrix:
+    // |R|   |1   1/2   -1/2|   |Y |
+    // |G| = |1    0     1/2| * |Co|
+    // |B|   |1  -1/2   -1/2|   |Cg|
+    let chroma_shift = (cll - 1) as usize;
+
+    let clip_i16 = |v: i16| v.max(0).min(255) as u8;
+
+    let co_signed = (co << chroma_shift) as i8;
+    let cg_signed = (cg << chroma_shift) as i8;
+
+    let y = i16::from(y);
+    let co = i16::from(co_signed);
+    let cg = i16::from(cg_signed);
+
+    let t = y - cg;
+    let r = clip_i16(t + co);
+    let g = clip_i16(y + cg);
+    let b = clip_i16(t - co);
+
+    Rgb { r, g, b }
+}
+
 impl BitmapStreamDecoder {
     /// Performs decoding of bitmap stream PDU from `bitmap_data` and writes decoded rgb24
     /// image to `dst` buffer.
@@ -286,35 +310,35 @@ mod tests {
     }
 
     #[test]
-    fn decode_64x24_aycocg_rle() {
+    fn decode_64x64_aycocg_rle() {
         // AYCoCg (With alpha), RLE, no chroma subsampling
         assert_decoded_image(
-            include_bytes!("test_assets/64x24_aycocg_rle.bin"),
-            include_bytes!("test_assets/64x24_aycocg_rle.bmp"),
+            include_bytes!("test_assets/64x64_aycocg_rle.bin"),
+            include_bytes!("test_assets/64x64_aycocg_rle.bmp"),
+            64,
             64,
-            24,
         );
     }
 
     #[test]
-    fn decode_64x24_ycocg_rle_ss() {
+    fn decode_64x64_ycocg_rle_ss() {
         // AYCoCg (No alpha), RLE, with chroma subsampling
         assert_decoded_image(
-            include_bytes!("test_assets/64x24_ycocg_rle_ss.bin"),
-            include_bytes!("test_assets/64x24_ycocg_rle_ss.bmp"),
+            include_bytes!("test_assets/64x64_ycocg_rle_ss.bin"),
+            include_bytes!("test_assets/64x64_ycocg_rle_ss.bmp"),
+            64,
             64,
-            24,
         );
     }
 
     #[test]
-    fn decode_64x57_ycocg_rle_ss() {
+    fn decode_64x35_ycocg_rle_ss() {
         // AYCoCg (No alpha), RLE, with chroma subsampling + odd resolution
         assert_decoded_image(
-            include_bytes!("test_assets/64x57_ycocg_rle_ss.bin"),
-            include_bytes!("test_assets/64x57_ycocg_rle_ss.bmp"),
+            include_bytes!("test_assets/64x35_ycocg_rle_ss.bin"),
+            include_bytes!("test_assets/64x35_ycocg_rle_ss.bmp"),
             64,
-            57,
+            35,
         );
     }
 

crates/ironrdp-session/src/rfx.rs

@@ -76,7 +76,7 @@ impl DecodingContext {
         let channels = channels.ok_or(general_err!("channels header is missing"))?;
 
         if channels.0.is_empty() {
-            return Err(general_err!("no RFX channel annouced"));
+            return Err(general_err!("no RFX channel announced"));
         }
 
         self.context = context;