get_picture() fails with Error::Again with non 1 threads in settings

[neumond]

Doesn't matter if I build 1.0.0 dav1d or the latest commit.
With default settings, send_data same image several times helps to finally get some result in get_picture.
If I set threads to 1, sending several times fails, and get_picture works after 1st send_data.
Looks like a serious issue with multithreaded balancer.
Code is quite trivial.

use avif_parse::read_avif;

pub fn load_avif(fname: &str) -> (Vec<u8>, u32, u32) {
    let context = read_avif(&mut File::open(fname).unwrap()).unwrap();
    let mut dsettings = dav1d::Settings::new();
    dsettings.set_n_threads(1);  // any value except 1 breaks the program
    let mut decoder = dav1d::Decoder::with_settings(&dsettings).unwrap();
    decoder.send_data(
        context.primary_item,
        None, None, None,
    ).unwrap();
    let picture = decoder.get_picture().unwrap();
    println!("Width {}", picture.width());
    println!("Height {}", picture.height());
    // TODO
    (Vec::new(), 0, 0)
}

[sdroege]

That's expected and intended behaviour.

If there are multiple threads then frames are decoded in parallel and you have to either wait or provide more frames first. Also once you're done decoding your stream, flush the decoder and then you'll get all pending frames immediately.

You can set the maximum number of frames it decodes in parallel via set_max_frame_delay(). Multi-threading is split into frame-parallel and slice-parallel decoding, so even with a max frame delay of 1 you will have some degree of parallelism.

[neumond]

Flush doesn't help.

    // 2 threads
    decoder.send_data(
        context.primary_item,
        None, None, None,
    ).unwrap();
    decoder.flush();
    let picture = decoder.get_picture().unwrap();  // Error::Again

[sdroege]

That seems like something to report to https://code.videolan.org/videolan/dav1d but they're going to ask for a C example to reproduce it. Theoretically the above code should work correctly.

[neumond]

Looks like api requires retrying on each Error::Again until I get a picture. Flush seems to destroy all unread pictures.

    decoder.send_data(
        context.primary_item,
        None, None, None,
    ).unwrap();
    let picture = loop {
        break match decoder.get_picture() {
            Ok(p) => p,
            Err(dav1d::Error::Again) => continue,
            e @ Err(_) => e.unwrap(),
        };
    };

My test C program:

#include <stdio.h>
#include <dav1d.h>


long get_file_size(FILE *f) {
    int err = fseek(f, 0, SEEK_END);
    if (err != 0) {
        printf("Error in fseek 1\n");
        return -1L;
    }
    long size = ftell(f);
    if (size < 0) {
        printf("Error in ftell\n");
        return size;
    }
    err = fseek(f, 0, SEEK_SET);
    if (err != 0) {
        printf("Error in fseek 2\n");
        return -1L;
    }
    return size;
}


int read_file(Dav1dData *out) {
    FILE *f;
    f = fopen("debug_dav1d_frame.bin", "r");
    if (f == NULL) {
        printf("Error opening file\n");
        return 1;
    }
    // file size
    long size = get_file_size(f);
    if (size < 0) return 1;

    uint8_t *buf = dav1d_data_create(out, size);

    size_t r = fread(buf, size, 1, f);
    if (r == 0) {
        printf("Error reading file\n");
        return 1;
    }
    fclose(f);
    return 0;
}


void consume_picture(Dav1dPicture *pic) {
    printf(
        "Width %d %d\n",
        pic->frame_hdr->width[0],
        pic->frame_hdr->width[1]);
    printf("Height %d\n", pic->frame_hdr->height);
    dav1d_picture_unref(pic);
}


int main() {
    int err;
    Dav1dSettings s = {0};
    Dav1dContext *context;
    Dav1dData data = {0};
    Dav1dPicture pic = {0};

    dav1d_default_settings(&s);
    s.n_threads = 2;

    printf("dav1d_open\n");
    err = dav1d_open(&context, &s);
    if (err != 0) {
        printf("Error in open %d\n", err);
        return 1;
    }

    printf("read_file\n");
    err = read_file(&data);
    if (err != 0) return 1;

    printf("dav1d_send_data\n");
    err = dav1d_send_data(context, &data);
    if (err != 0) {
        printf("Error in send_data %d\n", err);
        return 1;
    }

    // seems to destroy all images before they get read
    // printf("dav1d_flush\n");
    // dav1d_flush(context);

    printf("dav1d_get_picture\n");

    // naive reading, doesn't work
    // err = dav1d_get_picture(context, &pic);
    // if (err != 0) {
    //     printf("Error in get_picture %d\n", err);
    //     return 1;
    // }
    // consume_picture(&pic);

    // reading with retries, works!
    do {
        err = dav1d_get_picture(context, &pic);
        if (err == 0) {
            consume_picture(&pic);
            break;
        }
        if (err != DAV1D_ERR(EAGAIN)) {
            printf("Error in get_picture %d\n", err);
            return 1;
        }
        printf(".");
    } while (1);

    printf("All done\n");

    dav1d_close(&context);
    return 0;
}

[neumond]

This comment is wrong. It's required to call get_picture until you get the result, if you've finished sending the data.

dav1d-rs/tools/src/main.rs

Lines 81 to 82 in 0313f7d

	// Need to send more data to the decoder before it can decode new pictures
	Err(e) if e.is_again() => return,

[sdroege]

According to the documentation of the C API docs, the expected behaviour is

 *  Dav1dData data = { 0 };
 *  Dav1dPicture p = { 0 };
 *  int res;
 *
 *  read_data(&data);
 *  do {
 *      res = dav1d_send_data(c, &data);
 *      // Keep going even if the function can't consume the current data
 *         packet. It eventually will after one or more frames have been
 *         returned in this loop.
 *      if (res < 0 && res != DAV1D_ERR(EAGAIN))
 *          free_and_abort();
 *      res = dav1d_get_picture(c, &p);
 *      if (res < 0) {
 *          if (res != DAV1D_ERR(EAGAIN))
 *              free_and_abort();
 *      } else
 *          output_and_unref_picture(&p);
 *  // Stay in the loop as long as there's data to consume.
 *  } while (data.sz || read_data(&data) == SUCCESS);
 *
 *  // Handle EOS by draining all buffered frames.
 *  do {
 *      res = dav1d_get_picture(c, &p);
 *      if (res < 0) {
 *          if (res != DAV1D_ERR(EAGAIN))
 *              free_and_abort();
 *      } else
 *          output_and_unref_picture(&p);
 *  } while (res == 0);

I misremembered about flush(). That indeed discards all pending frames, which is not what you want here. But according to the above it should be required to call get_picture() at most once with EAGAIN (one time EAGAIN, next times you get the pending pictures).

See also

 * @note To drain buffered frames from the decoder (i.e. on end of stream),
 *       call this function until it returns DAV1D_ERR(EAGAIN).

[sdroege]

To be more clear: get_picture() called once is setting drain = 1 and might return EAGAIN even if not all pictures were returned yet. The next time you call it without having flushed or having sent more data (both reset drain = 0), it will wait (if drain == 1 still) until all pending pictures are returned and not return EAGAIN again until that happened.

The dav1d API is hard to use correctly unfortunately.

[neumond]

I would rather rely on getting EAGAIN twice in a row. Looks like this mechanism can be abstracted away as an iterator of pictures, which takes iterator of source data as construction argument.

[sdroege]

That would only make sense if you have an iterator of all data until the end of the stream, which is not necessarily the case. You rarely have all input data in memory but have to read it from somewhere.

And you don't want to drain the decoder after every single frame either as that reduces parallelism.

---

A better API for the bindings is certainly possible though, so if you have some ideas that would be great :)

[neumond]

Iterators are lazy, they don't require all the data in memory. This is roughly my idea with iterators, without optimizations and with some unwraps inside:

struct Dav1dDecoder {
    decoder: dav1d::Decoder,
    source: Box<dyn Iterator<Item = (usize, Vec<u8>)>>,
    current_data: Option<(usize, Vec<u8>)>,
}


impl Dav1dDecoder {
    fn create(
        settings: dav1d::Settings,
        source: Box<dyn Iterator<Item = Vec<u8>>>,
    ) -> Result<Self, dav1d::Error> {
        Ok(Self {
            decoder: dav1d::Decoder::with_settings(&settings)?,
            source: Box::new(source.fuse().enumerate()),
            current_data: None,
        })
    }
}


impl Iterator for Dav1dDecoder {
    type Item = dav1d::Picture;

    fn next(&mut self) -> Option<Self::Item> {
        let mut agains = 0;
        loop {
            if self.current_data.is_none() {
                self.current_data = self.source.next();
            }
            if self.current_data.is_some() {
                agains = 0;
                let (index, data) = self.current_data.take().unwrap();
                match self.decoder.send_data(data.to_vec(), Some(index as i64), None, None) {
                    Ok(..) => {},
                    Err(dav1d::Error::Again) => {
                        self.current_data = Some((index, data));
                    },
                    e @ Err(..) => e.unwrap(),
                }
            }
            match self.decoder.get_picture() {
                Ok(p) => return Some(p),
                Err(dav1d::Error::Again) => {
                    agains += 1;
                    if agains >= 2 { return None; }
                    continue;
                },
                e @ Err(..) => e.unwrap(),
            };
        };
    }
}


pub fn load_avifs() {
    let dsettings = dav1d::Settings::new();
    let mut decoder = Dav1dDecoder::create(dsettings, Box::new([
        "assets/texture1.avif",
        "assets/texture2.avif",
        "assets/texture3.avif",
        "assets/texture4.avif",
        "assets/texture5.avif",
        "assets/texture6.avif",
    ].iter().map(|fname| {
        let context = read_avif(&mut File::open(fname).unwrap()).unwrap();
        let vec = context.primary_item.to_vec();
        vec
    }))).unwrap();

    for picture in decoder {
        println!(
            "Offset {} {}x{}",
            picture.offset(), picture.width(), picture.height());
    }
}

[sdroege]

Iterators are lazy, they don't require all the data in memory.

You'd need to handle errors in the iterator then as reading can fail, and reading might be blocking or async so a Stream would probably also make sense in addition.

Also iterators are "pull" based, so some alternative for "push" based approaches would also be needed.

[neumond]

I'm not sure I'm able to invent an API wrapper that would fit everyone. Iterators solve my specific task quite well. It would be fine to have that as just an example. Probably EAGAIN could be replaced with distinct NoPicturesLeft+PictureNotReady and Sent+Buffered+Rejected, probably get_picture and send_data should have wait: bool parameter.

[lu-zero]

I guess Stream AsyncIterator might be the nicest API we could provide, up to a point.

[sdroege]

That's still a pull based API, so would make usage in other situations a bit inconvenient. Also the dav1d API does block under certain conditions, which is suboptimal for Rust futures.

Having an iterator-style API would seem useful but I don't think it can be the only one. Something like the current API seems necessary too, but the way how it's defined in C is rather hard to use correctly (see also: VLC and GStreamer were both using it incorrectly until earlier this month).