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

Fix gif OOM when decoding #1696

Merged
merged 2 commits into from Apr 20, 2022
Merged

Fix gif OOM when decoding #1696

merged 2 commits into from Apr 20, 2022

Conversation

5225225
Copy link
Contributor

@5225225 5225225 commented Apr 11, 2022

No description provided.

@5225225 5225225 changed the title Test commit to see if CI fails Fix gif OOM when decoding Apr 11, 2022
@5225225
Copy link
Contributor Author

5225225 commented Apr 11, 2022

Hmm,, I'm getting a failure

check_references tests/reference/jpg/progressive/3.jpg.79e90ab0.png
thread 'check_references' panicked at 'The decoded image's hash does not match (expected = 79e90ab0, actual = c959cb95).', tests/reference_images.rs:279:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    check_references

test result: FAILED. 2 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 16.36s

error: test failed, to rerun pass '--test reference_images'
cargo test  63.29s user 0.27s system 261% cpu 24.314 total

Looks to be the jpeg/platform_independent feature. If I enable that locally, it passes. Which explains why CI doesn't pick it up.

Ah, looks to be an intended failure.

@5225225 5225225 force-pushed the fix-gif-oom branch 2 times, most recently from cac78c2 to 7aebc74 Compare April 17, 2022 00:57

let mut frame_buffer = vec![0; buffer_size];

self.limits.free(
Copy link
Contributor Author

@5225225 5225225 Apr 17, 2022

Choose a reason for hiding this comment

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

I think the reserve/free API is good, but I needed to free earlier than this buffer is actually freed to avoid the limits not actually being reset (in the event of a EOF when reading, I want to reset the limits back to what they were when we were given it).

Maybe an alternate API like a Limits being an Allocator wrapper would be useful. Not sure! Probably best to just get more examples of code that use the limits API and then see what fits them the best.

Copy link
Member

Choose a reason for hiding this comment

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

It's probably correct, an allocator API would be interesting but it would need to interact with all containers. Vec and Box is potentially a good start but not generic enough. But it's hard to say if it was better to wait on an the allocator interface or to solve it separately.

Given that there is no concurrency in this specific, I would say the order doesn't matter to here. The only effect of free is to allow following reserve calls to succeed.

@5225225 5225225 marked this pull request as ready for review April 17, 2022 21:33
src/codecs/gif.rs Outdated Show resolved Hide resolved
@HeroicKatora HeroicKatora merged commit f2855f1 into image-rs:master Apr 20, 2022
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