-
Notifications
You must be signed in to change notification settings - Fork 142
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
APNG Encode #255
APNG Encode #255
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few concerns that need not be blockers except for write_separate_default_image
. I'd rather see it land at some point than delay it further. However, if you agree with the problems/improvements then it would be good to see them addressed.
src/encoder.rs
Outdated
match self.info { | ||
Info { | ||
animation_control: Some(_), | ||
frame_control: | ||
Some(FrameControl { | ||
sequence_number, .. | ||
}), | ||
.. | ||
} => { | ||
self.check_animation()?; | ||
self.write_fcTL()?; | ||
if sequence_number == 0 && !self.separate_default_image { | ||
self.write_chunk(chunk::IDAT, &chunk)?; | ||
} else { | ||
self.write_fdAT(&chunk)?; | ||
} | ||
self.info.animation_control.as_mut().unwrap().num_frames -= 1; | ||
} | ||
_ => self.write_chunk(chunk::IDAT, &chunk)?, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly like this pattern for a few reasons. Firstly the matching should be outside the loop so as to not suggest that the path could change while writing the image.
Secondly, changing animation_control
is a clever way to avoid another tracking variable but it's not expected. What if we were to add a getter? Then the info could be incorrect after you've written a few chunks.
And lastly, there should be a note on what happens if we write more images than the animation_control
allows. The connection between check_animation
avoiding the panic from wrapping in the subtraction is rather subtle. I expect we could have a lenient mode at some point that allows this so the control flow should be more obvious. If we don't subtract then there is also no problem.
src/encoder.rs
Outdated
@@ -234,7 +290,33 @@ impl<W: Write> Writer<W> { | |||
/// Writes the image data. | |||
pub fn write_image_data(&mut self, data: &[u8]) -> Result<()> { | |||
const MAX_CHUNK_LEN: u32 = (1u32 << 31) - 1; | |||
let zlib_encoded = self.deflate_image_data(data)?; | |||
for chunk in zlib_encoded.chunks(MAX_CHUNK_LEN as usize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MAX_CHUNK_LEN
is too much if we're writing fdAT
data since there are 4 bytes of sequence number added. This can also be addressed by matching first, and then looping.
src/encoder.rs
Outdated
/// Sets the default frame rate of the animation | ||
/// | ||
/// It will fail if `set_animation_info` isn't called before | ||
pub fn set_frame_rate(&mut self, delay_num: u16, delay_den: u16) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub fn set_frame_rate(&mut self, delay_num: u16, delay_den: u16) -> Result<()> { | |
pub fn set_frame_delay(&mut self, delay_num: u16, delay_den: u16) -> Result<()> { |
It should also make it more obvious that this only affects future frames.
src/encoder.rs
Outdated
/// If `num_plays` is set to 0 it will repeat infinitely | ||
/// | ||
/// It will fail if `frames` is 0 | ||
pub fn set_animation_info(&mut self, frames: u32, num_plays: u32) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter should just be an AnimationControl
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also what happens if you call this after having written the first frame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get the second comment, how could you call it after having written a frame first? Once you call write_header
you can't access the Encoder
anymore
src/encoder.rs
Outdated
@@ -305,6 +384,93 @@ impl<W: Write> Writer<W> { | |||
pub fn into_stream_writer_with_size(self, size: usize) -> StreamWriter<'static, W> { | |||
StreamWriter::new(ChunkOutput::Owned(self), size) | |||
} | |||
|
|||
pub fn write_separate_default_image(&mut self, data: &[u8]) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems odd. When transitioning something to animated PNG I would expect the opposite, that I need one or two more calls to setup the animation. I would however not expect that the still image, as which this chunk is interpreted when it is written, needs a separate methods. Consider two scenarios:
- The image is generated by a separate method whose parameter is
Writer<W>
. - An image is modified in a pipeline.
In both cases it's complicated that the reader has one method for all images but the write_image
is only for animated images if the image is animated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(You meant write_separate_default_image
at the end right?)
This could be solved by moving the method to the Encoder
creation, with like a set_separate_default_image(data: &[u8])
that will panic/error if the animation parameters are not set (or sets the default parameters for an animation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way would be to have a mandatory additional step where the FrameControl
data needs to be initialized, separately from the call that initalized the animation control. Then treat every call to write_image_data
as writing the default image as long as no frame control was set. That's very close to the way the chunks are organized in the image and avoids complicating the constructor.
src/encoder.rs
Outdated
@@ -276,7 +355,7 @@ impl<W: Write> Writer<W> { | |||
/// This borrows the writer. This preserves it which allows manually | |||
/// appending additional chunks after the image data has been written | |||
pub fn stream_writer(&mut self) -> StreamWriter<W> { | |||
self.stream_writer_with_size(DEFAULT_BUFFER_LENGTH) | |||
self.stream_writer_with_size(Self::DEFAULT_BUFFER_LENGTH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about stream writer, actually? Should it error or return a StreamWriter
for an animated image chunk? Not that it's critical because currently it simply allows one to write multiple IDAT
images anyways. 😅
src/encoder.rs
Outdated
/// Sets the default frame rate of the animation | ||
/// | ||
/// It will fail if `set_animation_info` isn't called before | ||
pub fn set_frame_rate(&mut self, delay_num: u16, delay_den: u16) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, why would it be possible to set this aspect specifically but not other parts of FrameControl
? Special casing frame rate seems appropriate for a format-agnostic interface but not for a mid-level interface as png
has.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some check-boxes to the first comment, I forgot about those when I wrote that
So I've thought a bit on your comments @HeroicKatora and this is what I came up to: About single images Currently the Having said that I think we can get rid of the Note: Right now it is possible to write additional image data as long as it fills the image completely (it can be used to hide an If backwards compatibility is required, Regarding animations I was thinking about making a new struct that would implement the way I intended it to work is that it would return an The way I imagined all of this to work is like that: // For single images
{
let mut encoder = Encoder::new(100, 100);
// set encorder options
let mut writer = encoder.write_header().unwrap();
writer.write_image_data(&data).unwrap(); // First part
// ...
writer.write_image_data(&data).unwrap(); // Last part
writer.finish().unwrap(); // Because it can silently fail while dropping
}
// For animations
{
let mut encoder = AnimationEncoder::new(100, 100, 20); // 20 frames
// set encoder options
let mut frames = encorder.write_header(&default_image).unwrap(); // or write_animation_header() if only Encoder is used
for frame_enc in frames {
// set frame options
let writer = frame_enc.write_header(); // or something on that line
writer.write_image_data(&data).unwrap(); // First part
// ...
writer.write_image_data(&data).unwrap(); // Last part
writer.finish().unwrap(); // Because it can silently fail while dropping
}
} Like that it might be better to call the two types of writers Obviously it's still very generic and some more thinking should be done in order to avoid code duplication, I'm not an expert so I don't expect this to be completely right. While I'm here, I've seen that the If we put struct IDATChunk;
impl ChunkType for IDATChunk {
const TYPE: [u8;4] = *b"IDAT";
}
// The above part wouldn't change much the current state/usage
impl IDATChunk { // or maybe impl io::Write
pub fn write<W: io::Write>(
writer: &mut W,
filter: FilterType,
bpp: BytesPerPixel,
curr: &[u8],
prev: &[u8],
) {
// ...
}
} Then we could avoid code duplication between the types I mentioned above ( That's it, I should give it another thought but I want to make progress and can't spend much time on this during the week so I'm going to throw it in and read your opinions |
I'm sorry that it took a while to answer but I wanted to be thorough. However, now this PR becomes more complicated with each commit, at least in my perception,—the chunk encoding logic is duplicated, and so is the
It isn't. Currently the plan is for 0.17 to have all breaking changes necessary to improve the API for full APNG support while keeping the rough structure we currently have. And the 0.16 interface is more or less finished, the master changes go into 0.17.
The
Point well taken. I do agree that the exposed
Another good idea for another PR :) I'm much more critical of merging the two types however. The image line state is necessary only for writing image data and the chunk length restriction is on the length of compressed data which we can't necessarily predict from the raw sample buffers alone, as it also depends on the prediction functions etc. Using less buffer copies could work though.
I don't like the idea of additional owning types, it's already too many. It really complicates many designs downstream to have two incompatible types. An example for this is the - for frame_enc in frames {
+ while let Some(frame_enc) = frames.next_animated() { …where // set encoder options
let mut encoder = Encoder::new(100, 100)
.animated(20); // Or don't call it, for static PNG.
let mut frames = encorder.write_header()?;
// This will never be part of the animation.
// If you don't call it on animated PNG, the first animated frame will write IDAT chunks.
frames.write_image_data(&default_image)?;
// Or don't loop, for static PNG.
while let Some(frame_enc) = frames.next_animated() {
// set frame options
let writer = frame_enc.write_header(); // or something on that line
writer.write_image_data(&data)?; // First part
// ...
writer.write_image_data(&data)?; // Last part
writer.finish()?; // Because it can silently fail while dropping
} If you don't loop then you've naturally written a non-animated PNG. If you don't call the default |
I agree with you that this is starting to look over engineered, I was going to write a little review of my code addressing the code duplication problem but you preceded me. Regarding the merging of the I'd like to have more details on the Also could you take a look at I'm going to start working on merging the writers/encoders, thank you for the useful advice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have more details on the Stream::pause and get_raw_writer part: as I understood you want this PartialChunkData to permit access to the internal writer so that raw data could be written but should the writer flush before or the internal state should remain untouched?
The current StreamWriter
and ChunkWriter
have two usage problems. Firstly, all data of a chunk must be available upfront, in a single contiguous slice. That often requires an allocation—have a look at the many vec![]
calls in chunk writing implementations.
Secondly, they contain logic when dropped, and no way to avoid that code and continue later. Since they borrow the Encoder
this can run into very problematic borrow issues where dropping would be a logic but, but the borrow must be (temporarily) ended. That can only be solved by leaking the Writer
so that its Drop
is never called. That's certainly not optimal and loses some state. The idea of ::pause
would be to allow that in a controlled manner where the data is retained and can be continued/borrowed again. As far as state is concerned, there are already plenty of ways to directly write to the stream. The correctness of the produced png is hardly an invariant we do —and need to— uphold if you corrupt your own output stream. I wouldn't see the problem in merely advising to be careful.
Got inline with the latest commit on master and merged the Writers/Encoders, now it works like your example but with some differences: For encoding an image let mut enc = Encoder::new(file, width, heigth);
// enc.set...
let mut ctrl = enc.write_header().unwrap();
let mut wrt = ctrl.write_frame_header().unwrap();
wrt.write_image_data(&data).unwrap();
// ... Nth write
wrt.finish().unwrap();
ctrl.finish().unwrap(); For encoding an animation let mut enc = Encoder::new(file, width, heigth);
// enc.set...
enc.animated(frames, repeat).unwrap();
let mut ctrl = enc.write_header().unwrap();
while ctrl.remaining() > 0 {
// ctrl.set...
let mut wrt = ctrl.write_frame_header().unwrap();
wrt.write_image_data(&data).unwrap();
// ... Nth write
wrt.finish().unwrap();
}
ctrl.finish().unwrap(); For encoding with a separate default image let mut enc = Encoder::new(file, width, heigth);
// enc.set...
enc.animated(frames, repeat).unwrap();
enc.with_separate_default_image().unwrap();
let mut ctrl = enc.write_header().unwrap();
// this could go in the while loop if the data is already lined up
{
let mut def_img_wrt = ctrl.write_frame_header().unwrap();
def_img_wrt.write_image_data(&def_img_data).unwrap();
def_img_wrt.finish().unwrap();
}
while ctrl.remaining() > 0 {
// ctrl.set...
let mut wrt = ctrl.write_frame_header().unwrap();
wrt.write_image_data(&data).unwrap();
// ... Nth write
wrt.finish().unwrap();
}
ctrl.finish().unwrap(); Regarding the Right now I'm using a |
A note: You're still doing many, many superfluous edits left-and-right (changing |
Sorry for making your job more difficult, it's my first time contributing to an open source project and I have still a lot to learn. I'm going to remove anything that hasn't to do with APNG encoding (like the time chunk that I forgot to remove after trying things out with the "chunk new" file). Also I thought that using Self instead of the explicit name was a good practice (not referring to the errors on the test, but to return type and constructors). Though, with my comment I was more interested on the |
The main challenge right now is that the pull request adds/modifies over 1300 lines. The general advice is to break pull requests down as much as possible since even when changes might be an improvement on their own (like improving formatting/naming/consistency) they can become overwhelming when trying to make sure a complex PR is correct. |
Welcome then 🙂 On teaching how to contribute to open source, there have already been two good demonstration on why to keep PRs small and self-contained. The first being the error rework which you had to adjust for. Chances are, other changes are in progress in parallel to your changes and every line or idea is a potential point of conflict. Minimizing those means quicker progress. |
Did some cleaning of changes that were not needed, hopefully this makes your work easier. Let me know what to do for the |
Please also remove the Zlib stuff, especially since you don't know how/if it works out. I've opened #267 as a dedicated tracking issue as it is currently merely a draft. |
It works, when I said I didn't understand the compression algorithm I wasn't referring to the usage of the library but to the actual implementation because I don't know if exposing the inner writer invalidates the compression stream. The I'm not sure if you noticed but Having said that, if you think this should not be done, taking those changes back it's easy |
Compared to ¹not 100% ensuring if you have a shared writer but very much encourages it. |
The need of a double lifetime comes from the fact that currently the It would look like this: pub struct Writer<W: Write> {
// ...
ctrl: Option<Controller>,
}
impl<W: Write> Writer<W> {
// ...
pub fn finish(&mut self) -> Result<Controller> { /* ... */ }
} |
It does not reflect my understanding of what needs to own the stream. I'm trying to pick out your conceptual changes of ownership but there are quite a lot and some unclear to me. The current concepts are:
Can you make a similarly clear differentiation for each struct in the PR and justify the difference instead of purely an addition? If not, then don't change them. As far as I can follow the changes:
Please focus on the last point in particular since that is the source for your two-lifetime borrows. There's a substantial amount of code expended to that complexity: The |
Ok so, my idea was to remove the For the understanding of the current mechanism, all the structs have the same functions as before, what is new is the On the I'm currently taking back the |
I completely forgot to post a comment, sorry for taking so long. I think it's ready for a review, I did a test yesterday and the encoding of animations was working |
@HeroicKatora I was wondering is there any progress going on? I don't want to sound annoying, just asking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That still seems to complicated. I had a question that was not yet answered:
Can you make a similarly clear differentiation [in purpose] for each struct in the PR and justify the difference instead of purely an addition [of methods to existing structs]?
On style itself, this article is a good primer on writing code that will be maintained for probably a good while.
For proceeding I would encourage you to split out the various encode
methods into a separate PR—or move those changes to the front of the commit series by rebasing. They are unquestionably useful in their implementation and rather straightforward. Then start from a blank canvas and unchanged structs, and try to get the same APNG working with minimal interface work. A system is clean not when there is nothing more to add, but when there is nothing more to take away.
I'm sorry for the delay between the implementation and that feedback—took the holidays for something completely different—, but I'm convinced it will be worth it to work on the interface.
src/common.rs
Outdated
if let Some(t) = &self.trns { | ||
chunk::encode_chunk(w, chunk::tRNS, t)?; | ||
} | ||
self.time.map_or(Ok(()), |v| v.encode(w))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map_or
is clever but not nice to read. There's reason for the surrounding code to not use it, and for clippy to lint against similar patterns.
pub struct Controller<W: Write> { | ||
w: Option<W>, | ||
data: ChunkData, | ||
// here the FrameControl is used for costomized data | ||
info: Info, | ||
total: u32, | ||
written: u32, | ||
filter: FilterType, | ||
adaptive: AdaptiveFilterType, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writer
was a better name for this. The library writes png
, the container format, not only PNG pixmaps. I'd much rather see the specialized structs named ImageWriter
, FrameWriter
or something of that sort.
adaptive: AdaptiveFilterType, | ||
} | ||
|
||
impl<W: Write> Controller<W> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was write_chunk
removed from this struct and is no only available when we can write a frame? That is a clear regression in functionality.
self.total - self.written | ||
} | ||
|
||
pub fn next_image(mut self) -> Result<Writer<W>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving makes for an awkward design (see the matching on Option<W>
later) because Rust's type system is linear. The concept of type states was rejected very early in the language design and for good reason. It requires every consumer to effectively implement the state machine in their own types as enum variants or something.
The idea of enforcing order through the type system might be 'clever' but is hardly usable in practice, it should be an implementation detail and there is no shame in enforcing it through dynamic checks. Unless some data structure invariant depends on it, but there is no such requirement. Since the library can not write all valid PNGs there must be an escape hatch, even if that enables the user to write 'invalid' PNGs—that might even be a feature.
if let Some(c) = &self.info.source_chromaticities { | ||
let enc = Self::chromaticities_to_be_bytes(&c); | ||
write_chunk(&mut self.w, chunk::cHRM, &enc)?; | ||
pub fn finish(&mut self) -> Result<W> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, on the other hand, should take self
by value. The specification clearly states that it is required and there are already escape hatches for both variants of not adhering to it: Writing no end can be done by forgetting, writing an additional end by manually calling write_chunk
. Instead, this method should have an ergonomic interface where the common usage has the right effect.
You might simply hide the implementation taking &mut _
in a private method.
self.write_chunk(chunk::IDAT, &chunk)?; | ||
} | ||
|
||
pub fn finish(&mut self) -> Result<Controller<W>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before, take by value in the public interface.
self.write_chunk(chunk::IDAT, &chunk)?; | ||
} | ||
|
||
pub fn finish(&mut self) -> Result<Controller<W>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On another point against returning the Controller
by value, an error will simply drop the controller including the stream and associated data. You'd have to move the controller into the error but please don't do this. If you instead borrowed from it then this can be avoided entirely.
.unwrap() | ||
.next_image() | ||
.unwrap() | ||
.write_image_data(&buf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it pretty clear that the changes are an ergonomic loss. A method call without parameters, between two unwraps/errors is not what you want the common, simple usage to look like.
self.source_gamma.map_or(Ok(()), |v| v.encode(w))?; | ||
self.source_chromaticities.map_or(Ok(()), |v| v.encode(w))?; | ||
} | ||
self.animation_control.map_or(Ok(()), |v| v.encode(w))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map_or
is clever, it is not particularly readable and not necessarly. clippy
will tell you to use if let
instead. I'm puzzled why that was changed in the first place.
filter: <_>::default(), | ||
adaptive: <_>::default(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type annotations make this a lot more readable, if one wants to understand what is happening. Another unecessary clever change. You seem to want to try out stuff but changing working, existing code is not the proper outlet for this.
I understand all the points on the cleverness of the code I wrote, and please don't get me wrong I didn't wrote that because I don't care about those things, it's just how I'm used to. The article you linked was helpful and I've been encountering many of those aspects since when I started working as programmer because the codebase I'm working on is really full of those clever lines and I have to go back and forth every time to understand what is going on and talking with my collegues started to make me think more on this aspect of writing code. I will open the pull requests for the encode methods as you suggested as soon as I can. Should I implement them in the Regarding the other questions I feel like many of them have already been answered in my previous comment. I think there has been a misunderstanding in that situation as the reaction to the comment and the fact that no other comment was submitted in the following day made me think that you were ok with what I wrote and that's why I started working on it. Regardless, I'm going to explain it again: First of all the only two addition are the I feel like the The
+1 that will be added, as you suggested: write chunks in-between frames directly (through Here comes in the new The Now, the On the note about not being scared of making dynamic checks in order to favour simplicity I'll try to come up with an implementation that uses only Let me know what you think of everything, also if you can give me other tips or anything it will be highly appreciated. |
This is pretty much what I had meant for and described the current
On owning vs. non-owning I'm very confused by your statement that an Option is required—mostly for that fact that you have kept On state machine: Rust's type system makes it so that you can use parameters zero or one time but you can not arbitrary copy them (this propery is called affine). When writing methods that take their parameter by value your code places a restriction on the caller: They can not use that object again, and they must have owned it. So, the methods taking
By doing this, any caller that wants to call All the Now you, proposing that dropping the Controller in an error case is fine, ask:
However, consider that most errors do not signal a corrupted PNG stream. Except for IO errors, most are checked before any write is performed. Your own added checks for APNG are of that sort. Mostly, all produced chunks will be intact and complete. Recovery would be possibly, in |
You did not understand my statement on owning/borrowing because you were not considering the BTW I when I wrote "And yes the public interface can easily be modified so that it consumes the Writer" I meant that the Right now the main thing I'm not sure of is how to handle animations on the
Related to the last point is why the What is confusing me here is the line between the need of checking the correctness of what it's going to be written and blaming who is using the library for using it wrong. If you could answer those questions and the other ones I asked on the previous comment about what to do with the encode methods I could start working on it tomorrow. |
One PR for all the encoding methods would be fine. The impls can live where each struct is defined, but that's not a strong preference. I'll get back to the rest of the response later. |
Side note on terminology: We're both discussing current semantics and wanted semantics. For obvious reason using the type names of this PR is confusing for those cases. Try to avoid renames such that semantics collide with current structs of the same name in the rework. Or we discuss the renames as a dedicated preceeding PR. If you want to do this, see: renaming in
Then don't. Have each just write at most a single frame, just like it is now. The main functionality of not requiring a full allocation is still there even when one needs to call the
It should be clear that changing a parameter has effect on its next usage, not immediately or retroactively. That is, there is little reason to stop it from being changed or to require it to be only changed once. So I would add setters of frame parameters to
An interesting little point here regarding Writer vs. StreamWriter: It's not necessary to have them as separate types. In fact, their methods may be merged. All that is necessary is to make the methods that write full frames return an error if any partial write through the stream methods has been performed. The only 'risk' involved for the caller with StreamWriter is that they might forget writing part of the frame, which corrupts the PNG. But neither the merge nor the split types addresses this risk differently with regards to their I briefly touched on this above. I don't think it is necessary to enforce total correctness of the PNG, except where we can make a specific guarantee and the caller explicitly requests us to do so. Accidentally making something impossibleis worse than incorrectness on wrong usage. That's also the reason why the The |
This is taking very long, these weeks have been quite busy for me but I've got some work done, this weekend I will finish it. If that's okay with you I'll open a new pull request where I start again from the current master (as you suggested) in order to clean up the messy commit history this one has. |
After another week I did it even if there was no answer, I opened a new PR and I'm closing this one |
As @HeroicKatora suggested, I made a PR directly here, this is supposed to continue the work done on image-png#99.
As I said over there, this PR takes the existing code and adds APNG encoding functionality, maintaining backwards compatibility, but is not supposed to be merged in it's current state. I'd like to hear suggestion from others on how to make it better.
APNG related features: