Review my decoder API design

code review
1

Hello, I am trying to build a QOI library as an exercise. I have a simple decoder working. I am now trying to come up with an idiomatic API for it.

pub struct QoiDecoder<R>
where
    R: Read,
{
    pub header: QoiHeader,
    pub reader: R,

    current: QoiPixel<4>,
    seen_pixels: [QoiPixel<4>; 64],
}

impl<R: Read> QoiDecoder<R> {
    pub fn new(mut reader: R) -> Result<Self, QoiError> {
        let mut header = [0; 14];
        reader.read_exact(&mut header).unwrap();
        let header = QoiHeader::try_from(header)?;

        Ok(QoiDecoder {
            header,
            reader,
            current: QoiPixel::default(),
            seen_pixels: [QoiPixel::zero(); 64],
        })
    }

    pub fn decode(mut self) -> Result<Vec<QoiPixel<4>>, QoiError> {
        todo!()
    }
    
    pub fn decode_to<W: Write>(mut self, bytes: &mut W) -> Result<(), QoiError> {
        todo!()
    }
}

I want to be able to read from an arbitrary source.
This is what I have so far and I am not sure if it's good.

A few things that came to mind:

  • Storing my input buffer in the Decoder struct, should I do it?
  • Need to support both RGB and RGBA pixels, I used const generics on my QoiPixel. Need to find out a way to decode for both pixel types.
  • The decoder stores state that is valid for one use. Therefore, the decoding functions should consume self. In that case, is there a point in having a struct?
  • QoiHeader implement TryFrom<[u8; 14]> and has an impl From<QoiHeader> for [u8; 14]. Is that a good practice?
  • Is the function naming correct?

I would be happy to receive some guidelines.
Thanks

2

The first thing I would do, would be decoupling decoding byte stream from IO. Don't store reader inside your decoder. Instead just accept byte slices (or bytes::Bytes) and return either decoded struct, decoding error, or request for more bytes (something akin to nom model).

After you got this working, you can wrap this decoder in some higher level API, that knows about Read (or AsyncRead - with this approach your decoding logic doesn't care if it is run in the sync or async context).

3

Thanks for the replay, for now, I'll assume the image data is fully loaded into memory.

pub fn new<T>(mut data: T) -> Result<Self, QoiError>
where
    T: AsRef<[u8]>,
{
    let header = QoiHeader::try_from(&data.as_ref()[0..14])?;

    Ok(QoiDecoder {
        header,
        current: QoiPixel::default(),
        seen_pixels: [QoiPixel::zero(); 64],
    })
}

I removed the Read member.
I implemented conversion between &[u8] and QoiHeader for convenience. It returns an error of the header byte size is not 14.

4

Should I store input data in my decoder?

pub struct QoiDecoder<T>
where
    T: AsRef<[u8]>,
{
    pub header: QoiHeader,
    pub data: T,

    current: QoiPixel<4>,
    seen_pixels: [QoiPixel<4>; 64],
}

impl<T> QoiDecoder<T>
where
    T: AsRef<[u8]>,
{
    pub fn new(data: T) -> Result<Self, QoiError> {
        let header = QoiHeader::try_from(&data.as_ref()[0..14])?;

        Ok(QoiDecoder {
            header,
            data,
            current: QoiPixel::default(),
            seen_pixels: [QoiPixel::zero(); 64],
        })
    }

    pub fn decode(mut self) -> Result<Vec<QoiPixel<4>>, QoiError> {
        todo!()
    }

    pub fn decode_to<W: Write>(mut self, bytes: &mut W) -> Result<(), QoiError> {
        todo!()
    }
}
5

If you decode the whole file in one call, then don't store the input data, just borrow it. If you're planning to allow getting the data in some streaming fashion (e.g. line by line), then keep the reader.

I don't quite agree with the other answer suggesting taking slices. That requires loading the whole file into memory first. It's probably not too bad for typical image file sizes, but using a reader is more elegant as it can read while decoding, so it doesn't have to keep the whole file. Slices and Vec support io::Read, so a reader is more general.

There's BufRead interface that is a compromise between slice and reader. It can give you a slice of a small buffer. You'd want to use it anyway, because for file I/O it'd be super slow to make a read call for every few pixels.

2 Likes
6

Instead of defining your own pixel struct, consider using the one in the rgb crate so that it’s compatible with other image-processing crates.

Also, you’re losing the image dimension information here: If I get 256 pixels out of the decoder, there’s no way to know whether that’s 1x256, 16x16, 8x32, etc— Consider defining a decoded image struct that has the relevant size information alongside the vector of pixel values.

Stylistically, it would be more idiomatic to drop the Qoi prefix from your type names and let downstream users refer to them like qoi::Decoder (assuming the crate/module name is qoi).

If the struct has only a default constructor and only one (consuming-self) method to call, then you’ll probably want to expose a free function to do everything in one shot. Having the struct in addition to this isn’t a problem, and may be useful later when you start adding features to your library.

For example, you might want to support incremental decoding, where the data loading is driven by outside code and bytes are provided as they become available. In that case, you’d have two methods to deal with: one to inject raw bytes and another to get back the decoded pixels.

7

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.