Seek requirement in write_to makes it impossible to stream encoded images

[Lonami]

I would like to be able ImageBuffer::write_to without requiring my writer to impl Seek.

My specific use case for this functionality is for shotgun, which can write_to(stdout()) (and does not implement Seek, as it might e.g. be piped to another process).

This is more generally applicable to other streaming workloads (such as a web-server that might want to stream data over HTTP but now can't because the output stream is append-only).

Draft

While I'm not familiar with the implementation of this crate, I suspect it should be possible for most formats to simply implement some sort of write_no_seek. The existing write_to could delegate to that method, so there would not be much code duplication. For those formats that do benefit significantly from seeking, two implementations would need to be maintained, which I understand could be more annoying. I suppose shared parts of the encoding could still be extracted into functions to be reused by both write implementations, so I don't think it would be that bad.

Much like how older versions used an intermediate buffer, I suspect the same could apply with this change. My guess is that this intermediate buffer could be smaller. Without this feature, one would need to create a buffer for the entire output, if the final stream is not seekable, which is less ideal.

If this makes sense and would be something that has a chance at being merged, I might give it a go.

Another option would be to use a feature-flag, to make the Seek requirement optional (and enabled by default) claiming it can help improve performance. But in my case it's likely that it degrades performance as now decoding (on a different program) can't start until encoding is complete.

[fintelia]

Most implementations of ImageEncoder do not require a Seek bound. So if you use that trait directly then you don't necessarily need to provide seekable writers.

There's a few file formats that do, however, need the Seek bound. TIFF in particular has a bunch of internal references that cannot be resolved until the size of various compressed chunks are known.

Does this existing API work for your use case?

[9ary]

Thanks for the pointer. This is the change I had to make when upgrading from 0.23 to 0.24: neXromancers/shotgun@4b7857a#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR238-R244

We only use the PNG and PNM encoders, so I suppose it shouldn't be too difficult to adapt to the API you suggested.

[Lonami]

Does this existing API work for your use case?

Yes, thank you, it's easy to get lost among the many traits 😅

[9ary]

Maybe closing this was a bit premature? Using ImageEncoder directly is possible, but it would be nicer/safer if there was a method that accepts an ImageBuffer instead of having to break it up manually.

[Lonami]

I feel like such addition, with my suggestions, would only unnecessarily bloat the API.

If it was possible to relax the bound of write_to somehow then yes it might be worthwhile to do so. But it's no longer as pressing, as we've gone from thinking "that's impossible" to "this could be a bit easier".

[fintelia]

Yeah, it is a definite tradeoff. Every additional method added makes it ever so slightly harder to find all the existing functionality we have

[9ary]

Yes I agree but for the record, this is what I had to do:

pub fn color_type<P, Container>(_: &image::ImageBuffer<P, Container>) -> image::ColorType
where
    P: image::PixelWithColorType,
{
    P::COLOR_TYPE
}

encoder.write_image(
    image.as_raw().as_bytes(),
    image.width(),
    image.height(),
    util::color_type(&image),
)

It's not too bad, but figuring out how to extract the raw bytes and the color type required me to dig into the source code. On the other hand, there could either be a ImageBuffer::write_with_encoder() or ImageEncoder::write_image_buffer() that does this without footguns or thinking. I don't think this use-case is too far out there at least.

The alternative of relaxing the trait bound without adding API surface could work, maybe by feature-gating it on the encoders that require Seek being enabled, but that might be even more complex to implement.