Unclear responsibilities of Info and Encoder types

[hoijui]

Hey! :-)
I would like to just add an iTXt text junk to an image, but otherwise leave it as it is, as much as possible.

There is docu for how to add the text junk when writing,
and there is docu for decoding and encoding separately.
I tried to figure out how to decode and encode again, 1:1, but I could not figure it out.
I imagine it is possible, as it is mentioned in #253,
but it is not shown there.
Would it be possible to add an example source file that decodes and encodes and changes a little thing in-between?
I'd be happy to provide the example, if I get some hints for how to do this.

[hoijui]

I am a it puzzled by the whole interface.
there is an Info struct that seems to contain (all)? the settings. It is used by Decoder and Encoder, and it can be fetched from the Decoder, but not set on the Encoder, one has to manually transfer each setting ...
does this mimic some C interface, or why is it like that?

[fintelia]

The overall interface predates my time as a maintainer and much of it likely even predates Rust 1.0. I wouldn't read too much into how the API is currently designed. At some point it would be nice to clean things up, but it has never been a priority.

I think it would be good to have an example like you describe, though as you're discovering it probably wouldn't end up looking very elegant.

[kornelski]

Indeed, the Info struct plays multiple roles, and this is not a good API.

• Info has an encode() method, but that method doesn't have access to Encoder (e.g. it can't share compressor instance for ztxt and iccp)

• Info::encode() is public, but it just writes some chunks without file header. It's an internal code in a wrong place, not useful to anyone.

• For encoding some chunks it has separate source_* fields, for some chunks it doesn't.

• It has to have a lifetime to borrow data for encoding, but doesn't need lifetimes when decoding.

I'd change Info to be purely information about decoded image, not used for encoding at all. Then provide separate options object for encoding, and helper methods for turning Info into encoding options for easy roundtripping.

Encoder also has some unnecessary complications:

• Encoder is not really an encoder, but a builder for a private encoding Options type. It takes W: Write that it doesn't use. The writer is just passed through in the final write_header(), but the W bound makes it harder for users to pass or store the encoding configuration options.

• Separation of encoding between Writer, Encoder, Info, and EncodableTextChunk means that they don't use the same zlib implementation, and zTXT and iCCP can't respect compression setting. The Writer is the actual encoder, and it should be doing all the work.

[kornelski]

@anforowicz @HeroicKatora Do you have a suggestion for a better interface for configuring animation encoding?

image-png/src/encoder.rs

Line 146 in 4b10bf8

/// FIXME: Configuring APNG might be easier (less individual errors) if we had an _adapter_ which

[anforowicz]

@anforowicz @HeroicKatora Do you have a suggestion for a better interface for configuring animation encoding?

image-png/src/encoder.rs

Line 146 in 4b10bf8

/// FIXME: Configuring APNG might be easier (less individual errors) if we had an _adapter_ which

Not really, sorry. From my perspective, the highest priority requirement is parity with the libpng-based SkPngEncoder API which doesn't support multi-frame images. So my usage of the png crate's encoding API looks more or less like this:

• Encoder:
  • Encoder::new
  • Encoder::set_color
  • Encoder::set_depth
  • Encoder::set_compression + Encoder::set_adaptive_filter based on a single enum that asks for low/medium/high compression level
  • Encoder::write_header
• Writer:
  • Writer::write_text_chunk (not sure how I feel about the latin1 handling here... nothing necessarily wrong here, but 1) IIUC fn encode_iso_8859_1_iter in text_metadata.rs will not reject ASCII control characters, 2) I am not sure if having Strings as inputs is better or worse than &[u8] - Strings require heap-allocation and going latin1 => String => back to latin1)
  • Writer::into_stream_writer
• Stream writer:
  • <StreamWriter as Write>::write
  • StreamWriter::finish