From d236fd298f936782bbd05ec9d4acd4e3b4e3a9ff Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 10 Jun 2024 21:05:11 +0200 Subject: [PATCH 1/9] fix: Altought it seems to have worked before, correctly propagate features --- breakwater-core/Cargo.toml | 1 + breakwater-core/src/lib.rs | 7 ++++++- breakwater-parser/Cargo.toml | 1 + breakwater/Cargo.toml | 6 ++++-- 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/breakwater-core/Cargo.toml b/breakwater-core/Cargo.toml index 0d73d3f..665e770 100644 --- a/breakwater-core/Cargo.toml +++ b/breakwater-core/Cargo.toml @@ -13,3 +13,4 @@ tokio.workspace = true [features] alpha = [] +binary-commands = [] diff --git a/breakwater-core/src/lib.rs b/breakwater-core/src/lib.rs index 3e9976a..bcb6e59 100644 --- a/breakwater-core/src/lib.rs +++ b/breakwater-core/src/lib.rs @@ -11,13 +11,18 @@ PX x y rrggbb: Color the pixel (x,y) with the given hexadecimal color rrggbb {} PX x y gg: Color the pixel (x,y) with the hexadecimal color gggggg. Basically this is the same as the other commands, but is a more efficient way of filling white, black or gray areas PX x y: Get the color value of the pixel (x,y) -SIZE: Get the size of the drawing surface, e.g. `SIZE 1920 1080` +{}SIZE: Get the size of the drawing surface, e.g. `SIZE 1920 1080` OFFSET x y: Apply offset (x,y) to all further pixel draws on this connection. This can e.g. be used to pre-calculate an image/animation and simply use the OFFSET command to move it around the screen without the need to re-calculate it ", if cfg!(feature = "alpha") { "PX x y rrggbbaa: Color the pixel (x,y) with the given hexadecimal color rrggbb and a transparency of aa, where ff means draw normally on top of the existing pixel and 00 means fully transparent (no change at all)" } else { "PX x y rrggbbaa: Color the pixel (x,y) with the given hexadecimal color rrggbb. The alpha part is discarded for performance reasons, as breakwater was compiled without the alpha feature" +}, +if cfg!(feature = "binary-commands") { + "PBxxyyrgba: Binary version of the PX command. x and y are little-endian 16 bit coordinates, r, g, b and are a byte each. There is *no* newline after the command.\n" +} else { + "" } ).as_bytes(); diff --git a/breakwater-parser/Cargo.toml b/breakwater-parser/Cargo.toml index e300282..35ba5d6 100644 --- a/breakwater-parser/Cargo.toml +++ b/breakwater-parser/Cargo.toml @@ -23,3 +23,4 @@ pixelbomber.workspace = true [features] alpha = [] +binary-commands = [] diff --git a/breakwater/Cargo.toml b/breakwater/Cargo.toml index 557369b..810ea92 100644 --- a/breakwater/Cargo.toml +++ b/breakwater/Cargo.toml @@ -37,6 +37,8 @@ vncserver = { workspace = true, optional = true } rstest.workspace = true [features] -default = ["vnc"] +default = ["vnc", "binary-commands"] + vnc = ["dep:vncserver"] -alpha = [] +alpha = ["breakwater-core/alpha", "breakwater-parser/alpha"] +binary-commands = ["breakwater-core/binary-commands", "breakwater-parser/binary-commands"] From 04138e340e3780fa81829ffa11be80f365154800 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 10 Jun 2024 22:24:25 +0200 Subject: [PATCH 2/9] WIP --- .github/workflows/build.yml | 8 +++--- breakwater-parser/src/original.rs | 16 ++++++++++++ breakwater/src/tests.rs | 42 +++++++++++++++++++++++++++++-- 3 files changed, 60 insertions(+), 6 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index e4e4a18..b4b4b21 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -123,17 +123,17 @@ jobs: uses: actions-rs/cargo@v1 with: command: test - args: --no-default-features + args: --no-default-features --all-targets - name: Test with all features turned on uses: actions-rs/cargo@v1 with: command: test - args: --all-features - - name: Test vnc features + args: --all-features --all-targets + - name: Test vnc feature uses: actions-rs/cargo@v1 with: command: test - args: --no-default-features --features vnc + args: --no-default-features --features vnc --all-targets run_build: name: Build for ${{ matrix.target }} diff --git a/breakwater-parser/src/original.rs b/breakwater-parser/src/original.rs index e35b38a..a55f8da 100644 --- a/breakwater-parser/src/original.rs +++ b/breakwater-parser/src/original.rs @@ -10,6 +10,7 @@ use crate::Parser; pub const PARSER_LOOKAHEAD: usize = "PX 1234 1234 rrggbbaa\n".len(); // Longest possible command pub(crate) const PX_PATTERN: u64 = string_to_number(b"PX \0\0\0\0\0"); +pub(crate) const PB_PATTERN: u64 = string_to_number(b"PB\0\0\0\0\0\0"); pub(crate) const OFFSET_PATTERN: u64 = string_to_number(b"OFFSET \0\0"); pub(crate) const SIZE_PATTERN: u64 = string_to_number(b"SIZE\0\0\0\0"); pub(crate) const HELP_PATTERN: u64 = string_to_number(b"HELP\0\0\0\0"); @@ -140,6 +141,21 @@ impl Parser for OriginalParser { continue; } } + // In case the feature is disabled this if should be optimized away, as "cfg!" should be a constant expression. + } else if cfg!(feature = "binary-commands") + && current_command & 0x0000_ffff == PB_PATTERN + { + let command_bytes = + unsafe { (buffer.as_ptr().add(i + 2) as *const u64).read_unaligned() }; + + let x = u16::from_le((command_bytes) as u16); + let y = u16::from_le((command_bytes >> 16) as u16); + let rgba = u32::from_le((command_bytes >> 32) as u32); + + self.fb.set(x as usize, y as usize, rgba & 0x00ff_ffff); + + i += 10; + continue; } else if current_command & 0x00ff_ffff_ffff_ffff == OFFSET_PATTERN { i += 7; diff --git a/breakwater/src/tests.rs b/breakwater/src/tests.rs index 42fc8dd..fef2976 100644 --- a/breakwater/src/tests.rs +++ b/breakwater/src/tests.rs @@ -136,6 +136,44 @@ async fn test_setting_pixel( assert_eq!(expected, stream.get_output()); } +#[cfg(feature = "binary-commands")] +#[rstest] +// No newline in between needed +#[case("PB\0\0\0\0\0\0\0\0PX 0 0\n", "PX 0 0 000000\n")] +#[case("PB\0\0\0\01234PX 0 0\n", "PX 0 0 313233\n")] +#[case("PB\0\0\0\0\0\0\0\0PB\0\0\0\01234PX 0 0\n", "PX 0 0 313233\n")] +#[case( + "PB\0\0\0\0\0\0\0\0PX 0 0\nPB\0\0\0\01234PX 0 0\n", + "PX 0 0 000000\nPX 0 0 313233\n" +)] +#[case("PB \0*\0____PX 32 42\n", "PX 32 42 5f5f5f\n")] +#[tokio::test] +async fn test_binary_commands( + #[case] input: &str, + #[case] expected: &str, + ip: IpAddr, + fb: Arc, + statistics_channel: ( + mpsc::Sender, + mpsc::Receiver, + ), +) { + let mut stream = MockTcpStream::from_input(input); + handle_connection( + &mut stream, + ip, + fb, + statistics_channel.0, + DEFAULT_NETWORK_BUFFER_SIZE, + page_size::get(), + None, + ) + .await + .unwrap(); + + assert_eq!(expected, stream.get_output()); +} + #[rstest] #[case("PX 0 0 aaaaaa\n")] #[case("PX 0 0 aa\n")] @@ -180,8 +218,8 @@ async fn test_safe( #[case(479, 361, 721, 391)] #[case(500, 500, 0, 0)] #[case(500, 500, 300, 400)] -#[case(fb().get_width(), fb().get_height(), 0, 0)] -#[case(fb().get_width() - 1, fb().get_height() - 1, 1, 1)] +// Yes, this exceeds the framebuffer size +#[case(10, 10, fb().get_width(), fb().get_height())] #[tokio::test] async fn test_drawing_rect( #[case] width: usize, From 0e2b06a5606942cba7fcf7763b1f6ea2f49cd5a8 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 11 Jun 2024 16:07:52 +0200 Subject: [PATCH 3/9] Finish implementation --- CHANGELOG.md | 1 + README.md | 6 ++++++ breakwater-core/src/lib.rs | 2 +- breakwater-parser/src/original.rs | 1 + breakwater-parser/src/refactored.rs | 25 ++++++++++++++++++++++++- 5 files changed, 33 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f19e6e..4555dc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to this project will be documented in this file. ### Added +- Support binary protocol ([#XX]) - Try to improve performance by calling `madvise` to inform Kernel we are reading sequentially ([#24]) - Expose metric on denied connection counts ([#26]) - Print nicer error messages ([#32]) diff --git a/README.md b/README.md index 2508baa..cf3fc03 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,8 @@ Commands must be sent newline-separated, for more details see [Pixelflut](https: * `PX x y rrggbbaa`: Color the pixel (x,y) with the given hexadecimal color rrggbb (alpha channel is ignored for now), e.g. `PX 10 10 ff0000ff` * `PX x y gg`: Color the pixel (x,y) with the hexadecimal color gggggg. Basically this is the same as the other commands, but is a more efficient way of filling white, black or gray areas, e.g. `PX 10 10 00` to paint black * `PX x y`: Get the color value of the pixel (x,y), e.g. `PX 10 10` +* `PBxxyyrgba`: Binary version of the `PX` command. `x` and `y` are little-endian 16 bit coordinates, `r`, `g`, `b` and `a` are a byte each. There is **no** newline after the command. +Tipp: For most use-cases this is the most efficient format with 10 bytes per pixel ;) * `SIZE`: Get the size of the drawing surface, e.g. `SIZE 1920 1080` * `OFFSET x y`: Apply offset (x,y) to all further pixel draws on this connection. This can e.g. be used to pre-calculate an image/animation and simply use the OFFSET command to move it around the screen without the need to re-calculate it, e.g. `OFFSET 100 100` @@ -60,6 +62,8 @@ Options: Height of the drawing surface [default: 720] -f, --fps Frames per second the server should aim for [default: 30] + --network-buffer-size + The size in bytes of the network buffer used for each open TCP connection. Please use at least 64 KB (64_000 bytes) [default: 262144] -t, --text Text to display on the screen. The text will be followed by "on " [default: "Pixelflut server (breakwater)"] --font @@ -78,6 +82,8 @@ Options: Enable dump of video stream into file. File location will be `/pixelflut_dump_{timestamp}.mp4 -v, --vnc-port Port of the VNC server [default: 5900] + -c, --connections-per-ip + Allow only a certain number of connections per ip address -h, --help Print help -V, --version diff --git a/breakwater-core/src/lib.rs b/breakwater-core/src/lib.rs index bcb6e59..e3828e8 100644 --- a/breakwater-core/src/lib.rs +++ b/breakwater-core/src/lib.rs @@ -20,7 +20,7 @@ if cfg!(feature = "alpha") { "PX x y rrggbbaa: Color the pixel (x,y) with the given hexadecimal color rrggbb. The alpha part is discarded for performance reasons, as breakwater was compiled without the alpha feature" }, if cfg!(feature = "binary-commands") { - "PBxxyyrgba: Binary version of the PX command. x and y are little-endian 16 bit coordinates, r, g, b and are a byte each. There is *no* newline after the command.\n" + "PBxxyyrgba: Binary version of the PX command. x and y are little-endian 16 bit coordinates, r, g, b and a are a byte each. There is *no* newline after the command.\n" } else { "" } diff --git a/breakwater-parser/src/original.rs b/breakwater-parser/src/original.rs index a55f8da..801149b 100644 --- a/breakwater-parser/src/original.rs +++ b/breakwater-parser/src/original.rs @@ -152,6 +152,7 @@ impl Parser for OriginalParser { let y = u16::from_le((command_bytes >> 16) as u16); let rgba = u32::from_le((command_bytes >> 32) as u32); + // TODO: Support alpha channel (behind alpha feature flag) self.fb.set(x as usize, y as usize, rgba & 0x00ff_ffff); i += 10; diff --git a/breakwater-parser/src/refactored.rs b/breakwater-parser/src/refactored.rs index 2761145..4bdf336 100644 --- a/breakwater-parser/src/refactored.rs +++ b/breakwater-parser/src/refactored.rs @@ -4,7 +4,8 @@ use breakwater_core::{framebuffer::FrameBuffer, HELP_TEXT}; use crate::{ original::{ - parse_pixel_coordinates, simd_unhex, HELP_PATTERN, OFFSET_PATTERN, PX_PATTERN, SIZE_PATTERN, + parse_pixel_coordinates, simd_unhex, HELP_PATTERN, OFFSET_PATTERN, PB_PATTERN, PX_PATTERN, + SIZE_PATTERN, }, Parser, }; @@ -83,6 +84,24 @@ impl RefactoredParser { } } + #[inline(always)] + fn handle_binary_pixel(&self, buffer: &[u8], mut idx: usize) -> (usize, usize) { + let previous = idx; + idx += 2; + + let command_bytes = unsafe { (buffer.as_ptr().add(idx) as *const u64).read_unaligned() }; + + let x = u16::from_le((command_bytes) as u16); + let y = u16::from_le((command_bytes >> 16) as u16); + let rgba = u32::from_le((command_bytes >> 32) as u32); + + // TODO: Support alpha channel (behind alpha feature flag) + self.fb.set(x as usize, y as usize, rgba & 0x00ff_ffff); + + idx += 8; + (idx, previous) + } + #[inline(always)] fn handle_offset(&mut self, idx: &mut usize, buffer: &[u8]) { let (x, y, present) = parse_pixel_coordinates(buffer.as_ptr(), idx); @@ -185,6 +204,10 @@ impl Parser for RefactoredParser { unsafe { (buffer.as_ptr().add(i) as *const u64).read_unaligned() }; if current_command & 0x00ff_ffff == PX_PATTERN { (i, last_byte_parsed) = self.handle_pixel(buffer, i, response); + } else if cfg!(feature = "binary-commands") + && current_command & 0x0000_ffff == PB_PATTERN + { + (i, last_byte_parsed) = self.handle_binary_pixel(buffer, i); } else if current_command & 0x00ff_ffff_ffff_ffff == OFFSET_PATTERN { i += 7; self.handle_offset(&mut i, buffer); From ff7eaf9a36269244baf4797dab1193f1899c6055 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 11 Jun 2024 16:09:48 +0200 Subject: [PATCH 4/9] Add testcase --- breakwater/src/tests.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/breakwater/src/tests.rs b/breakwater/src/tests.rs index fef2976..8f5fc06 100644 --- a/breakwater/src/tests.rs +++ b/breakwater/src/tests.rs @@ -147,6 +147,11 @@ async fn test_setting_pixel( "PX 0 0 000000\nPX 0 0 313233\n" )] #[case("PB \0*\0____PX 32 42\n", "PX 32 42 5f5f5f\n")] +// Also test that there can be newlines in between +#[case( + "PB\0\0\0\0\0\0\0\0\nPX 0 0\nPB\0\0\0\01234\n\n\nPX 0 0\n", + "PX 0 0 000000\nPX 0 0 313233\n" +)] #[tokio::test] async fn test_binary_commands( #[case] input: &str, From 29d0c24d1086e6c66509db9b6d3e528fdf8ba60f Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 11 Jun 2024 16:17:39 +0200 Subject: [PATCH 5/9] Silence clippy --- breakwater/src/tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/breakwater/src/tests.rs b/breakwater/src/tests.rs index 8f5fc06..52d65da 100644 --- a/breakwater/src/tests.rs +++ b/breakwater/src/tests.rs @@ -1,3 +1,5 @@ +#![allow(clippy::octal_escapes)] + use std::{ net::{IpAddr, Ipv4Addr}, sync::Arc, From 982684535b56c0d647f24d1fd7002a88e34f7af8 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 11 Jun 2024 16:18:08 +0200 Subject: [PATCH 6/9] changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4555dc1..a39fd03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. ### Added -- Support binary protocol ([#XX]) +- Support binary protocol ([#33]) - Try to improve performance by calling `madvise` to inform Kernel we are reading sequentially ([#24]) - Expose metric on denied connection counts ([#26]) - Print nicer error messages ([#32]) @@ -22,6 +22,7 @@ All notable changes to this project will be documented in this file. [#25]: https://github.com/sbernauer/breakwater/pull/25 [#26]: https://github.com/sbernauer/breakwater/pull/26 [#32]: https://github.com/sbernauer/breakwater/pull/32 +[#33]: https://github.com/sbernauer/breakwater/pull/33 ## [0.14.0] - 2024-05-30 at GPN 22 :) From eb86ef5f78223f870c4529222d7f0ad6f3ec3595 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 11 Jun 2024 16:24:32 +0200 Subject: [PATCH 7/9] fix test --- breakwater/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/breakwater/src/tests.rs b/breakwater/src/tests.rs index 52d65da..4b7f2ed 100644 --- a/breakwater/src/tests.rs +++ b/breakwater/src/tests.rs @@ -226,7 +226,7 @@ async fn test_safe( #[case(500, 500, 0, 0)] #[case(500, 500, 300, 400)] // Yes, this exceeds the framebuffer size -#[case(10, 10, fb().get_width(), fb().get_height())] +#[case(10, 10, fb().get_width() - 5, fb().get_height() - 5)] #[tokio::test] async fn test_drawing_rect( #[case] width: usize, @@ -248,7 +248,7 @@ async fn test_drawing_rect( let mut read_other_pixels_commands = String::new(); let mut read_other_pixels_commands_expected = String::new(); - for x in 0..fb.get_width() { + for x in 0..height { for y in 0..height { // Inside the rect if x >= offset_x && x <= offset_x + width && y >= offset_y && y <= offset_y + height { From 3ae39f3464385baad66fbd130eb16fe35a67ca47 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 11 Jun 2024 16:28:59 +0200 Subject: [PATCH 8/9] fix tests part 2 --- breakwater/src/tests.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/breakwater/src/tests.rs b/breakwater/src/tests.rs index 4b7f2ed..abfcb1a 100644 --- a/breakwater/src/tests.rs +++ b/breakwater/src/tests.rs @@ -20,7 +20,8 @@ fn ip() -> IpAddr { #[fixture] fn fb() -> Arc { - Arc::new(FrameBuffer::new(1920, 1080)) + // We keep the framebuffer so small, so that we can easily test all pixels in a test run + Arc::new(FrameBuffer::new(640, 480)) } #[fixture] @@ -37,13 +38,13 @@ fn statistics_channel() -> ( #[case("\n", "")] #[case("not a pixelflut command", "")] #[case("not a pixelflut command with newline\n", "")] -#[case("SIZE", "SIZE 1920 1080\n")] -#[case("SIZE\n", "SIZE 1920 1080\n")] -#[case("SIZE\nSIZE\n", "SIZE 1920 1080\nSIZE 1920 1080\n")] -#[case("SIZE", "SIZE 1920 1080\n")] +#[case("SIZE", "SIZE 640 480\n")] +#[case("SIZE\n", "SIZE 640 480\n")] +#[case("SIZE\nSIZE\n", "SIZE 640 480\nSIZE 640 480\n")] +#[case("SIZE", "SIZE 640 480\n")] #[case("HELP", std::str::from_utf8(HELP_TEXT).unwrap())] #[case("HELP\n", std::str::from_utf8(HELP_TEXT).unwrap())] -#[case("bla bla bla\nSIZE\nblub\nbla", "SIZE 1920 1080\n")] +#[case("bla bla bla\nSIZE\nblub\nbla", "SIZE 640 480\n")] #[tokio::test] async fn test_correct_responses_to_general_commands( #[case] input: &str, @@ -248,8 +249,8 @@ async fn test_drawing_rect( let mut read_other_pixels_commands = String::new(); let mut read_other_pixels_commands_expected = String::new(); - for x in 0..height { - for y in 0..height { + for x in 0..fb.get_width() { + for y in 0..fb.get_height() { // Inside the rect if x >= offset_x && x <= offset_x + width && y >= offset_y && y <= offset_y + height { fill_commands += &format!("PX {x} {y} {color:06x}\n"); From f265e44a40f3472b08fd750191db094094d22f5f Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 12 Jun 2024 13:25:35 +0200 Subject: [PATCH 9/9] Add benchmark on binary commands --- breakwater-parser/benches/parsing.rs | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/breakwater-parser/benches/parsing.rs b/breakwater-parser/benches/parsing.rs index b43eb42..3324c5f 100644 --- a/breakwater-parser/benches/parsing.rs +++ b/breakwater-parser/benches/parsing.rs @@ -21,6 +21,16 @@ fn compare_implementations(c: &mut Criterion) { false, false, false, + false, + ); + invoke_benchmark( + c, + "parse_binary_draw_commands", + "benches/non-transparent.png", + false, + false, + false, + true, ); invoke_benchmark( c, @@ -29,6 +39,7 @@ fn compare_implementations(c: &mut Criterion) { true, false, false, + false, ); invoke_benchmark( c, @@ -37,6 +48,7 @@ fn compare_implementations(c: &mut Criterion) { true, true, false, + false, ); invoke_benchmark( c, @@ -45,6 +57,7 @@ fn compare_implementations(c: &mut Criterion) { false, false, true, + false, ); } @@ -55,6 +68,7 @@ fn invoke_benchmark( shuffle: bool, use_offset: bool, use_gray: bool, + binary_usage: bool, ) { let commands = image_handler::load( vec![image], @@ -64,6 +78,7 @@ fn invoke_benchmark( .shuffle(shuffle) .offset_usage(use_offset) .gray_usage(use_gray) + .binary_usage(binary_usage) .chunks(1) .build(), ); @@ -86,10 +101,10 @@ fn invoke_benchmark( let fb = Arc::new(FrameBuffer::new(FRAMEBUFFER_WIDTH, FRAMEBUFFER_HEIGHT)); - #[cfg(target_arch = "x86_64")] - let parser_names = ["original", "refactored", "memchr", "assembler"]; - #[cfg(not(target_arch = "x86_64"))] - let parser_names = ["original", "refactored", "memchr"]; + let parser_names = vec!["original", "refactored" /*"memchr"*/]; + + // #[cfg(target_arch = "x86_64")] + // parser_names.push("assembler"); for parse_name in parser_names { c_group.bench_with_input(parse_name, &commands, |b, input| {