image-rs / image-gif

GIF en- and decoder
Apache License 2.0
150 stars 40 forks source link

Implement Iterator on Decoder #134

Closed tobiasvl closed 10 months ago

tobiasvl commented 2 years ago

I'm a Rust newbie, so I might be thinking weirdly here, but the current structure where Decoder is a struct that holds global information about the GIF (color and palette) and also state information (current frame) seems very strange to me.

Wouldn't it be more idiomatic if there was a struct with the global information, which you could then get an iterator from that would iterate over the frames?

I was just trying to use the global palette as a fallback for frames that lack frame palettes:

while let Ok(Some(frame)) = decoder.read_next_frame() { // mutable borrow
    let palette = frame
        .palette
        .as_ref()
        .and_then(|p| Some(&p[..])) // frame.palette is Vec<u8>, global_palette is &[u8]
        .unwrap_or(decoder.global_palette().ok_or(Error::PaletteError)?); // immutable borrow
    // ...
}

which obviously doesn't work since decoder is borrowed mutably for the loop. There might be better ways to do this (although cloning the global palette before the loop doesn't feel like it's better), but it just feels strange to me that iterating over frames need a mutable borrow of the entire Decoder, and it feels like this should be an Iterator.

HeroicKatora commented 2 years ago

Items yielded by an iterator must be independent from the iterator itself and can't borrow from the decoder. Hence we would have to clone or reference count all shared information if it were to implement Iterator. It's not possible without some form of other overhead.