Skip to content

Conversation

mcroomp
Copy link
Collaborator

@mcroomp mcroomp commented Oct 10, 2025

Add benchmark for the critical encoding and decoding of coefficients.

Best practice is to keep test code in a separate module so that all the imports are isolated.

Moved rstest to a test dependency

Moved to single implementation of read_file for testing

@mcroomp mcroomp requested a review from Copilot October 10, 2025 17:11
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds micro-benchmarks for JPEG coefficient encoding/decoding and improves test code organization by centralizing test utilities and separating test dependencies.

  • Adds new micro-benchmarks for block-level JPEG encoding and decoding operations
  • Consolidates the read_file function into a single implementation in lib.rs
  • Moves rstest from a runtime dependency to a test-only dependency

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/end_to_end.rs Removes duplicate read_file function and uses centralized version
lib/src/structs/lepton_file_writer.rs Moves test code into proper test module and uses centralized utilities
lib/src/structs/lepton_file_reader.rs Removes duplicate read_file function and reorganizes test code
lib/src/micro_benchmark.rs Adds exports for new block-level benchmarks
lib/src/lib.rs Adds centralized read_file function for testing/benchmarking
lib/src/jpeg/jpeg_write.rs Reorganizes test code and adds new block encoding benchmark
lib/src/jpeg/jpeg_read.rs Reorganizes test code and adds new block decoding benchmark
lib/src/jpeg/jpeg_header.rs Expands visibility of utility functions for benchmarking
lib/src/jpeg/bit_writer.rs Adds clear method and reorganizes test code
lib/src/jpeg/bit_reader.rs Reorganizes test code into proper module
dll/src/lib.rs Moves test code into proper module and uses centralized utilities
dll/Cargo.toml Moves rstest to dev-dependencies
benches/benches.rs Uses centralized read_file function and adds new benchmarks

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@mcroomp mcroomp requested a review from Copilot October 11, 2025 06:52
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +64 to +73
let mut data_buffer = Vec::new();
if let Err(e) = data_buffer.try_reserve_exact(encoded_length) {
return err_exit_code(
ExitCode::OutOfMemory,
format!(
"unable to allocate {} bytes for jpeg output buffer: {:?}",
encoded_length, e
),
);
}
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

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

Creating an empty Vec and then trying to reserve memory is less efficient than directly creating a Vec with capacity. Consider using Vec::with_capacity(encoded_length) or handle the allocation error at creation time.

Suggested change
let mut data_buffer = Vec::new();
if let Err(e) = data_buffer.try_reserve_exact(encoded_length) {
return err_exit_code(
ExitCode::OutOfMemory,
format!(
"unable to allocate {} bytes for jpeg output buffer: {:?}",
encoded_length, e
),
);
}
let mut data_buffer = Vec::with_capacity(encoded_length);

Copilot uses AI. Check for mistakes.

Comment on lines 18 to +19
impl BitWriter {
pub fn new(capacity: usize) -> Self {
pub fn new(data_buffer: Vec<u8>) -> Self {
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

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

The constructor now takes ownership of a Vec but doesn't validate or document expectations about the buffer state (e.g., should it be empty, can it contain existing data). Consider documenting the expected buffer state or adding validation.

Copilot uses AI. Check for mistakes.

let actree = HuffTree::construct_hufftree(&actbl, false).unwrap();

Box::new(move || {
use std::hint::black_box;
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

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

[nitpick] The black_box import is only used in benchmark code but is imported at the top of a function scope. Consider moving this import closer to its usage or document why it's needed for the benchmark.

Copilot uses AI. Check for mistakes.

@mcroomp mcroomp merged commit 110d408 into main Oct 21, 2025
5 checks passed
@mcroomp mcroomp deleted the jpegmicro branch October 21, 2025 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants