Skip to content
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.

Sign up for GitHub

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

fix clippy warnings #49

Merged
merged 2 commits into from
Jan 7, 2024
Merged

fix clippy warnings #49

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6de648a
fix clippy warnings
Jan 5, 2024
88b7c9b
destructure option structs
Jan 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# Version History

## Version 1.2.2 (2024-1-5)

- Fixed clippy warnings
- Added panic documentation

## Version 1.2.1 (2024-1-3)

- Improved performance when decoding near the end of a file
Expand Down
4 changes: 2 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "creek"
version = "1.2.1"
version = "1.2.2"
authors = ["Billy Messenger <[email protected]>"]
edition = "2021"
license = "MIT OR Apache-2.0"
Expand Down Expand Up @@ -57,7 +57,7 @@ decode-all = [
encode-wav = ["creek-encode-wav"]

[dependencies]
creek-core = { version = "0.2.1", path = "core" }
creek-core = { version = "0.2.2", path = "core" }
creek-decode-symphonia = { version = "0.3.1", path = "decode_symphonia", optional = true }
creek-encode-wav = { version = "0.2.0", path = "encode_wav", optional = true }

Expand Down
2 changes: 1 addition & 1 deletion core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "creek-core"
version = "0.2.1"
version = "0.2.2"
authors = ["Billy Messenger <[email protected]>"]
edition = "2021"
license = "MIT OR Apache-2.0"
Expand Down
2 changes: 1 addition & 1 deletion core/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![warn(rust_2018_idioms)]
#![warn(rust_2021_compatibility)]
// TODO: #![warn(clippy::missing_panics_doc)]
#![warn(clippy::missing_panics_doc)]
#![warn(clippy::clone_on_ref_ptr)]
#![deny(trivial_numeric_casts)]
#![forbid(unsafe_code)]
Expand Down
134 changes: 84 additions & 50 deletions core/src/read/read_stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use super::{
ClientToServerMsg, DataBlock, Decoder, HeapData, ReadData, ReadServer, ReadStreamOptions,
ServerToClientMsg,
};
use crate::read::server::ReadServerOptions;
use crate::{FileInfo, SERVER_WAIT_TIME};

/// Describes how to search for suitable caches when seeking in a [`ReadDiskStream`].
Expand Down Expand Up @@ -34,6 +35,15 @@ pub enum SeekMode {
NoCache,
}

struct ReadDiskStreamOptions<D: Decoder> {
start_frame: usize,
num_cache_blocks: usize,
num_look_ahead_blocks: usize,
max_num_caches: usize,
block_size: usize,
file_info: FileInfo<D::FileParams>,
}

/// A realtime-safe disk-streaming reader of audio files.
pub struct ReadDiskStream<D: Decoder> {
to_server_tx: Producer<ClientToServerMsg<D>>,
Expand Down Expand Up @@ -65,6 +75,11 @@ impl<D: Decoder> ReadDiskStream<D> {
/// * `file` - The path to the file to open.
/// * `start_frame` - The frame in the file to start reading from.
/// * `stream_opts` - Additional stream options.
///
/// # Panics
///
/// This will panic if `stream_opts.block_size`, `stream_opts.num_look_ahead_blocks`,
/// or `stream_opts.server_msg_channel_size` is `0`.
pub fn new<P: Into<PathBuf>>(
file: P,
start_frame: usize,
Expand All @@ -91,27 +106,32 @@ impl<D: Decoder> ReadDiskStream<D> {

let file: PathBuf = file.into();

match ReadServer::new(
file,
start_frame,
stream_opts.num_cache_blocks + stream_opts.num_look_ahead_blocks,
stream_opts.block_size,
match ReadServer::spawn(
ReadServerOptions {
file,
start_frame,
num_prefetch_blocks: stream_opts.num_cache_blocks
Copy link
Collaborator

Choose a reason for hiding this comment

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

stream_opts should be destructured instead of accessing all fields individually. This ensures that you don't miss any field. Also applies to other parts of the code.

Should be improved by a follow-up PR.

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean by "destructured"?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I know what you mean now.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, I did it.

+ stream_opts.num_look_ahead_blocks,
block_size: stream_opts.block_size,
additional_opts: stream_opts.additional_opts,
},
to_client_tx,
from_client_rx,
close_signal_rx,
stream_opts.additional_opts,
) {
Ok(file_info) => {
let client = ReadDiskStream::create(
ReadDiskStreamOptions {
start_frame,
num_cache_blocks: stream_opts.num_cache_blocks,
num_look_ahead_blocks: stream_opts.num_look_ahead_blocks,
max_num_caches: stream_opts.num_caches,
block_size: stream_opts.block_size,
file_info,
},
to_server_tx,
from_server_rx,
close_signal_tx,
start_frame,
stream_opts.num_cache_blocks,
stream_opts.num_look_ahead_blocks,
stream_opts.num_caches,
stream_opts.block_size,
file_info,
);

Ok(client)
Expand All @@ -120,24 +140,18 @@ impl<D: Decoder> ReadDiskStream<D> {
}
}

#[allow(clippy::too_many_arguments)] // TODO: Reduce number of arguments
pub(crate) fn create(
fn create(
opts: ReadDiskStreamOptions<D>,
to_server_tx: Producer<ClientToServerMsg<D>>,
from_server_rx: Consumer<ServerToClientMsg<D>>,
close_signal_tx: Producer<Option<HeapData<D::T>>>,
start_frame: usize,
num_cache_blocks: usize,
num_look_ahead_blocks: usize,
max_num_caches: usize,
block_size: usize,
file_info: FileInfo<D::FileParams>,
) -> Self {
let num_prefetch_blocks = num_cache_blocks + num_look_ahead_blocks;
let num_prefetch_blocks = opts.num_cache_blocks + opts.num_look_ahead_blocks;

let read_buffer = DataBlock::new(usize::from(file_info.num_channels), block_size);
let read_buffer = DataBlock::new(usize::from(opts.file_info.num_channels), opts.block_size);

// Reserve the last two caches as temporary caches.
let max_num_caches = max_num_caches + 2;
let max_num_caches = opts.max_num_caches + 2;

let mut caches: Vec<DataBlockCacheEntry<D::T>> = Vec::with_capacity(max_num_caches);
for _ in 0..max_num_caches {
Expand All @@ -152,15 +166,15 @@ impl<D: Decoder> ReadDiskStream<D> {

let mut prefetch_buffer: Vec<DataBlockEntry<D::T>> =
Vec::with_capacity(num_prefetch_blocks);
let mut wanted_start_frame = start_frame;
let mut wanted_start_frame = opts.start_frame;
for _ in 0..num_prefetch_blocks {
prefetch_buffer.push(DataBlockEntry {
use_cache_index: None,
block: None,
wanted_start_frame,
});

wanted_start_frame += block_size;
wanted_start_frame += opts.block_size;
}

let heap_data = Some(HeapData {
Expand All @@ -178,18 +192,18 @@ impl<D: Decoder> ReadDiskStream<D> {

current_block_index: 0,
next_block_index: 1,
current_block_start_frame: start_frame,
current_block_start_frame: opts.start_frame,
current_frame_in_block: 0,

temp_cache_index,
temp_seek_cache_index,

num_prefetch_blocks,
prefetch_size: num_prefetch_blocks * block_size,
cache_size: num_cache_blocks * block_size,
block_size,
prefetch_size: num_prefetch_blocks * opts.block_size,
cache_size: opts.num_cache_blocks * opts.block_size,
block_size: opts.block_size,

file_info,
file_info: opts.file_info,
fatal_error: false,
}
}
Expand Down Expand Up @@ -219,8 +233,10 @@ impl<D: Decoder> ReadDiskStream<D> {
/// relied on, then any blocks relying on the oldest cache will be silenced. In this case, (false)
/// will be returned.
pub fn can_move_cache(&mut self, cache_index: usize) -> bool {
// This check should never fail because it can only be `None` in the destructor.
let heap = self.heap_data.as_ref().unwrap();
let Some(heap) = self.heap_data.as_ref() else {
// This will never return here because `heap_data` can only be `None` in the destructor.
return false;
};

let mut using_cache = false;
let mut using_temp_cache = false;
Expand Down Expand Up @@ -266,8 +282,10 @@ impl<D: Decoder> ReadDiskStream<D> {
return Err(ReadError::FatalError(FatalReadError::StreamClosed));
}

// This check should never fail because it can only be `None` in the destructor.
let heap = self.heap_data.as_mut().unwrap();
let Some(heap) = self.heap_data.as_mut() else {
// This will never return here because `heap_data` can only be `None` in the destructor.
return Ok(false);
};

if cache_index >= heap.caches.len() - 2 {
return Err(ReadError::CacheIndexOutOfRange {
Expand Down Expand Up @@ -373,8 +391,10 @@ impl<D: Decoder> ReadDiskStream<D> {
return Err(ReadError::IOServerChannelFull);
}

// This check should never fail because it can only be `None` in the destructor.
let heap = self.heap_data.as_mut().unwrap();
let Some(heap) = self.heap_data.as_mut() else {
// This will never return here because `heap_data` can only be `None` in the destructor.
return Ok(false);
};

let mut found_cache = None;

Expand Down Expand Up @@ -509,8 +529,10 @@ impl<D: Decoder> ReadDiskStream<D> {
return Ok(false);
}

// This check should never fail because it can only be `None` in the destructor.
let heap = self.heap_data.as_ref().unwrap();
let Some(heap) = self.heap_data.as_mut() else {
// This will never return here because `heap_data` can only be `None` in the destructor.
return Ok(false);
};

// Check if the next two blocks are ready.

Expand Down Expand Up @@ -631,8 +653,10 @@ impl<D: Decoder> ReadDiskStream<D> {

// Retrieve any data sent from the server.

// This check should never fail because it can only be `None` in the destructor.
let heap = self.heap_data.as_mut().unwrap();
let Some(heap) = self.heap_data.as_mut() else {
// This will never return here because `heap_data` can only be `None` in the destructor.
return Ok(());
};

loop {
// Check that there is at-least one slot open before popping the next message.
Expand Down Expand Up @@ -765,8 +789,10 @@ impl<D: Decoder> ReadDiskStream<D> {

// Copy from first block.
{
// This check should never fail because it can only be `None` in the destructor.
let heap = self.heap_data.as_mut().unwrap();
let Some(heap) = self.heap_data.as_mut() else {
// This will never return here because `heap_data` can only be `None` in the destructor.
return Err(ReadError::IOServerChannelFull);
};

heap.read_buffer.clear();

Expand All @@ -782,8 +808,10 @@ impl<D: Decoder> ReadDiskStream<D> {

// Copy from second block
{
// This check should never fail because it can only be `None` in the destructor.
let heap = self.heap_data.as_mut().unwrap();
let Some(heap) = self.heap_data.as_mut() else {
// This will never return here because `heap_data` can only be `None` in the destructor.
return Err(ReadError::IOServerChannelFull);
};

copy_block_into_read_buffer(heap, self.current_block_index, 0, second_len);
}
Expand All @@ -792,8 +820,10 @@ impl<D: Decoder> ReadDiskStream<D> {
} else {
// Only need to copy from current block.
{
// This check should never fail because it can only be `None` in the destructor.
let heap = self.heap_data.as_mut().unwrap();
let Some(heap) = self.heap_data.as_mut() else {
// This will never return here because `heap_data` can only be `None` in the destructor.
return Err(ReadError::IOServerChannelFull);
};

heap.read_buffer.clear();

Expand All @@ -812,8 +842,10 @@ impl<D: Decoder> ReadDiskStream<D> {
}
}

// This check should never fail because it can only be `None` in the destructor.
let heap = self.heap_data.as_mut().unwrap();
let Some(heap) = self.heap_data.as_mut() else {
// This will never return here because `heap_data` can only be `None` in the destructor.
return Err(ReadError::IOServerChannelFull);
};

// This check should never fail because it can only be `None` in the destructor.
Ok(ReadData::new(
Expand All @@ -824,8 +856,10 @@ impl<D: Decoder> ReadDiskStream<D> {
}

fn advance_to_next_block(&mut self) -> Result<(), ReadError<D::FatalError>> {
// This check should never fail because it can only be `None` in the destructor.
let heap = self.heap_data.as_mut().unwrap();
let Some(heap) = self.heap_data.as_mut() else {
// This will never return here because `heap_data` can only be `None` in the destructor.
return Ok(());
};

let entry = &mut heap.prefetch_buffer[self.current_block_index];

Expand Down
Loading