[libav-devel] [PATCH] Revert "decode: copy the output parameters from the last bsf in the chain back to the AVCodecContext"

[James Almer]

This reverts commit 662558f985f50834eebe82d6b6854c66f33ab320.

The avcodec_parameters_to_context() call was freeing and reallocating
AVCodecContext->extradata, essentially taking ownership of it, which according
to the doxy is user owned. This is an API break and has produces crashes in
some library users like Firefox.
Revert until a better solution is found to internally propagate the filtered
extradata back into the decoder context.

Signed-off-by: James Almer <***@gmail.com>
---
See https://bugzilla.mozilla.org/show_bug.cgi?id=1486080

Suggestions to work around it are very welcome, of course. While no bitstream
filter currently autoinserted by a decoder requires the filtered extradata to
be propagated to work properly, we don't know what may be needed in the future,
and without this the usability of bsfs in decoders is potentially limited.

libavcodec/decode.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index d10a2c8b5..2dab7f2a7 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -221,10 +221,6 @@ int ff_decode_bsfs_init(AVCodecContext *avctx)
goto fail;
}

- ret = avcodec_parameters_to_context(avctx, s->bsfs[s->nb_bsfs - 1]->par_out);
- if (ret < 0)
- return ret;
-
return 0;
fail:
ff_decode_bsfs_uninit(avctx);

[Luca Barbato]

Post by James Almer
This reverts commit 662558f985f50834eebe82d6b6854c66f33ab320.
The avcodec_parameters_to_context() call was freeing and reallocating
AVCodecContext->extradata, essentially taking ownership of it, which according
to the doxy is user owned. This is an API break and has produces crashes in
some library users like Firefox.
Revert until a better solution is found to internally propagate the filtered
extradata back into the decoder context.

The doxy says:

* - decoding: Set/allocated/freed by user.

We could be more explicit on what you can do with it though.

Post by James Almer
---
See https://bugzilla.mozilla.org/show_bug.cgi?id=1486080
Suggestions to work around it are very welcome, of course. While no bitstream
filter currently autoinserted by a decoder requires the filtered extradata to
be propagated to work properly, we don't know what may be needed in the future,
and without this the usability of bsfs in decoders is potentially limited.

We already have AV_PKT_DATA_NEW_EXTRADATA to deal with extradata changes
at the demuxer level, we might reuse it.

I'm fine with the revert.

lu

[James Almer]

Post by Luca Barbato

Post by James Almer
This reverts commit 662558f985f50834eebe82d6b6854c66f33ab320.
The avcodec_parameters_to_context() call was freeing and reallocating
AVCodecContext->extradata, essentially taking ownership of it, which according
to the doxy is user owned. This is an API break and has produces crashes in
some library users like Firefox.
Revert until a better solution is found to internally propagate the filtered
extradata back into the decoder context.

* - decoding: Set/allocated/freed by user.

Yes, meaning the user could just set this pointer to some buffer they
intend to reuse and free long after they closed and freed the
AVCodecContext. If libavcodec goes and tries to replace it, taking
ownership of it, then things can go very wrong.

For that matter, commit fd056029f45a9f6d213d9fce8165632042511d4f already
made that doxy obsolete by introducing avcodec_free_context() which
unconditionally frees extradata right after calling avcodec_close(), a
function that was doing it as well but only when it's an encoder.
The reason that commit didn't generate any widespread issues like this
one is because said users never migrated to avcodec_free_context() to
free the AVCodecContext.

Any suggestion on what to do? The above commit by Anton is four years
old. Do we make it official in the doxy that extradata is to be
allocated by the user (using av_malloc() functions) but then ownership
is passed to libavcodec after an avcodec_open2() call? It would require
to change how avcodec_close() frees the extradata as well.

Post by Luca Barbato
We could be more explicit on what you can do with it though.

Post by James Almer
---
See https://bugzilla.mozilla.org/show_bug.cgi?id=1486080
Suggestions to work around it are very welcome, of course. While no bitstream
filter currently autoinserted by a decoder requires the filtered extradata to
be propagated to work properly, we don't know what may be needed in the future,
and without this the usability of bsfs in decoders is potentially limited.

We already have AV_PKT_DATA_NEW_EXTRADATA to deal with extradata changes
at the demuxer level, we might reuse it.
I'm fine with the revert.
lu
_______________________________________________
libav-devel mailing list
https://lists.libav.org/mailman/listinfo/libav-devel

[Luca Barbato]

Post by James Almer
Any suggestion on what to do? The above commit by Anton is four years
old. Do we make it official in the doxy that extradata is to be
allocated by the user (using av_malloc() functions) but then ownership
is passed to libavcodec after an avcodec_open2() call? It would require
to change how avcodec_close() frees the extradata as well.

I'm not against it. Probably shouldn't cause major problems once it is
known.

lu