Skip to content

Preliminary refactoring patches for QOI #730

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.

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

Merged
merged 10 commits into from
Mar 31, 2025
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 6 additions & 8 deletions crates/ironrdp-bench/benches/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@ pub fn rfx_enc_tile_bench(c: &mut Criterion) {
let quant = rfx::Quant::default();
let algo = rfx::EntropyAlgorithm::Rlgr3;
let bitmap = BitmapUpdate {
top: 0,
left: 0,
x: 0,
y: 0,
width: NonZero::new(64).unwrap(),
height: NonZero::new(64).unwrap(),
format: ironrdp_server::PixelFormat::ARgb32,
data: vec![0; 64 * 64 * 4],
order: ironrdp_server::PixelOrder::BottomToTop,
data: vec![0; 64 * 64 * 4].into(),
stride: 64 * 4,
};
c.bench_function("rfx_enc_tile", |b| b.iter(|| rfx_enc_tile(&bitmap, &quant, algo, 0, 0)));
Expand All @@ -26,13 +25,12 @@ pub fn rfx_enc_bench(c: &mut Criterion) {
let quant = rfx::Quant::default();
let algo = rfx::EntropyAlgorithm::Rlgr3;
let bitmap = BitmapUpdate {
top: 0,
left: 0,
x: 0,
y: 0,
width: NonZero::new(2048).unwrap(),
height: NonZero::new(2048).unwrap(),
format: ironrdp_server::PixelFormat::ARgb32,
data: vec![0; 2048 * 2048 * 4],
order: ironrdp_server::PixelOrder::BottomToTop,
data: vec![0; 2048 * 2048 * 4].into(),
stride: 64 * 4,
};
c.bench_function("rfx_enc", |b| b.iter(|| rfx_enc(&bitmap, &quant, algo)));
Expand Down
1 change: 1 addition & 0 deletions crates/ironrdp-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ tracing = { version = "0.1", features = ["log"] }
x509-cert = { version = "0.2.5", optional = true }
rustls-pemfile = { version = "2.2.0", optional = true }
rayon = { version = "1.10.0", optional = true }
bytes = "1"

[dev-dependencies]
tokio = { version = "1", features = ["sync"] }
Expand Down
100 changes: 87 additions & 13 deletions crates/ironrdp-server/src/display.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use core::num::NonZeroU16;

use anyhow::Result;
use bytes::{Bytes, BytesMut};
use ironrdp_displaycontrol::pdu::DisplayControlMonitorLayout;
use ironrdp_pdu::pointer::PointerPositionAttribute;

Expand All @@ -24,12 +25,6 @@ pub enum DisplayUpdate {
DefaultPointer,
}

#[derive(Debug, Clone, Copy, PartialEq)]
pub enum PixelOrder {
TopToBottom,
BottomToTop,
}

#[derive(Clone)]
pub struct RGBAPointer {
pub width: u16,
Expand Down Expand Up @@ -61,32 +56,111 @@ pub struct ColorPointer {
pub xor_mask: Vec<u8>,
}

pub struct Framebuffer {
pub width: NonZeroU16,
pub height: NonZeroU16,
pub format: PixelFormat,
pub data: BytesMut,
Copy link
Member

@CBenoit CBenoit Mar 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: What is the benefit of using BytesMut over Vec<u8> at this place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not nessarily a Vec, we may have shared memory. Although I realize shared memory may be problematic too... Perhaps a whole copy is inavoidable..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User has to be careful that shared memory will be owned by the server. But we should still allow shared memory to avoid copy.

Copy link
Member

@CBenoit CBenoit Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that at some point freeze() or alike is called, and memory is actually shared then?
This sounds good to me, but as you point out, let’s be careful about how we use this: we experienced issues in the past where the way BytesMut was used caused too much memory being allocated and freed, resulting in both performance and memory usage being poor. (Big chunk of memory may also never get freed, as long as a view on it is remaining, even if it’s a very small view. And how BytesMut is used is also very relevant.)
I assume you are measuring the CPU and memory usage, and have a good insight. It would help avoid regressions to add some in-line comments to document the decision.

pub stride: usize,
}

impl TryInto<Framebuffer> for BitmapUpdate {
type Error = &'static str;

fn try_into(self) -> Result<Framebuffer, Self::Error> {
assert_eq!(self.x, 0);
assert_eq!(self.y, 0);
Ok(Framebuffer {
width: self.width,
height: self.height,
format: self.format,
data: self.data.try_into_mut().map_err(|_| "BitmapUpdate is shared")?,
stride: self.stride,
})
}
}

/// Bitmap Display Update
///
/// Bitmap updates are encoded using RDP 6.0 compression, fragmented and sent using
/// Fastpath Server Updates
///
#[derive(Clone)]
pub struct BitmapUpdate {
pub top: u16,
pub left: u16,
pub x: u16,
pub y: u16,
pub width: NonZeroU16,
pub height: NonZeroU16,
pub format: PixelFormat,
pub order: PixelOrder,
pub data: Vec<u8>,
pub data: Bytes,
pub stride: usize,
}

impl BitmapUpdate {
/// Extracts a sub-region of the bitmap update.
///
/// # Parameters
///
/// - `x`: The x-coordinate of the top-left corner of the sub-region.
/// - `y`: The y-coordinate of the top-left corner of the sub-region.
/// - `width`: The width of the sub-region.
/// - `height`: The height of the sub-region.
///
/// # Returns
///
/// An `Option` containing a new `BitmapUpdate` representing the sub-region if the specified
/// dimensions are within the bounds of the original bitmap update, otherwise `None`.
///
/// # Example
///
/// ```
/// # use core::num::NonZeroU16;
/// # use bytes::Bytes;
/// # use ironrdp_graphics::image_processing::PixelFormat;
/// # use ironrdp_server::BitmapUpdate;
/// let original = BitmapUpdate {
/// x: 0,
/// y: 0,
/// width: NonZeroU16::new(100).unwrap(),
/// height: NonZeroU16::new(100).unwrap(),
/// format: PixelFormat::ARgb32,
/// data: Bytes::from(vec![0; 40000]),
/// stride: 400,
/// };
///
/// let sub_region = original.sub(10, 10, NonZeroU16::new(50).unwrap(), NonZeroU16::new(50).unwrap());
/// assert!(sub_region.is_some());
/// ```
#[must_use]
pub fn sub(&self, x: u16, y: u16, width: NonZeroU16, height: NonZeroU16) -> Option<Self> {
if x + width.get() > self.width.get() || y + height.get() > self.height.get() {
None
} else {
let bpp = usize::from(self.format.bytes_per_pixel());
let start = usize::from(y) * self.stride + usize::from(x) * bpp;
let end = start + usize::from(height.get() - 1) * self.stride + usize::from(width.get()) * bpp;
Some(Self {
x: self.x + x,
y: self.y + y,
width,
height,
format: self.format,
data: self.data.slice(start..end),
stride: self.stride,
})
}
}
}

impl core::fmt::Debug for BitmapUpdate {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
f.debug_struct("BitmapUpdate")
.field("top", &self.top)
.field("left", &self.left)
.field("x", &self.x)
.field("y", &self.y)
.field("width", &self.width)
.field("height", &self.height)
.field("format", &self.format)
.field("order", &self.order)
.field("stride", &self.stride)
.finish()
}
}
Expand Down
37 changes: 11 additions & 26 deletions crates/ironrdp-server/src/encoder/bitmap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use ironrdp_graphics::rdp6::{ABgrChannels, ARgbChannels, BgrAChannels, BitmapStr
use ironrdp_pdu::bitmap::{self, BitmapData, BitmapUpdateData, Compression};
use ironrdp_pdu::geometry::InclusiveRectangle;

use crate::{BitmapUpdate, PixelOrder};
use crate::BitmapUpdate;

// PERF: we could also remove the need for this buffer
pub(crate) struct BitmapEncoder {
Expand Down Expand Up @@ -39,31 +39,25 @@ impl BitmapEncoder {

for (i, chunk) in chunks.enumerate() {
let height = chunk.len() / bitmap.stride;
let top = usize::from(bitmap.top) + i * chunk_height;
let top = usize::from(bitmap.y) + i * chunk_height;

let encoder = BitmapStreamEncoder::new(usize::from(bitmap.width.get()), height);

let len = match bitmap.order {
PixelOrder::BottomToTop => {
Self::encode_slice(encoder, bitmap.format, &chunk[..row_len], self.buffer.as_mut_slice())
}
let len = {
let pixels = chunk
.chunks(bitmap.stride)
.map(|row| &row[..row_len])
.rev()
.flat_map(|row| row.chunks(bytes_per_pixel));

PixelOrder::TopToBottom => {
let pixels = chunk
.chunks(bitmap.stride)
.map(|row| &row[..row_len])
.rev()
.flat_map(|row| row.chunks(bytes_per_pixel));

Self::encode_iter(encoder, bitmap.format, pixels, self.buffer.as_mut_slice())
}
Self::encode_iter(encoder, bitmap.format, pixels, self.buffer.as_mut_slice())
};

let data = BitmapData {
rectangle: InclusiveRectangle {
left: bitmap.left,
left: bitmap.x,
top: u16::try_from(top).unwrap(),
right: bitmap.left + bitmap.width.get() - 1,
right: bitmap.x + bitmap.width.get() - 1,
bottom: u16::try_from(top + height - 1).unwrap(),
},
width: u16::from(bitmap.width),
Expand All @@ -84,15 +78,6 @@ impl BitmapEncoder {
Ok(cursor.pos())
}

fn encode_slice(mut encoder: BitmapStreamEncoder, format: PixelFormat, src: &[u8], dst: &mut [u8]) -> usize {
match format {
PixelFormat::ARgb32 | PixelFormat::XRgb32 => encoder.encode_bitmap::<ARgbChannels>(src, dst, true).unwrap(),
PixelFormat::RgbA32 | PixelFormat::RgbX32 => encoder.encode_bitmap::<RgbAChannels>(src, dst, true).unwrap(),
PixelFormat::ABgr32 | PixelFormat::XBgr32 => encoder.encode_bitmap::<ABgrChannels>(src, dst, true).unwrap(),
PixelFormat::BgrA32 | PixelFormat::BgrX32 => encoder.encode_bitmap::<BgrAChannels>(src, dst, true).unwrap(),
}
}

fn encode_iter<'a, P>(mut encoder: BitmapStreamEncoder, format: PixelFormat, src: P, dst: &mut [u8]) -> usize
where
P: Iterator<Item = &'a [u8]> + Clone,
Expand Down
Loading
Loading