| Message ID | 20210709145301.3715-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/libdav1d: parse sequence headers in extradata if available | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On 7/9/2021 3:53 PM, James Almer wrote: > + res = dav1d_parse_sequence_header(&seq, c->extradata + offset, > + c->extradata_size - offset); > + if (res < 0) { > + av_log(c, explode ? AV_LOG_ERROR : AV_LOG_INFO, > + "Error decoding extradata\n"); > + return explode ? AVERROR_INVALIDDATA : 0; > + } > + I don't think it is a good idea to fail like this, even in explode mode. Both the AV1-in-ISOBMFF and AV1-in-Matroska specs do specify that the sequence header OBU must be first if there is one in the configOBUs, however, they do not require on actually be present. For example, configOBUs may contain a single metadata OBU, and nothing else - and it would be entirely valid - so failing here would actually be wrong, even in explode mode. This can happen, if, for example, you have a file with HDR metadata in the av1c box, but in-band sequence headers. - Derek
On 7/11/2021 8:40 AM, Derek Buitenhuis wrote: > On 7/9/2021 3:53 PM, James Almer wrote: >> + res = dav1d_parse_sequence_header(&seq, c->extradata + offset, >> + c->extradata_size - offset); >> + if (res < 0) { >> + av_log(c, explode ? AV_LOG_ERROR : AV_LOG_INFO, >> + "Error decoding extradata\n"); >> + return explode ? AVERROR_INVALIDDATA : 0; >> + } >> + > > I don't think it is a good idea to fail like this, even in explode > mode. > > Both the AV1-in-ISOBMFF and AV1-in-Matroska specs do specify that > the sequence header OBU must be first if there is one in the > configOBUs, however, they do not require on actually be present. > > For example, configOBUs may contain a single metadata OBU, and > nothing else - and it would be entirely valid - so failing here > would actually be wrong, even in explode mode. This can happen, > if, for example, you have a file with HDR metadata in the av1c box, > but in-band sequence headers. I can amend this patch locally to check for DAV1D_ERR(ENOENT), which was implemented to signal "No seqhdr is present", and return 0 in that case, but it's a very recent addition, so older libdav1d builds will never emit that error code. Guess just returning 0 on all scenarios here until we increase the minimum required library version is better, so I'll do that. > > - Derek > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c index c39df418d5..8132ff1593 100644 --- a/libavcodec/libdav1d.c +++ b/libavcodec/libdav1d.c @@ -158,6 +158,47 @@ static void libdav1d_init_params(AVCodecContext *c, const Dav1dSequenceHeader *s } } +static av_cold int libdav1d_parse_extradata(AVCodecContext *c) +{ + Dav1dSequenceHeader seq; + size_t offset = 0; + int res, explode = !!(c->err_recognition & AV_EF_EXPLODE); + + if (!c->extradata || c->extradata_size <= 0) + return 0; + + if (c->extradata[0] & 0x80) { + int version = c->extradata[0] & 0x7F; + + if (version != 1 || c->extradata_size < 4) { + av_log(c, explode ? AV_LOG_ERROR : AV_LOG_INFO, + "Error decoding extradata\n"); + return explode ? AVERROR_INVALIDDATA : 0; + } + + // Do nothing if there are no configOBUs to parse + if (c->extradata_size == 4) + return 0; + + offset = 4; + } + + res = dav1d_parse_sequence_header(&seq, c->extradata + offset, + c->extradata_size - offset); + if (res < 0) { + av_log(c, explode ? AV_LOG_ERROR : AV_LOG_INFO, + "Error decoding extradata\n"); + return explode ? AVERROR_INVALIDDATA : 0; + } + + libdav1d_init_params(c, &seq); + res = ff_set_dimensions(c, seq.max_width, seq.max_height); + if (res < 0) + return res; + + return 0; +} + static av_cold int libdav1d_init(AVCodecContext *c) { Libdav1dContext *dav1d = c->priv_data; @@ -192,6 +233,10 @@ static av_cold int libdav1d_init(AVCodecContext *c) av_log(c, AV_LOG_DEBUG, "Using %d frame threads, %d tile threads\n", s.n_frame_threads, s.n_tile_threads); + res = libdav1d_parse_extradata(c); + if (res < 0) + return res; + res = dav1d_open(&dav1d->c, &s); if (res < 0) return AVERROR(ENOMEM);
This allows the decoder context to be initialized with all stream parameters before a packet is parsed. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/libdav1d.c | 45 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)