Add APNG Encode Support #93
Conversation
|
@abonander Nice if you can get a look. |
|
@rrbutani |
| let ret = if self.separate_default_image { // If we've already written the default image we can write frames the normal way | ||
| // fcTL + fdAT | ||
|
|
||
| self.write_fcTL().and(self.write_fdAT(data)) |
There was a problem hiding this comment.
This kind of chaining could lead to data corruption as the write_fdAT() operation will be executed regardless of whether or not write_fcTL() succeeds. Since some I/O errors are transient, the fdAT chunk could be written without an fcTL chunk which would confuse the decoder.
These should instead be wrapped with try!() like all the other fallible operations (although the ? operator has been stable for a long time now, try!() would be consistent with the rest of the crate).
|
|
||
| #[allow(non_snake_case)] | ||
| fn write_fdAT(&mut self, data: &[u8]) -> Result<()> { | ||
| // println!("Writing fdAT:{:?}", self.info.frame_control.unwrap().sequence_number+1); |
There was a problem hiding this comment.
These printlns should be removed once the PR is ready to be merged.
|
Thanks for the feedback @abonander; I'll rebase and make the suggested changes |
|
Is this ready to be merged? (Other than the couple conflicts) |
|
There seem to be two potentially independent changes here:
As far as I can see only the former is SemVer relevant while the latter can be done in a point-release after the first has landed. I'd like to release a new version of |
It mostly works except that sequence numbers are still (somehow) wrong
The right way to do this is with a Parameter, but I'm a little pressed for time so I'll go back and do it the right way later.
|
I opened a pull request on the @rrbutani fork: rrbutani#1 I made it work but some features are missing, like the option to set a different framerate for each frame or dividing each frame image into more fdAT chunks (like with the I'm willing to continue contribuiting on this but I'd like to hear some opinions on how to make it better, as right now it doesn't feel right. |
|
@Rimpampa I think you should make your own PR so we don't need to wait on the fork's author to respond (it's been quite a while). Then also rebase those commits to resolve the merge conflict. Let me know if you'd like any help with that, I've done it once before on this one :) |
|
Closed/Moved to #255 |
Missing support for a couple of features, but seems to work.
This probably isn't ready to merge as is, but I thought I'd make the PR anyways to get some feedback.