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.

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

Improve encode_gray performance in BmpEncoder #2193

Merged
merged 3 commits into from Apr 8, 2024

Conversation

mattatz
Copy link
Contributor

@mattatz mattatz commented Apr 5, 2024

Hi image-rs team,

This Pull Request focuses on enhancing the performance of the encode_gray function within BmpEncoder.
It optimizes the buffer writing process, thereby improving the overall execution speed of the function.

This modification has significantly improved the encoding speed for large-resolution BMP files.

@fintelia
Copy link
Contributor

fintelia commented Apr 7, 2024

Could you share any benchmarks results you've measured from these changes?

@mattatz
Copy link
Contributor Author

mattatz commented Apr 8, 2024

The former is a benchmark for encoding BMP files before the change, and the latter is the benchmark after the change.
Significant improvements can be seen in the cases affected by the changes, namely zero-L8-file and zero-L8-rawvec.

before
after

// alpha is never written as it's not widely supported

// color value is equal to the palette index
if bytes_per_pixel == 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make this x_stride == 1?

}
// alpha is never written as it's not widely supported
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move this comment into the else branch?

// improve performance by writing the whole row at once
let row_end = row_start + y_stride;
self.writer
.write_all(&image[row_start as usize..row_end as usize])?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly comes down to style, but I think this would be slightly nicer as .write_all(&image[row_start as usize..][..y_stride as usize]

@mattatz
Copy link
Contributor Author

mattatz commented Apr 8, 2024

@fintelia
Thank you for the review 👍
I have incorporated all the feedback.

@fintelia fintelia merged commit 1c95b86 into image-rs:main Apr 8, 2024
31 checks passed
@fintelia
Copy link
Contributor

fintelia commented Apr 8, 2024

Thanks!

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.

None yet

2 participants