On 29/09/2020 19:23, James Almer wrote:
On 9/29/2020 3:07 PM, Mark Thompson wrote:
On 29/09/2020 17:17, James Almer wrote:
On 9/29/2020 12:57 PM, Mark Thompson wrote:
On 25/09/2020 15:43, James Almer wrote:
Fixes: member access within null pointer of type 'TileGroupInfo' (aka
'struct TileGroupInfo')
Fixes:
25725/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-5166692706287616
Found-by: continuous fuzzing process
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: James Almer <jamr...@gmail.com>
---
libavcodec/av1dec.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
index 07026b7aeb..e5cfc3f2f2 100644
--- a/libavcodec/av1dec.c
+++ b/libavcodec/av1dec.c
@@ -381,6 +381,20 @@ fail:
return AVERROR(ENOMEM);
}
+static void av1_decode_flush(AVCodecContext *avctx)
+{
+ AV1DecContext *s = avctx->priv_data;
+
+ for (int i = 0; i < FF_ARRAY_ELEMS(s->ref); i++)
+ av1_frame_unref(avctx, &s->ref[i]);
+
+ av1_frame_unref(avctx, &s->cur_frame);
+ s->raw_frame_header = NULL;
+ s->raw_seq = NULL;
+
+ ff_cbs_flush(s->cbc);
+}
+
static av_cold int av1_decode_free(AVCodecContext *avctx)
{
AV1DecContext *s = avctx->priv_data;
@@ -841,23 +855,11 @@ static int av1_decode_frame(AVCodecContext
*avctx, void *frame,
end:
ff_cbs_fragment_reset(&s->current_obu);
+ if (ret < 0)
+ av1_decode_flush(avctx);
return ret;
}
-static void av1_decode_flush(AVCodecContext *avctx)
-{
- AV1DecContext *s = avctx->priv_data;
-
- for (int i = 0; i < FF_ARRAY_ELEMS(s->ref); i++)
- av1_frame_unref(avctx, &s->ref[i]);
-
- av1_frame_unref(avctx, &s->cur_frame);
- s->raw_frame_header = NULL;
- s->raw_seq = NULL;
-
- ff_cbs_flush(s->cbc);
-}
-
AVCodec ff_av1_decoder = {
.name = "av1",
.long_name = NULL_IF_CONFIG_SMALL("Alliance for Open
Media AV1"),
This seems questionable - if you have some packet loss and lose an
intermediate frame, I don't think you want to throw away all the old
state since it may be able to continue from an earlier frame which was
received correctly.
If a frame was not parsed correctly, then the reference frame state from
then on will be invalid. Especially if ff_cbs_read_packet() is what
failed, considering everything after the corrupt OBU within the TU will
be discarded by CBS.
Only the reference frame state which the frame would have modified has
changed, which need not be all of it if the encoder knows it is going to
be sending over an unreliable channel.
Also intra refresh, though I haven't seen that implemented in an AV1
encoder yet.
I can replace this to simply setting raw_frame_header to NULL if you
prefer, but for reference, dav1d (the decoder that hopefully will
eventually be ported into this native decoder) throws the entire state
away on a frame decoding failure.
I think that answer is preferable, though I guess it could just be
ignored for now and fixed when it comes up in future. (Presumably noone
has tried to use dav1d for these sorts of cases yet.)
Which option do you prefer, then? If i just set raw_frame_header to NULL
on failure and do nothing else, future frames will be parsed and
get_buffer() will still be called despite no pix_fmt being set (And thus
fail).
I think you can avoid that by making sure that AV1ReferenceFrameState.valid is
false for the frame that was lost?
Future frames will fail to parse if they refer directly to it, and if they
don't then they will continue to work.
If i also set raw_seq to NULL on failure then that can be avoided, but
it will be more or less functionally the same as this patch seeing that
no frame will be handled by this decoder until a new sequence header is
found, even if the CBS state was left intact.
Throwing away the sequence header doesn't seem right.
- Mark
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
