From 32f049f3c7dde16435a71f1ab8c3b0102fbbcd2c Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 12 Jun 2024 13:28:06 +0200 Subject: [PATCH] feat: Support binary protocol (#33) --- .github/workflows/build.yml | 8 ++-- CHANGELOG.md | 2 + README.md | 6 +++ breakwater-core/Cargo.toml | 1 + breakwater-core/src/lib.rs | 7 ++- breakwater-parser/Cargo.toml | 1 + breakwater-parser/benches/parsing.rs | 23 ++++++++-- breakwater-parser/src/original.rs | 17 ++++++++ breakwater-parser/src/refactored.rs | 25 ++++++++++- breakwater/Cargo.toml | 6 ++- breakwater/src/tests.rs | 64 ++++++++++++++++++++++++---- 11 files changed, 139 insertions(+), 21 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/CHANGELOG.md b/CHANGELOG.md index 2f19e6e..a39fd03 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 ([#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]) @@ -21,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 :) 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/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..e3828e8 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 a 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-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| { diff --git a/breakwater-parser/src/original.rs b/breakwater-parser/src/original.rs index e35b38a..801149b 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,22 @@ 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); + + // TODO: Support alpha channel (behind alpha feature flag) + 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-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); 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"] diff --git a/breakwater/src/tests.rs b/breakwater/src/tests.rs index 42fc8dd..abfcb1a 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, @@ -18,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] @@ -35,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, @@ -136,6 +139,49 @@ 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")] +// 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, + #[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 +226,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() - 5, fb().get_height() - 5)] #[tokio::test] async fn test_drawing_rect( #[case] width: usize, @@ -204,7 +250,7 @@ async fn test_drawing_rect( let mut read_other_pixels_commands_expected = String::new(); for x in 0..fb.get_width() { - for y in 0..height { + 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");