Skip to content

Splat adam7 interlacing #576

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

Closed

Conversation

thirumurugan-git
Copy link
Contributor

As a follow-up to this issue in Chromium, I created a corresponding PR in image-png.

CC: @anforowicz

@thirumurugan-git
Copy link
Contributor Author

@anforowicz Please review this PR and share your suggestions on the TODOs.

Copy link
Contributor

@anforowicz anforowicz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@kornelski
Copy link
Contributor

I think filling the image pixels completely is the expected behavior, so it would be nice to make this easy/default, but I'm not sure how such thing could be integrated with the library API.

Doing this in expand_interlaced_row would needlessly overdraw pixels when decoding multiple passes at once. So even though standalone function doesn't feel like the most elegant solution to me, I don't have a suggestion for anything better.

@anforowicz
Copy link
Contributor

@kornelski and/or @fintelia - can you take another look please?

info: &Adam7Info,
bits_pp: usize,
bit_pos: usize,
frame_width_in_bits: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm suspicious that this might overflow on 32-bit systems. The problem isn't so much that there's likely to be many valid images more than 500 million pixels wide, but rather that fuzzers are really good at figuring out how to populate header fields so that the code reaches the overflow before noticing that the rest of the file is corrupt.

(Looking now, I think the existing expand_adam7_bits code might have the same issue)

src/adam7.rs Outdated
bytes_pp: usize,
start_byte: usize,
) {
let height = (img.len() - start_byte + stride - 1) / stride;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can use the div_ceil method

Comment on lines +12 to +15
line_mul: usize,
line_off: usize,
samp_mul: usize,
samp_off: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

Rust convention is to mostly avoid abbreviations in variable names. So for instance, sample_offset rather than samp_off

6 => ((w - 1.0) / 2.0, h / 2.0),
7 => (w, (h - 1.0) / 2.0),
_ => unreachable!(),
let (line_mul, line_off, samp_mul, samp_off) = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this whole function can be simplified a bunch. If I'm not missing something, should be a matter of...

let pass = PASS_CONSTANTS[self.current_pass as usize - 1];

self.line_width = (self.width - pass.samp_off).div_ceil(pass.samp_mul);
self.lines = (self.width - pass.line_off).div_ceil(pass.line_mul);
self.line = 0;

(And making the constants u32 rather than usize)

@thirumurugan-git
Copy link
Contributor Author

@fintelia I've made the necessary changes for the splat expand pass. However, updating the parameter types as discussed, like changing u32 for image width/height and using u64 for byte handling, would require refactoring the existing codebase, including tests. Since that would introduce significant changes beyond the scope of this feature, I’d prefer not to include that refactoring here.

Would it make sense to create a separate issue to track the type changes and have further discussions around using u32::div_ceil or u64::div_ceil there? Let me know your thoughts.

@fintelia
Copy link
Contributor

I'm OK with deferring changes to other parts of the codebase, but I do want to make sure that this PR doesn't introduce new integer overflow bugs. At a minimum, we absolutely cannot do img.len() * 8 which will overflow even on an RGB8 16K x 16K image (on 32-bit platforms).

@thirumurugan-git
Copy link
Contributor Author

@fintelia tested this(multibyte_expand_pass_test_helper(1, 256000000, 32)) code and confirmed that a buffer overflow does occur. However, the overflow originates in the existing code(let line_start = prog_line * row_stride_in_bytes * 8; method: expand_adam7_bits) before it reaches my changes. Would it be better to address the existing overflow in a separate PR and then proceed with fixing and merging this PR? Or would it be preferable to fix both overflows in the current PR and merge it together? What are your thoughts?

@fintelia
Copy link
Contributor

fintelia commented May 30, 2025

Small PRs are much preferred! Please go ahead with a separate PR

@197g 197g self-requested a review June 4, 2025 21:19
@197g 197g self-assigned this Jun 4, 2025
@197g
Copy link
Member

197g commented Jun 26, 2025

Due to fixing of #599 and the main rework of the adam7 code, I'll be picking this up and rebasing the core to a very similar structure. @thirumurugan-git Thank you for the brunt of the algorithmic details, test case etc. Should make this a breeze.

Edit: A replacement PR will probably work best given that most of the commits here made it through smaller commit series into the main branch already.

@thirumurugan-git
Copy link
Contributor Author

@197g Thank you for taking up my PR. I’ve been caught up with my own schedule, so I haven’t had the chance to review the changes yet. I’ll go through them as soon as I get some time. Also, is there any Discord or Telegram community for image-rs that I can join?

@197g
Copy link
Member

197g commented Jun 28, 2025

@thirumurugan-git There's a matrix space: #image-rs_image:gitter.im

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants