Free reference when data is incomplete.

[vrabaud]

That can be problematic if we decode 1 YUV tile, 2 alpha tiles and we are back to decoding 1 YUV tile.

This fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=66313

[y-guyon]

Good job debugging this, it was not easy!

[vrabaud]

Hmm, that fixes it but there might be a side effect.

[wantehchang]

Vincent: I don't understand this change. Assuming this change is correct, we need to delete the comment // send more data.

The dav1d_data_unref(&dav1dData) call will zero the dav1dData struct, so when we loop back to line 92, dataData.data will be null and the dav1d_send_data() call at line 93 will be skipped. So we won't be sending more data.

[vrabaud]

Right, that's why I thought there would be side effects. But reading the official snippet at https://github.com/videolan/dav1d/blob/a6878be7e07114f5a2915ad46300700f0db55197/include/dav1d/dav1d.h#L213, our code was wrong. As no new data is sent, no need to continue indeed.

[wantehchang]

The official snippet keeps calling dav1d_send_data(c, &data) while data.sz is nonzero. Our original code matches that, except that we check dav1dData.data instead of dav1dData.sz (lines 102-105):

            if (dav1dData.data) {
                // send more data
                continue;
            }

So in our original code, if we change the if (dav1dData.data) checks at line 92 and line 102 to if (dav1dData.sz), then we will match that part of the official snippet exactly.

[wantehchang]

Please apply the following patch. First, in src/codec_dav1d.c, change #if 1 to #if 0. Run the test. Study the log messages (search for "WTC"). Then, change #if 0 back to #if 1. Run the test. Study the log messages.

diff --git a/src/codec_dav1d.c b/src/codec_dav1d.c
index 0fb36c5c..d720be8a 100644
--- a/src/codec_dav1d.c
+++ b/src/codec_dav1d.c
@@ -3,6 +3,8 @@
 
 #include "avif/internal.h"
 
+#include <stdio.h>
+
 #if defined(_MSC_VER)
 #pragma warning(disable : 4201) // nonstandard extension used: nameless struct/union
 #endif
@@ -88,16 +90,25 @@ static avifBool dav1dCodecGetNextImage(struct avifCodec * codec,
         return AVIF_FALSE;
     }
 
+    fprintf(stderr, "WTC WTC WTC: BEGIN FOR LOOP: alpha=%d sz=%zu\n",
+            alpha, dav1dData.sz);
     for (;;) {
         if (dav1dData.data) {
+            fprintf(stderr, "WTC WTC WTC:   Calling dav1d_send_data(%p, sz=%zu)\n",
+                    codec->internal->dav1dContext, dav1dData.sz);
             int res = dav1d_send_data(codec->internal->dav1dContext, &dav1dData);
+            fprintf(stderr, "WTC WTC WTC:   res=%d, sz=%zu\n",
+                    res, dav1dData.sz);
             if ((res < 0) && (res != DAV1D_ERR(EAGAIN))) {
                 dav1d_data_unref(&dav1dData);
                 return AVIF_FALSE;
             }
         }
 
+        fprintf(stderr, "WTC WTC WTC:   Calling dav1d_get_picture(%p)\n",
+                codec->internal->dav1dContext);
         int res = dav1d_get_picture(codec->internal->dav1dContext, &nextFrame);
+        fprintf(stderr, "WTC WTC WTC:   res=%d\n", res);
         if (res == DAV1D_ERR(EAGAIN)) {
             if (dav1dData.data) {
                 // send more data
@@ -121,10 +132,36 @@ static avifBool dav1dCodecGetNextImage(struct avifCodec * codec,
             }
         }
     }
+    fprintf(stderr, "WTC WTC WTC: END FOR LOOP\n");
     if (dav1dData.data) {
         dav1d_data_unref(&dav1dData);
     }
 
+#if 1
+    // Drain all buffered frames in the decoder.
+    int res;
+    fprintf(stderr, "WTC WTC WTC: BEGIN DRAIN LOOP\n");
+    do {
+        Dav1dPicture frame;
+        memset(&frame, 0, sizeof(Dav1dPicture));
+        fprintf(stderr, "WTC WTC WTC:   Calling dav1d_get_picture(%p)\n",
+                codec->internal->dav1dContext);
+        res = dav1d_get_picture(codec->internal->dav1dContext, &frame);
+        fprintf(stderr, "WTC WTC WTC:   res=%d\n", res);
+        if (res < 0) {
+            if (res != DAV1D_ERR(EAGAIN)) {
+                if (gotPicture) {
+                    dav1d_picture_unref(&nextFrame);
+                }
+                return AVIF_FALSE;
+            }
+        } else {
+            dav1d_picture_unref(&frame);
+        }
+    } while (res == 0);
+    fprintf(stderr, "WTC WTC WTC: END DRAIN LOOP\n");
+#endif
+
     if (gotPicture) {
         dav1d_picture_unref(&codec->internal->dav1dPicture);
         codec->internal->dav1dPicture = nextFrame;
diff --git a/src/read.c b/src/read.c
index 4bc25479..e43062e5 100644
--- a/src/read.c
+++ b/src/read.c
@@ -4290,6 +4290,8 @@ static avifResult avifDecoderCreateCodecs(avifDecoder * decoder)
         // Otherwise, we will use |tiles.count| decoder instances (one instance for each tile).
         avifBool canUseSingleCodecInstance = (data->tiles.count == 1) ||
                                              (decoder->imageCount == 1 && avifTilesCanBeDecodedWithSameCodecInstance(data));
+        fprintf(stderr, "WTC WTC WTC: canUseSingleCodecInstance=%d\n",
+                canUseSingleCodecInstance);
         if (canUseSingleCodecInstance) {
             AVIF_CHECKRES(avifCodecCreateInternal(decoder->codecChoice, &decoder->data->tiles.tile[0], &decoder->diag, &data->codec));
             for (unsigned int i = 0; i < decoder->data->tiles.count; ++i) {

[wantehchang]

@vigneshvg Vignesh: Please take a look at this bug because this is related to decoding all tiles with a single decoder instance.

In the test output, the log messages show that the input data of size 1613 bytes contains three frames. If we don't drain the buffered frames in the decoder, when the decoder is used to decode the next tile, the first buffered frame will be output.

Note: The input data of size 125 bytes contains one frame followed by an unknown OBU (if dav1d_log is enabled, dav1d will emit the warning message "Unknown OBU type 14 of size 26"). If the decoder is not drained, when the decoder is used to decode the next tile, the buffered unknown OBU will be processed first.

I think we should fix this bug by adding a drain loop (the code inside #if 1 in the patch). Both the official example in dav1d.h and the dav1d.c program have a drain loop.

[vigneshvg]

I think we should fix this bug by adding a drain loop (the code inside #if 1 in the patch). Both the official example in dav1d.h and the dav1d.c program have a drain loop.

That does seem like the right fix. But i have a question about what this actually means, if the color tile has 3 frames inside one OBU unit, then what is the right AVIF behavior? Should we return the first returned frame or the last one?

[wantehchang]

I think this can be considered as undefined behavior, so we can either return an error or do something reasonable. I suggest we should return the first frame in the sample data that meets the criteria (if there is a layer selector), because this is easier to implement. We just need to add a loop to drain any subsequent frames.

The drain loop may degrade performance, so we need to make sure dav1d_get_picture() returns DAV1D_ERR(EAGAIN) quickly when there are no buffered frames in the decoder.

[wantehchang]

I implemented the drain loop in #2002.

[wantehchang]

Vincent: I will take a closer look when I get to the office.

[wantehchang]

The official snippet keeps calling dav1d_send_data(c, &data) while data.sz is nonzero. Our original code matches that, except that we check dav1dData.data instead of dav1dData.sz (lines 102-105):

            if (dav1dData.data) {
                // send more data
                continue;
            }

So in our original code, if we change the if (dav1dData.data) checks at line 92 and line 102 to if (dav1dData.sz), then we will match that part of the official snippet exactly.

[vrabaud]

Closing in favor of #2002