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

Incremental decoding implemented #26

Merged
merged 2 commits into from
Feb 27, 2019
Merged

Conversation

birktj
Copy link
Member

@birktj birktj commented Feb 1, 2019

This is an implementation of incremental decoding (decoding one strip at a time). This PR is the first step towards image-rs/image#858. I am reasonably confident that the implementation is correct in the same cases as the last implementation. I have tested it on the tests from #25 and they are correct.

The public api has changed in a small way with two new public methods on Decoder, namely Decoder::strip_count and Decoder::read_strip. If there are any objections to the api changes or the implementation I would be happy to sort them out.

@birktj
Copy link
Member Author

birktj commented Feb 8, 2019

@fintelia (or anyone else) I wonder if you could give me any feedback on this PR so that I can continue with image-rs/image#858?

@fintelia
Copy link
Contributor

fintelia commented Feb 8, 2019

I'm not really that familiar with the TIFF decoding logic, but assuming that tests pass and so forth the code looks reasonable. My one concern is that having read_strip allocate and return a Vector might cause you to have to make extra copies when implementing Read.

If you think this is ready, I can go ahead and merge this PR and we can address any rough edges in a future patch.

@birktj
Copy link
Member Author

birktj commented Feb 8, 2019

@fintelia I tried to keep it compatible with the current DecodingResult. I believe this could be done better, but that would require some larger api changes which is something I am looking into for the longer run.

I guess if I marked DecodingBuffer and read_strip_to_buffer as public I could fix the extra copy issue and simply copy some of the read_strip logic into the eventual Reader. I will fix that now and then we should then have what we need to continue with an TiffReader in the image crate for now.

Marked `DecodingBuffer` and `read_strip_to_buffer`
as public to reduce copies in eventual `TiffReader`.
@birktj
Copy link
Member Author

birktj commented Feb 26, 2019

@fintelia friendly ping. Would it be possible to merge this or do you have any other concerns.

@fintelia fintelia merged commit ff26217 into image-rs:master Feb 27, 2019
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