From 619b310f91b670367ff5661282d367f2e3ed959f Mon Sep 17 00:00:00 2001 From: bits0rcerer <25325997+bits0rcerer@users.noreply.github.com> Date: Fri, 2 Aug 2024 17:39:26 +0200 Subject: [PATCH 01/12] decouple breakwater-parser --- Cargo.lock | 2 +- breakwater-core/Cargo.toml | 1 + breakwater-core/src/framebuffer.rs | 23 ++++++++++++++++++ breakwater-parser/Cargo.toml | 2 -- breakwater-parser/src/assembler.rs | 12 ++++++---- breakwater-parser/src/lib.rs | 37 +++++++++++++++++++++++++---- breakwater-parser/src/memchr.rs | 22 ++++++++++------- breakwater-parser/src/original.rs | 22 +++++++++-------- breakwater-parser/src/refactored.rs | 21 ++++++++-------- breakwater/src/server.rs | 3 ++- 10 files changed, 103 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 78db8ae..de6d0f8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -264,6 +264,7 @@ dependencies = [ name = "breakwater-core" version = "0.15.0" dependencies = [ + "breakwater-parser", "const_format", "tokio", ] @@ -272,7 +273,6 @@ dependencies = [ name = "breakwater-parser" version = "0.15.0" dependencies = [ - "breakwater-core", "criterion", "enum_dispatch", "memchr", diff --git a/breakwater-core/Cargo.toml b/breakwater-core/Cargo.toml index 665e770..2c2adf5 100644 --- a/breakwater-core/Cargo.toml +++ b/breakwater-core/Cargo.toml @@ -8,6 +8,7 @@ edition.workspace = true repository.workspace = true [dependencies] +breakwater-parser = { path = "../breakwater-parser"} const_format.workspace = true tokio.workspace = true diff --git a/breakwater-core/src/framebuffer.rs b/breakwater-core/src/framebuffer.rs index 61f7eda..88b0a4d 100644 --- a/breakwater-core/src/framebuffer.rs +++ b/breakwater-core/src/framebuffer.rs @@ -1,5 +1,6 @@ use std::slice; + pub struct FrameBuffer { width: usize, height: usize, @@ -67,3 +68,25 @@ impl FrameBuffer { unsafe { slice::from_raw_parts(self.buffer.as_ptr() as *const u8, len_in_bytes) } } } + +impl breakwater_parser::FrameBuffer for FrameBuffer { + fn get_width(&self) -> usize { + self.get_width() + } + + fn get_height(&self) -> usize { + self.get_height() + } + + fn get_unchecked(&self, x: usize, y: usize) -> u32 { + self.get_unchecked(x, y) + } + + fn set(&self, x: usize, y: usize, rgba: u32) { + self.set(x, y, rgba) + } + + fn get_buffer(&self) -> &[u32] { + self.get_buffer() + } +} diff --git a/breakwater-parser/Cargo.toml b/breakwater-parser/Cargo.toml index 35ba5d6..f571c76 100644 --- a/breakwater-parser/Cargo.toml +++ b/breakwater-parser/Cargo.toml @@ -12,8 +12,6 @@ name = "parsing" harness = false [dependencies] -breakwater-core.workspace = true - enum_dispatch.workspace = true memchr.workspace = true diff --git a/breakwater-parser/src/assembler.rs b/breakwater-parser/src/assembler.rs index 0947fe4..f111f72 100644 --- a/breakwater-parser/src/assembler.rs +++ b/breakwater-parser/src/assembler.rs @@ -1,13 +1,17 @@ -use std::arch::asm; +use std::{arch::asm, sync::Arc}; -use crate::Parser; +use crate::{FrameBuffer, Parser}; const PARSER_LOOKAHEAD: usize = "PX 1234 1234 rrggbbaa\n".len(); // Longest possible command #[derive(Default)] -pub struct AssemblerParser {} +pub struct AssemblerParser { + help_text: &'static [u8], + alt_help_text: &'static [u8], + fb: Arc, +} -impl Parser for AssemblerParser { +impl Parser for AssemblerParser { fn parse(&mut self, buffer: &[u8], _response: &mut Vec) -> usize { let mut last_byte_parsed = 0; diff --git a/breakwater-parser/src/lib.rs b/breakwater-parser/src/lib.rs index 638eb00..89916f2 100644 --- a/breakwater-parser/src/lib.rs +++ b/breakwater-parser/src/lib.rs @@ -19,10 +19,37 @@ pub trait Parser { } #[enum_dispatch] -pub enum ParserImplementation { - Original(original::OriginalParser), - Refactored(refactored::RefactoredParser), - Naive(memchr::MemchrParser), +pub enum ParserImplementation { + Original(original::OriginalParser), + Refactored(refactored::RefactoredParser), + Naive(memchr::MemchrParser), #[cfg(target_arch = "x86_64")] - Assembler(assembler::AssemblerParser), + Assembler(assembler::AssemblerParser), +} + +pub trait FrameBuffer { + fn get_width(&self) -> usize; + fn get_height(&self) -> usize; + fn get_size(&self) -> usize { + self.get_width() * self.get_height() + } + + #[inline] + fn get(&self, x: usize, y: usize) -> Option { + if x < self.get_width() && y < self.get_height() { + Some(self.get_unchecked(x, y)) + } else { + None + } + } + fn get_unchecked(&self, x: usize, y: usize) -> u32; + + fn set(&self, x: usize, y: usize, rgba: u32); + + #[inline] + fn as_bytes(&self) -> &[u8] { + let len_in_bytes = self.get_buffer().len() * 4; + unsafe { std::slice::from_raw_parts(self.get_buffer().as_ptr() as *const u8, len_in_bytes) } + } + fn get_buffer(&self) -> &[u32]; } diff --git a/breakwater-parser/src/memchr.rs b/breakwater-parser/src/memchr.rs index 6233133..b726120 100644 --- a/breakwater-parser/src/memchr.rs +++ b/breakwater-parser/src/memchr.rs @@ -1,20 +1,24 @@ use std::sync::Arc; -use breakwater_core::framebuffer::FrameBuffer; +use crate::{FrameBuffer, Parser}; -use crate::Parser; - -pub struct MemchrParser { - fb: Arc, +pub struct MemchrParser { + help_text: &'static [u8], + alt_help_text: &'static [u8], + fb: Arc, } -impl MemchrParser { - pub fn new(fb: Arc) -> Self { - Self { fb } +impl MemchrParser { + pub fn new(fb: Arc, help_text: &'static [u8], alt_help_text: &'static [u8]) -> Self { + Self { + fb, + help_text, + alt_help_text, + } } } -impl Parser for MemchrParser { +impl Parser for MemchrParser { fn parse(&mut self, buffer: &[u8], _response: &mut Vec) -> usize { let mut last_char_after_newline = 0; for newline in memchr::memchr_iter(b'\n', buffer) { diff --git a/breakwater-parser/src/original.rs b/breakwater-parser/src/original.rs index 801149b..936d748 100644 --- a/breakwater-parser/src/original.rs +++ b/breakwater-parser/src/original.rs @@ -3,9 +3,7 @@ use std::{ sync::Arc, }; -use breakwater_core::{framebuffer::FrameBuffer, ALT_HELP_TEXT, HELP_TEXT}; - -use crate::Parser; +use crate::{FrameBuffer, Parser}; pub const PARSER_LOOKAHEAD: usize = "PX 1234 1234 rrggbbaa\n".len(); // Longest possible command @@ -15,23 +13,27 @@ 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"); -pub struct OriginalParser { +pub struct OriginalParser { connection_x_offset: usize, connection_y_offset: usize, - fb: Arc, + help_text: &'static [u8], + alt_help_text: &'static [u8], + fb: Arc, } -impl OriginalParser { - pub fn new(fb: Arc) -> Self { +impl OriginalParser { + pub fn new(fb: Arc, help_text: &'static [u8], alt_help_text: &'static [u8]) -> Self { Self { connection_x_offset: 0, connection_y_offset: 0, fb, + help_text, + alt_help_text, } } } -impl Parser for OriginalParser { +impl Parser for OriginalParser { fn parse(&mut self, buffer: &[u8], response: &mut Vec) -> usize { let mut last_byte_parsed = 0; let mut help_count = 0; @@ -183,11 +185,11 @@ impl Parser for OriginalParser { match help_count { 0..=2 => { - response.extend_from_slice(HELP_TEXT); + response.extend_from_slice(self.help_text); help_count += 1; } 3 => { - response.extend_from_slice(ALT_HELP_TEXT); + response.extend_from_slice(self.alt_help_text); help_count += 1; } _ => { diff --git a/breakwater-parser/src/refactored.rs b/breakwater-parser/src/refactored.rs index 4bdf336..5905ef5 100644 --- a/breakwater-parser/src/refactored.rs +++ b/breakwater-parser/src/refactored.rs @@ -1,29 +1,30 @@ use std::sync::Arc; -use breakwater_core::{framebuffer::FrameBuffer, HELP_TEXT}; - use crate::{ original::{ parse_pixel_coordinates, simd_unhex, HELP_PATTERN, OFFSET_PATTERN, PB_PATTERN, PX_PATTERN, SIZE_PATTERN, - }, - Parser, + }, FrameBuffer, Parser }; const PARSER_LOOKAHEAD: usize = "PX 1234 1234 rrggbbaa\n".len(); // Longest possible command -pub struct RefactoredParser { +pub struct RefactoredParser { connection_x_offset: usize, connection_y_offset: usize, - fb: Arc, + help_text: &'static [u8], + alt_help_text: &'static [u8], + fb: Arc, } -impl RefactoredParser { - pub fn new(fb: Arc) -> Self { +impl RefactoredParser { + pub fn new(fb: Arc, help_text: &'static [u8], alt_help_text: &'static [u8]) -> Self { Self { connection_x_offset: 0, connection_y_offset: 0, fb, + help_text, + alt_help_text, } } @@ -122,7 +123,7 @@ impl RefactoredParser { #[inline(always)] fn handle_help(&self, response: &mut Vec) { - response.extend_from_slice(HELP_TEXT); + response.extend_from_slice(self.help_text); } #[inline(always)] @@ -192,7 +193,7 @@ impl RefactoredParser { } } -impl Parser for RefactoredParser { +impl Parser for RefactoredParser { fn parse(&mut self, buffer: &[u8], response: &mut Vec) -> usize { let mut last_byte_parsed = 0; diff --git a/breakwater/src/server.rs b/breakwater/src/server.rs index d378228..80d71f0 100644 --- a/breakwater/src/server.rs +++ b/breakwater/src/server.rs @@ -4,6 +4,7 @@ use std::collections::HashMap; use std::{cmp::min, net::IpAddr, sync::Arc, time::Duration}; use breakwater_core::{framebuffer::FrameBuffer, CONNECTION_DENIED_TEXT}; +use breakwater_core::{ALT_HELP_TEXT, HELP_TEXT}; use breakwater_parser::{original::OriginalParser, Parser}; use log::{debug, info, warn}; use memadvise::{Advice, MemAdviseError}; @@ -178,7 +179,7 @@ pub async fn handle_connection( // Not using `ParserImplementation` to avoid the dynamic dispatch. // let mut parser = ParserImplementation::Simple(SimpleParser::new(fb)); - let mut parser = OriginalParser::new(fb); + let mut parser = OriginalParser::new(fb, HELP_TEXT, ALT_HELP_TEXT); let parser_lookahead = parser.parser_lookahead(); // If we send e.g. an StatisticsEvent::BytesRead for every time we read something from the socket the statistics thread would go crazy. From 301e02340b9c4382581175bc2322933d1f46ce8d Mon Sep 17 00:00:00 2001 From: bits0rcerer <25325997+bits0rcerer@users.noreply.github.com> Date: Fri, 2 Aug 2024 17:47:32 +0200 Subject: [PATCH 02/12] clippy --- breakwater-core/Cargo.toml | 2 +- breakwater-parser/src/assembler.rs | 1 + breakwater-parser/src/memchr.rs | 1 + breakwater-parser/src/refactored.rs | 1 + 4 files changed, 4 insertions(+), 1 deletion(-) diff --git a/breakwater-core/Cargo.toml b/breakwater-core/Cargo.toml index 2c2adf5..a62c0cc 100644 --- a/breakwater-core/Cargo.toml +++ b/breakwater-core/Cargo.toml @@ -8,7 +8,7 @@ edition.workspace = true repository.workspace = true [dependencies] -breakwater-parser = { path = "../breakwater-parser"} +breakwater-parser.workspace = true const_format.workspace = true tokio.workspace = true diff --git a/breakwater-parser/src/assembler.rs b/breakwater-parser/src/assembler.rs index f111f72..6c85530 100644 --- a/breakwater-parser/src/assembler.rs +++ b/breakwater-parser/src/assembler.rs @@ -5,6 +5,7 @@ use crate::{FrameBuffer, Parser}; const PARSER_LOOKAHEAD: usize = "PX 1234 1234 rrggbbaa\n".len(); // Longest possible command #[derive(Default)] +#[allow(dead_code)] pub struct AssemblerParser { help_text: &'static [u8], alt_help_text: &'static [u8], diff --git a/breakwater-parser/src/memchr.rs b/breakwater-parser/src/memchr.rs index b726120..3d6c276 100644 --- a/breakwater-parser/src/memchr.rs +++ b/breakwater-parser/src/memchr.rs @@ -2,6 +2,7 @@ use std::sync::Arc; use crate::{FrameBuffer, Parser}; +#[allow(dead_code)] pub struct MemchrParser { help_text: &'static [u8], alt_help_text: &'static [u8], diff --git a/breakwater-parser/src/refactored.rs b/breakwater-parser/src/refactored.rs index 5905ef5..d0c26ac 100644 --- a/breakwater-parser/src/refactored.rs +++ b/breakwater-parser/src/refactored.rs @@ -9,6 +9,7 @@ use crate::{ const PARSER_LOOKAHEAD: usize = "PX 1234 1234 rrggbbaa\n".len(); // Longest possible command +#[allow(dead_code)] pub struct RefactoredParser { connection_x_offset: usize, connection_y_offset: usize, From c886f00f326464ed3975288d0cceefe2c3aa4dff Mon Sep 17 00:00:00 2001 From: bits0rcerer <25325997+bits0rcerer@users.noreply.github.com> Date: Fri, 2 Aug 2024 17:49:02 +0200 Subject: [PATCH 03/12] #[inline] --- breakwater-core/src/framebuffer.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/breakwater-core/src/framebuffer.rs b/breakwater-core/src/framebuffer.rs index 88b0a4d..ff2269e 100644 --- a/breakwater-core/src/framebuffer.rs +++ b/breakwater-core/src/framebuffer.rs @@ -70,22 +70,27 @@ impl FrameBuffer { } impl breakwater_parser::FrameBuffer for FrameBuffer { + #[inline] fn get_width(&self) -> usize { self.get_width() } + #[inline] fn get_height(&self) -> usize { self.get_height() } + #[inline] fn get_unchecked(&self, x: usize, y: usize) -> u32 { self.get_unchecked(x, y) } + #[inline] fn set(&self, x: usize, y: usize, rgba: u32) { self.set(x, y, rgba) } + #[inline] fn get_buffer(&self) -> &[u32] { self.get_buffer() } From 6b94760b1edd7befa8c472de50d2e6e54a9ddcfa Mon Sep 17 00:00:00 2001 From: bits0rcerer <25325997+bits0rcerer@users.noreply.github.com> Date: Fri, 2 Aug 2024 18:00:04 +0200 Subject: [PATCH 04/12] unchecked is unsafe --- breakwater-core/src/framebuffer.rs | 2 +- breakwater-parser/src/lib.rs | 4 ++-- breakwater-parser/src/original.rs | 2 +- breakwater-parser/src/refactored.rs | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/breakwater-core/src/framebuffer.rs b/breakwater-core/src/framebuffer.rs index ff2269e..caf357b 100644 --- a/breakwater-core/src/framebuffer.rs +++ b/breakwater-core/src/framebuffer.rs @@ -81,7 +81,7 @@ impl breakwater_parser::FrameBuffer for FrameBuffer { } #[inline] - fn get_unchecked(&self, x: usize, y: usize) -> u32 { + unsafe fn get_unchecked(&self, x: usize, y: usize) -> u32 { self.get_unchecked(x, y) } diff --git a/breakwater-parser/src/lib.rs b/breakwater-parser/src/lib.rs index 89916f2..dcdfabf 100644 --- a/breakwater-parser/src/lib.rs +++ b/breakwater-parser/src/lib.rs @@ -37,12 +37,12 @@ pub trait FrameBuffer { #[inline] fn get(&self, x: usize, y: usize) -> Option { if x < self.get_width() && y < self.get_height() { - Some(self.get_unchecked(x, y)) + Some(unsafe { self.get_unchecked(x, y) }) } else { None } } - fn get_unchecked(&self, x: usize, y: usize) -> u32; + unsafe fn get_unchecked(&self, x: usize, y: usize) -> u32; fn set(&self, x: usize, y: usize, rgba: u32); diff --git a/breakwater-parser/src/original.rs b/breakwater-parser/src/original.rs index 936d748..047f959 100644 --- a/breakwater-parser/src/original.rs +++ b/breakwater-parser/src/original.rs @@ -96,7 +96,7 @@ impl Parser for OriginalParser { } let alpha_comp = 0xff - alpha; - let current = self.fb.get_unchecked(x, y); + let current = unsafe { self.fb.get_unchecked(x, y)} ; let r = (rgba >> 16) & 0xff; let g = (rgba >> 8) & 0xff; let b = rgba & 0xff; diff --git a/breakwater-parser/src/refactored.rs b/breakwater-parser/src/refactored.rs index d0c26ac..cf35eea 100644 --- a/breakwater-parser/src/refactored.rs +++ b/breakwater-parser/src/refactored.rs @@ -154,7 +154,7 @@ impl RefactoredParser { } let alpha_comp = 0xff - alpha; - let current = self.fb.get_unchecked(x, y); + let current = unsafe { self.fb.get_unchecked(x, y) }; let r = (rgba >> 16) & 0xff; let g = (rgba >> 8) & 0xff; let b = rgba & 0xff; From eff5aa6c160d6cdf3830a819b7cc53268d0f8b40 Mon Sep 17 00:00:00 2001 From: bits0rcerer <25325997+bits0rcerer@users.noreply.github.com> Date: Fri, 2 Aug 2024 18:09:52 +0200 Subject: [PATCH 05/12] remove not required trait methods --- breakwater-core/src/framebuffer.rs | 5 ----- breakwater-parser/src/lib.rs | 9 ++------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/breakwater-core/src/framebuffer.rs b/breakwater-core/src/framebuffer.rs index caf357b..0039283 100644 --- a/breakwater-core/src/framebuffer.rs +++ b/breakwater-core/src/framebuffer.rs @@ -89,9 +89,4 @@ impl breakwater_parser::FrameBuffer for FrameBuffer { fn set(&self, x: usize, y: usize, rgba: u32) { self.set(x, y, rgba) } - - #[inline] - fn get_buffer(&self) -> &[u32] { - self.get_buffer() - } } diff --git a/breakwater-parser/src/lib.rs b/breakwater-parser/src/lib.rs index dcdfabf..5b79b9b 100644 --- a/breakwater-parser/src/lib.rs +++ b/breakwater-parser/src/lib.rs @@ -42,14 +42,9 @@ pub trait FrameBuffer { None } } + /// # Safety + /// make sure x and y are in bounds unsafe fn get_unchecked(&self, x: usize, y: usize) -> u32; fn set(&self, x: usize, y: usize, rgba: u32); - - #[inline] - fn as_bytes(&self) -> &[u8] { - let len_in_bytes = self.get_buffer().len() * 4; - unsafe { std::slice::from_raw_parts(self.get_buffer().as_ptr() as *const u8, len_in_bytes) } - } - fn get_buffer(&self) -> &[u32]; } From ebfa821e41317a496af3673fbabf91c6ee5622b1 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Sat, 3 Aug 2024 20:35:32 +0200 Subject: [PATCH 06/12] Formatting --- breakwater-core/src/framebuffer.rs | 1 - breakwater-parser/src/lib.rs | 3 +++ breakwater-parser/src/original.rs | 2 +- breakwater-parser/src/refactored.rs | 5 +++-- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/breakwater-core/src/framebuffer.rs b/breakwater-core/src/framebuffer.rs index 0039283..6cb400c 100644 --- a/breakwater-core/src/framebuffer.rs +++ b/breakwater-core/src/framebuffer.rs @@ -1,6 +1,5 @@ use std::slice; - pub struct FrameBuffer { width: usize, height: usize, diff --git a/breakwater-parser/src/lib.rs b/breakwater-parser/src/lib.rs index 5b79b9b..381a79c 100644 --- a/breakwater-parser/src/lib.rs +++ b/breakwater-parser/src/lib.rs @@ -29,7 +29,9 @@ pub enum ParserImplementation { pub trait FrameBuffer { fn get_width(&self) -> usize; + fn get_height(&self) -> usize; + fn get_size(&self) -> usize { self.get_width() * self.get_height() } @@ -42,6 +44,7 @@ pub trait FrameBuffer { None } } + /// # Safety /// make sure x and y are in bounds unsafe fn get_unchecked(&self, x: usize, y: usize) -> u32; diff --git a/breakwater-parser/src/original.rs b/breakwater-parser/src/original.rs index 047f959..a0f587d 100644 --- a/breakwater-parser/src/original.rs +++ b/breakwater-parser/src/original.rs @@ -96,7 +96,7 @@ impl Parser for OriginalParser { } let alpha_comp = 0xff - alpha; - let current = unsafe { self.fb.get_unchecked(x, y)} ; + let current = unsafe { self.fb.get_unchecked(x, y) }; let r = (rgba >> 16) & 0xff; let g = (rgba >> 8) & 0xff; let b = rgba & 0xff; diff --git a/breakwater-parser/src/refactored.rs b/breakwater-parser/src/refactored.rs index cf35eea..a87b338 100644 --- a/breakwater-parser/src/refactored.rs +++ b/breakwater-parser/src/refactored.rs @@ -4,13 +4,14 @@ use crate::{ original::{ parse_pixel_coordinates, simd_unhex, HELP_PATTERN, OFFSET_PATTERN, PB_PATTERN, PX_PATTERN, SIZE_PATTERN, - }, FrameBuffer, Parser + }, + FrameBuffer, Parser, }; const PARSER_LOOKAHEAD: usize = "PX 1234 1234 rrggbbaa\n".len(); // Longest possible command #[allow(dead_code)] -pub struct RefactoredParser { +pub struct RefactoredParser { connection_x_offset: usize, connection_y_offset: usize, help_text: &'static [u8], From 408aee7691264850389a34fe1d116859a8ba21ee Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Sat, 3 Aug 2024 22:16:15 +0200 Subject: [PATCH 07/12] refactor!: Remove breakwater-core crate --- CHANGELOG.md | 7 ++ Cargo.lock | 24 +----- Cargo.toml | 3 +- breakwater-core/Cargo.toml | 17 ---- breakwater-core/src/lib.rs | 30 -------- breakwater-core/src/test_helpers/mod.rs | 5 -- breakwater-parser/Cargo.toml | 2 +- breakwater-parser/benches/parsing.rs | 31 ++++---- breakwater-parser/src/assembler.rs | 12 +-- breakwater-parser/src/framebuffer/mod.rs | 28 +++++++ .../src/framebuffer/simple.rs | 68 ++++------------ breakwater-parser/src/lib.rs | 77 +++++++++---------- breakwater-parser/src/memchr.rs | 11 +-- breakwater-parser/src/original.rs | 12 +-- breakwater-parser/src/refactored.rs | 11 +-- breakwater/Cargo.toml | 5 +- breakwater/src/main.rs | 7 +- breakwater/src/server.rs | 20 ++--- breakwater/src/sinks/ffmpeg.rs | 10 +-- breakwater/src/sinks/vnc.rs | 25 +++--- .../src/test_helpers/dev_null_tcp_stream.rs | 0 .../src/test_helpers/mock_tcp_stream.rs | 0 breakwater/src/test_helpers/mod.rs | 2 + breakwater/src/tests.rs | 27 +++---- 24 files changed, 174 insertions(+), 260 deletions(-) delete mode 100644 breakwater-core/Cargo.toml delete mode 100644 breakwater-core/src/lib.rs delete mode 100644 breakwater-core/src/test_helpers/mod.rs create mode 100644 breakwater-parser/src/framebuffer/mod.rs rename breakwater-core/src/framebuffer.rs => breakwater-parser/src/framebuffer/simple.rs (52%) rename {breakwater-core => breakwater}/src/test_helpers/dev_null_tcp_stream.rs (100%) rename {breakwater-core => breakwater}/src/test_helpers/mock_tcp_stream.rs (100%) create mode 100644 breakwater/src/test_helpers/mod.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index db7f5bc..7a82735 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,13 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Changed + +- BREAKING: Remove the `breakwater-core` crate ([#35]) +- Add a `FrameBuffer` trait, rename the existing implementation one to `SimpleFrameBuffer` ([#35]) + +[#35]: https://github.com/sbernauer/breakwater/pull/35 + ## [0.15.0] - 2024-06-12 ### Added diff --git a/Cargo.lock b/Cargo.lock index de6d0f8..5abbb9c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -238,7 +238,6 @@ checksum = "7c12d1856e42f0d817a835fe55853957c85c8c8a470114029143d3f12671446e" name = "breakwater" version = "0.15.0" dependencies = [ - "breakwater-core", "breakwater-parser", "chrono", "clap", @@ -260,21 +259,12 @@ dependencies = [ "vncserver", ] -[[package]] -name = "breakwater-core" -version = "0.15.0" -dependencies = [ - "breakwater-parser", - "const_format", - "tokio", -] - [[package]] name = "breakwater-parser" version = "0.15.0" dependencies = [ + "const_format", "criterion", - "enum_dispatch", "memchr", "pixelbomber", ] @@ -598,18 +588,6 @@ version = "1.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3dca9240753cf90908d7e4aac30f630662b02aebaa1b58a3cadabdb23385b58b" -[[package]] -name = "enum_dispatch" -version = "0.3.13" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "aa18ce2bc66555b3218614519ac839ddb759a7d6720732f979ef8d13be147ecd" -dependencies = [ - "once_cell", - "proc-macro2", - "quote", - "syn", -] - [[package]] name = "env_filter" version = "0.1.0" diff --git a/Cargo.toml b/Cargo.toml index bc0691e..9606c3b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,5 +1,5 @@ [workspace] -members = ["breakwater-core", "breakwater-parser", "breakwater"] +members = ["breakwater-parser", "breakwater"] resolver = "2" [workspace.package] @@ -14,7 +14,6 @@ chrono = "0.4" clap = { version = "4.5", features = ["derive"] } const_format = "0.2" criterion = {version = "0.5", features = ["async_tokio"]} -enum_dispatch = "0.3" env_logger = "0.11" log = "0.4" memadvise = "0.1" diff --git a/breakwater-core/Cargo.toml b/breakwater-core/Cargo.toml deleted file mode 100644 index a62c0cc..0000000 --- a/breakwater-core/Cargo.toml +++ /dev/null @@ -1,17 +0,0 @@ -[package] -name = "breakwater-core" -description = "Core structs" -version.workspace = true -authors.workspace = true -license.workspace = true -edition.workspace = true -repository.workspace = true - -[dependencies] -breakwater-parser.workspace = true -const_format.workspace = true -tokio.workspace = true - -[features] -alpha = [] -binary-commands = [] diff --git a/breakwater-core/src/lib.rs b/breakwater-core/src/lib.rs deleted file mode 100644 index e3828e8..0000000 --- a/breakwater-core/src/lib.rs +++ /dev/null @@ -1,30 +0,0 @@ -use const_format::formatcp; - -pub mod framebuffer; -pub mod test_helpers; - -pub const HELP_TEXT: &[u8] = formatcp!("\ -Pixelflut server powered by breakwater https://github.com/sbernauer/breakwater -Available commands: -HELP: Show this help -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` -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(); - -pub const ALT_HELP_TEXT: &[u8] = b"Stop spamming HELP!\n"; -pub const CONNECTION_DENIED_TEXT: &[u8] = b"Connection denied as connection limit is reached"; diff --git a/breakwater-core/src/test_helpers/mod.rs b/breakwater-core/src/test_helpers/mod.rs deleted file mode 100644 index 78968bc..0000000 --- a/breakwater-core/src/test_helpers/mod.rs +++ /dev/null @@ -1,5 +0,0 @@ -pub use dev_null_tcp_stream::*; -pub use mock_tcp_stream::*; - -mod dev_null_tcp_stream; -mod mock_tcp_stream; diff --git a/breakwater-parser/Cargo.toml b/breakwater-parser/Cargo.toml index f571c76..339bc6c 100644 --- a/breakwater-parser/Cargo.toml +++ b/breakwater-parser/Cargo.toml @@ -12,7 +12,7 @@ name = "parsing" harness = false [dependencies] -enum_dispatch.workspace = true +const_format.workspace = true memchr.workspace = true [dev-dependencies] diff --git a/breakwater-parser/benches/parsing.rs b/breakwater-parser/benches/parsing.rs index 3324c5f..91f757f 100644 --- a/breakwater-parser/benches/parsing.rs +++ b/breakwater-parser/benches/parsing.rs @@ -1,11 +1,9 @@ use std::{sync::Arc, time::Duration}; -use breakwater_core::framebuffer::FrameBuffer; #[cfg(target_arch = "x86_64")] -use breakwater_parser::assembler::AssemblerParser; +use breakwater_parser::AssemblerParser; use breakwater_parser::{ - memchr::MemchrParser, original::OriginalParser, refactored::RefactoredParser, Parser, - ParserImplementation, + MemchrParser, OriginalParser, Parser, RefactoredParser, SimpleFrameBuffer, }; use criterion::{criterion_group, criterion_main, Criterion}; use pixelbomber::image_handler::{self, ImageConfigBuilder}; @@ -99,7 +97,10 @@ fn invoke_benchmark( let mut c_group = c.benchmark_group(bench_name); - let fb = Arc::new(FrameBuffer::new(FRAMEBUFFER_WIDTH, FRAMEBUFFER_HEIGHT)); + let fb = Arc::new(SimpleFrameBuffer::new( + FRAMEBUFFER_WIDTH, + FRAMEBUFFER_HEIGHT, + )); let parser_names = vec!["original", "refactored" /*"memchr"*/]; @@ -108,19 +109,13 @@ fn invoke_benchmark( for parse_name in parser_names { c_group.bench_with_input(parse_name, &commands, |b, input| { - b.iter(|| { - let mut parser = match parse_name { - "original" => ParserImplementation::Original(OriginalParser::new(fb.clone())), - "refactored" => { - ParserImplementation::Refactored(RefactoredParser::new(fb.clone())) - } - "memchr" => ParserImplementation::Naive(MemchrParser::new(fb.clone())), - #[cfg(target_arch = "x86_64")] - "assembler" => ParserImplementation::Assembler(AssemblerParser::default()), - _ => panic!("Parser implementation {parse_name} not known"), - }; - - parser.parse(input, &mut Vec::new()); + b.iter(|| match parse_name { + "original" => OriginalParser::new(fb.clone()).parse(input, &mut Vec::new()), + "refactored" => RefactoredParser::new(fb.clone()).parse(input, &mut Vec::new()), + "memchr" => MemchrParser::new(fb.clone()).parse(input, &mut Vec::new()), + #[cfg(target_arch = "x86_64")] + "assembler" => AssemblerParser::new(fb.clone()).parse(input, &mut Vec::new()), + _ => panic!("Parser implementation {parse_name} not known"), }); }); } diff --git a/breakwater-parser/src/assembler.rs b/breakwater-parser/src/assembler.rs index 6c85530..ed947b8 100644 --- a/breakwater-parser/src/assembler.rs +++ b/breakwater-parser/src/assembler.rs @@ -4,12 +4,14 @@ use crate::{FrameBuffer, Parser}; const PARSER_LOOKAHEAD: usize = "PX 1234 1234 rrggbbaa\n".len(); // Longest possible command -#[derive(Default)] -#[allow(dead_code)] pub struct AssemblerParser { - help_text: &'static [u8], - alt_help_text: &'static [u8], - fb: Arc, + _fb: Arc, +} + +impl AssemblerParser { + pub fn new(_fb: Arc) -> Self { + Self { _fb } + } } impl Parser for AssemblerParser { diff --git a/breakwater-parser/src/framebuffer/mod.rs b/breakwater-parser/src/framebuffer/mod.rs new file mode 100644 index 0000000..a7f95a4 --- /dev/null +++ b/breakwater-parser/src/framebuffer/mod.rs @@ -0,0 +1,28 @@ +pub mod simple; + +pub trait FrameBuffer { + fn get_width(&self) -> usize; + + fn get_height(&self) -> usize; + + fn get_size(&self) -> usize { + self.get_width() * self.get_height() + } + + #[inline] + fn get(&self, x: usize, y: usize) -> Option { + if x < self.get_width() && y < self.get_height() { + Some(unsafe { self.get_unchecked(x, y) }) + } else { + None + } + } + + /// # Safety + /// make sure x and y are in bounds + unsafe fn get_unchecked(&self, x: usize, y: usize) -> u32; + + fn set(&self, x: usize, y: usize, rgba: u32); + + fn as_bytes(&self) -> &[u8]; +} diff --git a/breakwater-core/src/framebuffer.rs b/breakwater-parser/src/framebuffer/simple.rs similarity index 52% rename from breakwater-core/src/framebuffer.rs rename to breakwater-parser/src/framebuffer/simple.rs index 6cb400c..46771be 100644 --- a/breakwater-core/src/framebuffer.rs +++ b/breakwater-parser/src/framebuffer/simple.rs @@ -1,50 +1,41 @@ -use std::slice; +use super::FrameBuffer; -pub struct FrameBuffer { +pub struct SimpleFrameBuffer { width: usize, height: usize, buffer: Vec, } -impl FrameBuffer { +impl SimpleFrameBuffer { pub fn new(width: usize, height: usize) -> Self { let mut buffer = Vec::with_capacity(width * height); buffer.resize_with(width * height, || 0); - FrameBuffer { + Self { width, height, buffer, } } +} - pub fn get_width(&self) -> usize { +impl FrameBuffer for SimpleFrameBuffer { + #[inline(always)] + fn get_width(&self) -> usize { self.width } - pub fn get_height(&self) -> usize { - self.height - } - - pub fn get_size(&self) -> usize { - self.width * self.height - } - #[inline(always)] - pub fn get(&self, x: usize, y: usize) -> Option { - if x < self.width && y < self.height { - Some(*unsafe { self.buffer.get_unchecked(x + y * self.width) }) - } else { - None - } + fn get_height(&self) -> usize { + self.height } #[inline(always)] - pub fn get_unchecked(&self, x: usize, y: usize) -> u32 { - *unsafe { self.buffer.get_unchecked(x + y * self.width) } + unsafe fn get_unchecked(&self, x: usize, y: usize) -> u32 { + *self.buffer.get_unchecked(x + y * self.width) } #[inline(always)] - pub fn set(&self, x: usize, y: usize, rgba: u32) { + fn set(&self, x: usize, y: usize, rgba: u32) { // https://github.com/sbernauer/breakwater/pull/11 // If we make the FrameBuffer large enough (e.g. 10_000 x 10_000) we don't need to check the bounds here // (x and y are max 4 digit numbers). Flamegraph has shown 5.21% of runtime in this bound check. On the other @@ -58,34 +49,9 @@ impl FrameBuffer { } } - pub fn get_buffer(&self) -> &[u32] { - &self.buffer - } - - pub fn as_bytes(&self) -> &[u8] { - let len_in_bytes = self.buffer.len() * 4; - unsafe { slice::from_raw_parts(self.buffer.as_ptr() as *const u8, len_in_bytes) } - } -} - -impl breakwater_parser::FrameBuffer for FrameBuffer { - #[inline] - fn get_width(&self) -> usize { - self.get_width() - } - - #[inline] - fn get_height(&self) -> usize { - self.get_height() - } - - #[inline] - unsafe fn get_unchecked(&self, x: usize, y: usize) -> u32 { - self.get_unchecked(x, y) - } - - #[inline] - fn set(&self, x: usize, y: usize, rgba: u32) { - self.set(x, y, rgba) + fn as_bytes(&self) -> &[u8] { + let len = 4 * self.buffer.len(); + let ptr = self.buffer.as_ptr() as *const u8; + unsafe { std::slice::from_raw_parts(ptr, len) } } } diff --git a/breakwater-parser/src/lib.rs b/breakwater-parser/src/lib.rs index 381a79c..336e05b 100644 --- a/breakwater-parser/src/lib.rs +++ b/breakwater-parser/src/lib.rs @@ -1,15 +1,46 @@ // Needed for simple implementation #![feature(portable_simd)] -use enum_dispatch::enum_dispatch; +use const_format::formatcp; + +mod assembler; +mod framebuffer; +mod memchr; +mod original; +mod refactored; #[cfg(target_arch = "x86_64")] -pub mod assembler; -pub mod memchr; -pub mod original; -pub mod refactored; +pub use assembler::AssemblerParser; +pub use framebuffer::{simple::SimpleFrameBuffer, FrameBuffer}; +pub use memchr::MemchrParser; +pub use original::OriginalParser; +pub use refactored::RefactoredParser; + +pub const HELP_TEXT: &[u8] = formatcp!("\ +Pixelflut server powered by breakwater https://github.com/sbernauer/breakwater +Available commands: +HELP: Show this help +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` +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(); + +pub const ALT_HELP_TEXT: &[u8] = b"Stop spamming HELP!\n"; -#[enum_dispatch(ParserImplementation)] pub trait Parser { /// Returns the last byte parsed. The next parsing loop will again contain all data that was not parsed. fn parse(&mut self, buffer: &[u8], response: &mut Vec) -> usize; @@ -17,37 +48,3 @@ pub trait Parser { // Sadly this cant be const (yet?) (https://github.com/rust-lang/rust/issues/71971 and https://github.com/rust-lang/rfcs/pull/2632) fn parser_lookahead(&self) -> usize; } - -#[enum_dispatch] -pub enum ParserImplementation { - Original(original::OriginalParser), - Refactored(refactored::RefactoredParser), - Naive(memchr::MemchrParser), - #[cfg(target_arch = "x86_64")] - Assembler(assembler::AssemblerParser), -} - -pub trait FrameBuffer { - fn get_width(&self) -> usize; - - fn get_height(&self) -> usize; - - fn get_size(&self) -> usize { - self.get_width() * self.get_height() - } - - #[inline] - fn get(&self, x: usize, y: usize) -> Option { - if x < self.get_width() && y < self.get_height() { - Some(unsafe { self.get_unchecked(x, y) }) - } else { - None - } - } - - /// # Safety - /// make sure x and y are in bounds - unsafe fn get_unchecked(&self, x: usize, y: usize) -> u32; - - fn set(&self, x: usize, y: usize, rgba: u32); -} diff --git a/breakwater-parser/src/memchr.rs b/breakwater-parser/src/memchr.rs index 3d6c276..d91327b 100644 --- a/breakwater-parser/src/memchr.rs +++ b/breakwater-parser/src/memchr.rs @@ -2,20 +2,13 @@ use std::sync::Arc; use crate::{FrameBuffer, Parser}; -#[allow(dead_code)] pub struct MemchrParser { - help_text: &'static [u8], - alt_help_text: &'static [u8], fb: Arc, } impl MemchrParser { - pub fn new(fb: Arc, help_text: &'static [u8], alt_help_text: &'static [u8]) -> Self { - Self { - fb, - help_text, - alt_help_text, - } + pub fn new(fb: Arc) -> Self { + Self { fb } } } diff --git a/breakwater-parser/src/original.rs b/breakwater-parser/src/original.rs index a0f587d..b3c685d 100644 --- a/breakwater-parser/src/original.rs +++ b/breakwater-parser/src/original.rs @@ -3,7 +3,7 @@ use std::{ sync::Arc, }; -use crate::{FrameBuffer, Parser}; +use crate::{FrameBuffer, Parser, ALT_HELP_TEXT, HELP_TEXT}; pub const PARSER_LOOKAHEAD: usize = "PX 1234 1234 rrggbbaa\n".len(); // Longest possible command @@ -16,19 +16,15 @@ pub(crate) const HELP_PATTERN: u64 = string_to_number(b"HELP\0\0\0\0"); pub struct OriginalParser { connection_x_offset: usize, connection_y_offset: usize, - help_text: &'static [u8], - alt_help_text: &'static [u8], fb: Arc, } impl OriginalParser { - pub fn new(fb: Arc, help_text: &'static [u8], alt_help_text: &'static [u8]) -> Self { + pub fn new(fb: Arc) -> Self { Self { connection_x_offset: 0, connection_y_offset: 0, fb, - help_text, - alt_help_text, } } } @@ -185,11 +181,11 @@ impl Parser for OriginalParser { match help_count { 0..=2 => { - response.extend_from_slice(self.help_text); + response.extend_from_slice(HELP_TEXT); help_count += 1; } 3 => { - response.extend_from_slice(self.alt_help_text); + response.extend_from_slice(ALT_HELP_TEXT); help_count += 1; } _ => { diff --git a/breakwater-parser/src/refactored.rs b/breakwater-parser/src/refactored.rs index a87b338..8fdccd1 100644 --- a/breakwater-parser/src/refactored.rs +++ b/breakwater-parser/src/refactored.rs @@ -5,28 +5,23 @@ use crate::{ parse_pixel_coordinates, simd_unhex, HELP_PATTERN, OFFSET_PATTERN, PB_PATTERN, PX_PATTERN, SIZE_PATTERN, }, - FrameBuffer, Parser, + FrameBuffer, Parser, HELP_TEXT, }; const PARSER_LOOKAHEAD: usize = "PX 1234 1234 rrggbbaa\n".len(); // Longest possible command -#[allow(dead_code)] pub struct RefactoredParser { connection_x_offset: usize, connection_y_offset: usize, - help_text: &'static [u8], - alt_help_text: &'static [u8], fb: Arc, } impl RefactoredParser { - pub fn new(fb: Arc, help_text: &'static [u8], alt_help_text: &'static [u8]) -> Self { + pub fn new(fb: Arc) -> Self { Self { connection_x_offset: 0, connection_y_offset: 0, fb, - help_text, - alt_help_text, } } @@ -125,7 +120,7 @@ impl RefactoredParser { #[inline(always)] fn handle_help(&self, response: &mut Vec) { - response.extend_from_slice(self.help_text); + response.extend_from_slice(HELP_TEXT); } #[inline(always)] diff --git a/breakwater/Cargo.toml b/breakwater/Cargo.toml index 810ea92..1248c7c 100644 --- a/breakwater/Cargo.toml +++ b/breakwater/Cargo.toml @@ -12,7 +12,6 @@ name = "breakwater" path = "src/main.rs" [dependencies] -breakwater-core.workspace = true breakwater-parser.workspace = true chrono.workspace = true @@ -40,5 +39,5 @@ rstest.workspace = true default = ["vnc", "binary-commands"] vnc = ["dep:vncserver"] -alpha = ["breakwater-core/alpha", "breakwater-parser/alpha"] -binary-commands = ["breakwater-core/binary-commands", "breakwater-parser/binary-commands"] +alpha = ["breakwater-parser/alpha"] +binary-commands = ["breakwater-parser/binary-commands"] diff --git a/breakwater/src/main.rs b/breakwater/src/main.rs index 5402c11..01effae 100644 --- a/breakwater/src/main.rs +++ b/breakwater/src/main.rs @@ -1,6 +1,6 @@ use std::{env, num::TryFromIntError, sync::Arc}; -use breakwater_core::framebuffer::FrameBuffer; +use breakwater_parser::SimpleFrameBuffer; use clap::Parser; use log::{info, trace}; use prometheus_exporter::PrometheusExporter; @@ -26,6 +26,8 @@ mod prometheus_exporter; mod server; mod sinks; mod statistics; +#[cfg(test)] +mod test_helpers; #[cfg(test)] mod tests; @@ -87,7 +89,8 @@ async fn main() -> Result<(), Error> { let args = CliArgs::parse(); - let fb = Arc::new(FrameBuffer::new(args.width, args.height)); + // Not using dynamic dispatch here for performance reasons + let fb = Arc::new(SimpleFrameBuffer::new(args.width, args.height)); // If we make the channel to big, stats will start to lag behind // TODO: Check performance impact in real-world scenario. Maybe the statistics thread blocks the other threads diff --git a/breakwater/src/server.rs b/breakwater/src/server.rs index 80d71f0..d0d0972 100644 --- a/breakwater/src/server.rs +++ b/breakwater/src/server.rs @@ -3,9 +3,7 @@ use std::collections::hash_map::Entry; use std::collections::HashMap; use std::{cmp::min, net::IpAddr, sync::Arc, time::Duration}; -use breakwater_core::{framebuffer::FrameBuffer, CONNECTION_DENIED_TEXT}; -use breakwater_core::{ALT_HELP_TEXT, HELP_TEXT}; -use breakwater_parser::{original::OriginalParser, Parser}; +use breakwater_parser::{FrameBuffer, OriginalParser, Parser}; use log::{debug, info, warn}; use memadvise::{Advice, MemAdviseError}; use snafu::{ResultExt, Snafu}; @@ -18,6 +16,8 @@ use tokio::{ use crate::statistics::StatisticsEvent; +const CONNECTION_DENIED_TEXT: &[u8] = b"Connection denied as connection limit is reached"; + // Every client connection spawns a new thread, so we need to limit the number of stat events we send const STATISTICS_REPORT_INTERVAL: Duration = Duration::from_millis(250); @@ -41,20 +41,20 @@ pub enum Error { }, } -pub struct Server { +pub struct Server { // listen_address: String, listener: TcpListener, - fb: Arc, + fb: Arc, statistics_tx: mpsc::Sender, network_buffer_size: usize, connections_per_ip: HashMap, max_connections_per_ip: Option, } -impl Server { +impl Server { pub async fn new( listen_address: &str, - fb: Arc, + fb: Arc, statistics_tx: mpsc::Sender, network_buffer_size: usize, max_connections_per_ip: Option, @@ -142,10 +142,10 @@ impl Server { } } -pub async fn handle_connection( +pub async fn handle_connection( mut stream: impl AsyncReadExt + AsyncWriteExt + Send + Unpin, ip: IpAddr, - fb: Arc, + fb: Arc, statistics_tx: mpsc::Sender, page_size: usize, network_buffer_size: usize, @@ -179,7 +179,7 @@ pub async fn handle_connection( // Not using `ParserImplementation` to avoid the dynamic dispatch. // let mut parser = ParserImplementation::Simple(SimpleParser::new(fb)); - let mut parser = OriginalParser::new(fb, HELP_TEXT, ALT_HELP_TEXT); + let mut parser = OriginalParser::new(fb); let parser_lookahead = parser.parser_lookahead(); // If we send e.g. an StatisticsEvent::BytesRead for every time we read something from the socket the statistics thread would go crazy. diff --git a/breakwater/src/sinks/ffmpeg.rs b/breakwater/src/sinks/ffmpeg.rs index 26bf6df..ae6e00e 100644 --- a/breakwater/src/sinks/ffmpeg.rs +++ b/breakwater/src/sinks/ffmpeg.rs @@ -1,6 +1,6 @@ use std::{process::Stdio, sync::Arc, time::Duration}; -use breakwater_core::framebuffer::FrameBuffer; +use breakwater_parser::FrameBuffer; use chrono::Local; use log::debug; use snafu::{ResultExt, Snafu}; @@ -25,15 +25,15 @@ pub enum Error { WriteDataToFfmeg { source: std::io::Error }, } -pub struct FfmpegSink { - fb: Arc, +pub struct FfmpegSink { + fb: Arc, rtmp_address: Option, video_save_folder: Option, fps: u32, } -impl FfmpegSink { - pub fn new(args: &CliArgs, fb: Arc) -> Option { +impl FfmpegSink { + pub fn new(args: &CliArgs, fb: Arc) -> Option { if args.rtmp_address.is_some() || args.video_save_folder.is_some() { Some(FfmpegSink { fb, diff --git a/breakwater/src/sinks/vnc.rs b/breakwater/src/sinks/vnc.rs index 496e6b7..83e1da6 100644 --- a/breakwater/src/sinks/vnc.rs +++ b/breakwater/src/sinks/vnc.rs @@ -1,6 +1,6 @@ use std::{sync::Arc, time::Duration}; -use breakwater_core::framebuffer::FrameBuffer; +use breakwater_parser::FrameBuffer; use core::slice; use number_prefix::NumberPrefix; use rusttype::{point, Font, Scale}; @@ -42,9 +42,10 @@ pub enum Error { } // Sorry! Help needed :) -unsafe impl<'a> Send for VncServer<'a> {} -pub struct VncServer<'a> { - fb: Arc, +unsafe impl<'a, FB: FrameBuffer> Send for VncServer<'a, FB> {} + +pub struct VncServer<'a, FB: FrameBuffer> { + fb: Arc, screen: RfbScreenInfoPtr, target_fps: u32, @@ -56,10 +57,10 @@ pub struct VncServer<'a> { font: Font<'a>, } -impl<'a> VncServer<'a> { +impl<'a, FB: FrameBuffer> VncServer<'a, FB> { #[allow(clippy::too_many_arguments)] pub fn new( - fb: Arc, + fb: Arc, port: u16, target_fps: u32, statistics_tx: Sender, @@ -119,8 +120,11 @@ impl<'a> VncServer<'a> { pub fn run(&mut self) -> Result<(), Error> { let target_loop_duration = Duration::from_micros(1_000_000 / self.target_fps as u64); - let vnc_fb_slice: &mut [u32] = unsafe { - slice::from_raw_parts_mut((*self.screen).frameBuffer as *mut u32, self.fb.get_size()) + let vnc_fb_slice: &mut [u8] = unsafe { + slice::from_raw_parts_mut( + (*self.screen).frameBuffer as *mut u8, + self.fb.get_size() * 4, + ) }; // A line less because the (height - STATS_SURFACE_HEIGHT) belongs to the stats and gets refreshed by them let height_up_to_stats_text = self.fb.get_height() - STATS_HEIGHT - 1; @@ -132,8 +136,9 @@ impl<'a> VncServer<'a> { } let start = std::time::Instant::now(); - vnc_fb_slice[0..fb_size_up_to_stats_text] - .copy_from_slice(&self.fb.get_buffer()[0..fb_size_up_to_stats_text]); + + vnc_fb_slice[0..fb_size_up_to_stats_text * 4] + .copy_from_slice(&self.fb.as_bytes()[0..fb_size_up_to_stats_text * 4]); // Only refresh the drawing surface, not the stats surface rfb_mark_rect_as_modified( diff --git a/breakwater-core/src/test_helpers/dev_null_tcp_stream.rs b/breakwater/src/test_helpers/dev_null_tcp_stream.rs similarity index 100% rename from breakwater-core/src/test_helpers/dev_null_tcp_stream.rs rename to breakwater/src/test_helpers/dev_null_tcp_stream.rs diff --git a/breakwater-core/src/test_helpers/mock_tcp_stream.rs b/breakwater/src/test_helpers/mock_tcp_stream.rs similarity index 100% rename from breakwater-core/src/test_helpers/mock_tcp_stream.rs rename to breakwater/src/test_helpers/mock_tcp_stream.rs diff --git a/breakwater/src/test_helpers/mod.rs b/breakwater/src/test_helpers/mod.rs new file mode 100644 index 0000000..d388a74 --- /dev/null +++ b/breakwater/src/test_helpers/mod.rs @@ -0,0 +1,2 @@ +pub mod dev_null_tcp_stream; +pub mod mock_tcp_stream; diff --git a/breakwater/src/tests.rs b/breakwater/src/tests.rs index abfcb1a..154cb3f 100644 --- a/breakwater/src/tests.rs +++ b/breakwater/src/tests.rs @@ -5,12 +5,13 @@ use std::{ sync::Arc, }; -use breakwater_core::{framebuffer::FrameBuffer, test_helpers::MockTcpStream, HELP_TEXT}; +use breakwater_parser::{FrameBuffer, SimpleFrameBuffer, HELP_TEXT}; use rstest::{fixture, rstest}; use tokio::sync::mpsc; use crate::{ cli_args::DEFAULT_NETWORK_BUFFER_SIZE, server::handle_connection, statistics::StatisticsEvent, + test_helpers::mock_tcp_stream::MockTcpStream, }; #[fixture] @@ -19,9 +20,9 @@ fn ip() -> IpAddr { } #[fixture] -fn fb() -> Arc { +fn fb() -> Arc { // We keep the framebuffer so small, so that we can easily test all pixels in a test run - Arc::new(FrameBuffer::new(640, 480)) + Arc::new(SimpleFrameBuffer::new(640, 480)) } #[fixture] @@ -46,11 +47,11 @@ fn statistics_channel() -> ( #[case("HELP\n", std::str::from_utf8(HELP_TEXT).unwrap())] #[case("bla bla bla\nSIZE\nblub\nbla", "SIZE 640 480\n")] #[tokio::test] -async fn test_correct_responses_to_general_commands( +async fn test_correct_responses_to_general_commands( #[case] input: &str, #[case] expected: &str, ip: IpAddr, - fb: Arc, + fb: Arc, statistics_channel: ( mpsc::Sender, mpsc::Receiver, @@ -113,11 +114,11 @@ async fn test_correct_responses_to_general_commands( )] // The get pixel result is also offseted #[case("OFFSET 0 0\nPX 0 42 abcdef\nPX 0 42\n", "PX 0 42 abcdef\n")] #[tokio::test] -async fn test_setting_pixel( +async fn test_setting_pixel( #[case] input: &str, #[case] expected: &str, ip: IpAddr, - fb: Arc, + fb: Arc, statistics_channel: ( mpsc::Sender, mpsc::Receiver, @@ -156,11 +157,11 @@ async fn test_setting_pixel( "PX 0 0 000000\nPX 0 0 313233\n" )] #[tokio::test] -async fn test_binary_commands( +async fn test_binary_commands( #[case] input: &str, #[case] expected: &str, ip: IpAddr, - fb: Arc, + fb: Arc, statistics_channel: ( mpsc::Sender, mpsc::Receiver, @@ -186,10 +187,10 @@ async fn test_binary_commands( #[case("PX 0 0 aaaaaa\n")] #[case("PX 0 0 aa\n")] #[tokio::test] -async fn test_safe( +async fn test_safe( #[case] input: &str, ip: IpAddr, - fb: Arc, + fb: Arc, statistics_channel: ( mpsc::Sender, mpsc::Receiver, @@ -229,13 +230,13 @@ async fn test_safe( // Yes, this exceeds the framebuffer size #[case(10, 10, fb().get_width() - 5, fb().get_height() - 5)] #[tokio::test] -async fn test_drawing_rect( +async fn test_drawing_rect( #[case] width: usize, #[case] height: usize, #[case] offset_x: usize, #[case] offset_y: usize, ip: IpAddr, - fb: Arc, + fb: Arc, statistics_channel: ( mpsc::Sender, mpsc::Receiver, From 9eed1e2b6aeb134400f8a0ada7233210078df05b Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Sun, 4 Aug 2024 11:17:32 +0200 Subject: [PATCH 08/12] Fix compilation error in tests --- breakwater/src/tests.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/breakwater/src/tests.rs b/breakwater/src/tests.rs index c31e9b3..cc2370b 100644 --- a/breakwater/src/tests.rs +++ b/breakwater/src/tests.rs @@ -325,7 +325,7 @@ async fn test_binary_sync_pixels() { #[rstest] #[tokio::test] /// Try painting the very last pixel of the screen. There is only space for a single pixel left. -async fn test_binary_sync_pixels_last_pixel(fb: Arc) { +async fn test_binary_sync_pixels_last_pixel(fb: Arc) { let mut input = Vec::new(); let x = fb.get_width() as u16 - 1; let y = fb.get_height() as u16 - 1; @@ -350,7 +350,7 @@ async fn test_binary_sync_pixels_last_pixel(fb: Arc) { #[rstest] #[tokio::test] /// Try painting some pixels in the middle of the screen -async fn test_binary_sync_pixels_in_the_middle(fb: Arc) { +async fn test_binary_sync_pixels_in_the_middle(fb: Arc) { let mut input = Vec::new(); let mut expected = String::new(); @@ -389,7 +389,7 @@ async fn test_binary_sync_pixels_in_the_middle(fb: Arc) { #[rstest] #[tokio::test] /// Try painting too much pixels, so it overflows the framebuffer. -async fn test_binary_sync_pixels_exceeding_screen(fb: Arc) { +async fn test_binary_sync_pixels_exceeding_screen(fb: Arc) { let mut input = Vec::new(); let x = fb.get_width() as u16 - 1; let y = fb.get_height() as u16 - 1; @@ -410,7 +410,7 @@ async fn test_binary_sync_pixels_exceeding_screen(fb: Arc) { #[tokio::test] /// Try painting more pixels that fit in the buffer. This checks if the parse correctly keeps track of the command /// across multiple parse calls as the pixel screen send is bigger than the buffer. -async fn test_binary_sync_pixels_larger_than_buffer(fb: Arc) { +async fn test_binary_sync_pixels_larger_than_buffer(fb: Arc) { // let fb = Arc::new(FrameBuffer::new(50, 30)); // For testing let num_pixels = (fb.get_width() * fb.get_height()) as u32; From dc52c50b07ed589871995692f415a9e652dd5404 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Sun, 4 Aug 2024 11:21:52 +0200 Subject: [PATCH 09/12] Fix clippy on arm --- breakwater-parser/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/breakwater-parser/src/lib.rs b/breakwater-parser/src/lib.rs index c909010..5f76ed9 100644 --- a/breakwater-parser/src/lib.rs +++ b/breakwater-parser/src/lib.rs @@ -3,6 +3,7 @@ use const_format::formatcp; +#[cfg(target_arch = "x86_64")] mod assembler; mod framebuffer; mod memchr; From 9db72449c1d8d861a0e0c4f6a0088390f1398602 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 8 Aug 2024 11:04:57 +0200 Subject: [PATCH 10/12] Add as_pixels to FrameBuffer trait --- breakwater-parser/src/framebuffer/mod.rs | 2 ++ breakwater-parser/src/framebuffer/simple.rs | 4 ++++ breakwater/src/sinks/vnc.rs | 13 +++++-------- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/breakwater-parser/src/framebuffer/mod.rs b/breakwater-parser/src/framebuffer/mod.rs index 58e9abe..8742d04 100644 --- a/breakwater-parser/src/framebuffer/mod.rs +++ b/breakwater-parser/src/framebuffer/mod.rs @@ -44,4 +44,6 @@ pub trait FrameBuffer { fn set_multi_from_start_index(&self, starting_index: usize, pixels: &[u8]) -> usize; fn as_bytes(&self) -> &[u8]; + + fn as_pixels(&self) -> &[u32]; } diff --git a/breakwater-parser/src/framebuffer/simple.rs b/breakwater-parser/src/framebuffer/simple.rs index d57c2f9..49f7cd5 100644 --- a/breakwater-parser/src/framebuffer/simple.rs +++ b/breakwater-parser/src/framebuffer/simple.rs @@ -79,6 +79,10 @@ impl FrameBuffer for SimpleFrameBuffer { let ptr = self.buffer.as_ptr() as *const u8; unsafe { std::slice::from_raw_parts(ptr, len) } } + + fn as_pixels(&self) -> &[u32] { + &self.buffer + } } #[cfg(test)] diff --git a/breakwater/src/sinks/vnc.rs b/breakwater/src/sinks/vnc.rs index 83e1da6..3c8adae 100644 --- a/breakwater/src/sinks/vnc.rs +++ b/breakwater/src/sinks/vnc.rs @@ -120,12 +120,10 @@ impl<'a, FB: FrameBuffer> VncServer<'a, FB> { pub fn run(&mut self) -> Result<(), Error> { let target_loop_duration = Duration::from_micros(1_000_000 / self.target_fps as u64); - let vnc_fb_slice: &mut [u8] = unsafe { - slice::from_raw_parts_mut( - (*self.screen).frameBuffer as *mut u8, - self.fb.get_size() * 4, - ) + let vnc_fb_slice: &mut [u32] = unsafe { + slice::from_raw_parts_mut((*self.screen).frameBuffer as *mut u32, self.fb.get_size()) }; + // A line less because the (height - STATS_SURFACE_HEIGHT) belongs to the stats and gets refreshed by them let height_up_to_stats_text = self.fb.get_height() - STATS_HEIGHT - 1; let fb_size_up_to_stats_text = self.fb.get_width() * height_up_to_stats_text; @@ -136,9 +134,8 @@ impl<'a, FB: FrameBuffer> VncServer<'a, FB> { } let start = std::time::Instant::now(); - - vnc_fb_slice[0..fb_size_up_to_stats_text * 4] - .copy_from_slice(&self.fb.as_bytes()[0..fb_size_up_to_stats_text * 4]); + vnc_fb_slice[0..fb_size_up_to_stats_text] + .copy_from_slice(&self.fb.as_pixels()[0..fb_size_up_to_stats_text]); // Only refresh the drawing surface, not the stats surface rfb_mark_rect_as_modified( From b0c73bceb1c99d47f9ad2f2379f14ac99b1886e0 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 8 Aug 2024 11:07:09 +0200 Subject: [PATCH 11/12] changelog --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c851a69..5e88699 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,8 +11,8 @@ All notable changes to this project will be documented in this file. ### Changed - BREAKING: Feature `binary-commands` has been renamed to `binary-set-pixel` ([#34]) -- BREAKING: Remove the `breakwater-core` crate ([#35]) -- Add a `FrameBuffer` trait, rename the existing implementation one to `SimpleFrameBuffer` ([#35]) +- BREAKING: Remove the `breakwater-core` crate ([#37]) +- Add a `FrameBuffer` trait, rename the existing implementation one to `SimpleFrameBuffer` ([#37]) ### Fixed @@ -20,8 +20,8 @@ All notable changes to this project will be documented in this file. - Minor performance improvement when parsing `HELP` and `SIZE` requests ([#36]) [#34]: https://github.com/sbernauer/breakwater/pull/34 -[#35]: https://github.com/sbernauer/breakwater/pull/35 [#36]: https://github.com/sbernauer/breakwater/pull/36 +[#37]: https://github.com/sbernauer/breakwater/pull/37 ## [0.15.0] - 2024-06-12 From 4a670034b25dde126513b46605cd655870924d64 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 8 Aug 2024 11:09:58 +0200 Subject: [PATCH 12/12] More inlining :) --- breakwater-parser/src/framebuffer/simple.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/breakwater-parser/src/framebuffer/simple.rs b/breakwater-parser/src/framebuffer/simple.rs index 49f7cd5..1d8d02b 100644 --- a/breakwater-parser/src/framebuffer/simple.rs +++ b/breakwater-parser/src/framebuffer/simple.rs @@ -74,12 +74,14 @@ impl FrameBuffer for SimpleFrameBuffer { num_pixels } + #[inline(always)] fn as_bytes(&self) -> &[u8] { let len = 4 * self.buffer.len(); let ptr = self.buffer.as_ptr() as *const u8; unsafe { std::slice::from_raw_parts(ptr, len) } } + #[inline(always)] fn as_pixels(&self) -> &[u32] { &self.buffer }