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

Fixed point in JPEG encoder #2386

Closed
awxkee opened this issue Nov 27, 2024 · 1 comment · Fixed by #2387
Closed

Fixed point in JPEG encoder #2386

awxkee opened this issue Nov 27, 2024 · 1 comment · Fixed by #2387

Comments

@awxkee
Copy link
Contributor

awxkee commented Nov 27, 2024

YUV encoding/deconding usually works well enough in fixed-point arithmetic and it is faster than floating point. Therefore, it may be possible to replace current JPEG encoding pipeline with Q16 multiplication. Notably, libjpeg-turbo also done in fixed-point arithmetic, so there will be no significant deviation from almost reference implementation.

However, switching to fixed-point is unlikely to yield significant improvements, there is only a few percent gains in end-to-end encoding.

Here is an experimental implementation to try out this approach: https://github.com/awxkee/image/tree/test_yuv_jpeg_fixed_point

cargo bench --bench fixed_point

Tests shows that fixed-point faster something about 20%, but in end-to-end encoding gain are varies from 2% to 7%.

P.S.
Anyway, it seems here .round() is missing, so luma and chroma values are truncated on each pass. Over time, this truncation leads to #2311

fn rgb_to_ycbcr<P: Pixel>(pixel: P) -> (u8, u8, u8) {
use crate::traits::Primitive;
use num_traits::cast::ToPrimitive;
let [r, g, b] = pixel.to_rgb().0;
let max: f32 = P::Subpixel::DEFAULT_MAX_VALUE.to_f32().unwrap();
let r: f32 = r.to_f32().unwrap();
let g: f32 = g.to_f32().unwrap();
let b: f32 = b.to_f32().unwrap();
// Coefficients from JPEG File Interchange Format (Version 1.02), multiplied for 255 maximum.
let y = 76.245 / max * r + 149.685 / max * g + 29.07 / max * b;
let cb = -43.0185 / max * r - 84.4815 / max * g + 127.5 / max * b + 128.;
let cr = 127.5 / max * r - 106.7685 / max * g - 20.7315 / max * b + 128.;
(y as u8, cb as u8, cr as u8)
}

@HeroicKatora
Copy link
Member

Getting 2% - 7% over the whole pipeline while improving the accuracy in the process is quite massive. Would you submit a PR with the change to the numeric conversion? Without the benchmarks probably as they won't measure active code, though it'd be nice to refer to them somehow in the evolution. (Quirky idea: maybe you could split that into two commits and then also add a revert of the benchmark addition 😂). Also please do keep the comment with the original coefficients, I think they explain the magic integer coefficients nicely.

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 a pull request may close this issue.

2 participants