Re: [FFmpeg-devel] [PATCH] avcodec/decode: use a single list bsf for codec decode bsfs

[Marton Balint]

On Sat, 25 Apr 2020, James Almer wrote:


On 4/25/2020 7:11 PM, Marton Balint wrote:

Signed-off-by: Marton Balint 
---
 libavcodec/cuviddec.c |   2 +-
 libavcodec/decode.c   | 162 +++---
 libavcodec/internal.h |   3 +-
 3 files changed, 22 insertions(+), 145 deletions(-)

diff --git a/libavcodec/cuviddec.c b/libavcodec/cuviddec.c
index 50dc8956c3..13a7db10cd 100644
--- a/libavcodec/cuviddec.c
+++ b/libavcodec/cuviddec.c
@@ -946,7 +946,7 @@ static av_cold int cuvid_decode_init(AVCodecContext *avctx)
 }

 if (avctx->codec->bsfs) {
-const AVCodecParameters *par = 
avctx->internal->filter.bsfs[avctx->internal->filter.nb_bsfs - 1]->par_out;
+const AVCodecParameters *par = avctx->internal->filter.bsf->par_out;
 ctx->cuparse_ext.format.seqhdr_data_length = par->extradata_size;
 memcpy(ctx->cuparse_ext.raw_seqhdr_data,
par->extradata,
diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index d4bdb9b1c0..167eaa6bb6 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -205,100 +205,28 @@ int ff_decode_bsfs_init(AVCodecContext *avctx)
 {
 AVCodecInternal *avci = avctx->internal;
 DecodeFilterContext *s = &avci->filter;
-const char *bsfs_str;
 int ret;

-if (s->nb_bsfs)
+if (s->bsf)
 return 0;

-bsfs_str = avctx->codec->bsfs ? avctx->codec->bsfs : "null";


If i'm reading this right, if the string passed to
av_bsf_list_parse_str() results in no bsf being inserted to the list,
ff_list_bsf acts the same as if it was the null bsf, right?


Yes.




-while (bsfs_str && *bsfs_str) {
-AVBSFContext **tmp;
-const AVBitStreamFilter *filter;
-char *bsf, *bsf_options_str, *bsf_name;
-
-bsf = av_get_token(&bsfs_str, ",");
-if (!bsf) {
-ret = AVERROR(ENOMEM);
-goto fail;
-}
-bsf_name = av_strtok(bsf, "=", &bsf_options_str);
-if (!bsf_name) {
-av_freep(&bsf);
-ret = AVERROR(ENOMEM);
-goto fail;
-}
-
-filter = av_bsf_get_by_name(bsf_name);
-if (!filter) {
-av_log(avctx, AV_LOG_ERROR, "A non-existing bitstream filter %s "
-   "requested by a decoder. This is a bug, please report 
it.\n",
-   bsf_name);
-av_freep(&bsf);
-ret = AVERROR_BUG;
-goto fail;
-}
-
-tmp = av_realloc_array(s->bsfs, s->nb_bsfs + 1, sizeof(*s->bsfs));
-if (!tmp) {
-av_freep(&bsf);
-ret = AVERROR(ENOMEM);
-goto fail;
-}
-s->bsfs = tmp;
-
-ret = av_bsf_alloc(filter, &s->bsfs[s->nb_bsfs]);
-if (ret < 0) {
-av_freep(&bsf);
-goto fail;
-}
-s->nb_bsfs++;
-
-if (s->nb_bsfs == 1) {
-/* We do not currently have an API for passing the input timebase 
into decoders,
- * but no filters used here should actually need it.
- * So we make up some plausible-looking number (the MPEG 90kHz 
timebase) */
-s->bsfs[s->nb_bsfs - 1]->time_base_in = (AVRational){ 1, 9 };
-ret = avcodec_parameters_from_context(s->bsfs[s->nb_bsfs - 
1]->par_in,
-  avctx);
-} else {
-s->bsfs[s->nb_bsfs - 1]->time_base_in = s->bsfs[s->nb_bsfs - 
2]->time_base_out;
-ret = avcodec_parameters_copy(s->bsfs[s->nb_bsfs - 1]->par_in,
-  s->bsfs[s->nb_bsfs - 2]->par_out);
-}
-if (ret < 0) {
-av_freep(&bsf);
-goto fail;
-}
-
-if (bsf_options_str && filter->priv_class) {
-const AVOption *opt = av_opt_next(s->bsfs[s->nb_bsfs - 
1]->priv_data, NULL);
-const char * shorthand[2] = {NULL};
-
-if (opt)
-shorthand[0] = opt->name;
-
-ret = av_opt_set_from_string(s->bsfs[s->nb_bsfs - 1]->priv_data, bsf_options_str, 
shorthand, "=", ":");
-if (ret < 0) {
-if (ret != AVERROR(ENOMEM)) {
-av_log(avctx, AV_LOG_ERROR, "Invalid options for bitstream 
filter %s "
-   "requested by the decoder. This is a bug, please report 
it.\n",
-   bsf_name);
-ret = AVERROR_BUG;
-}
-av_freep(&bsf);
-goto fail;
-}
-}
-av_freep(&bsf);
+ret = av_bsf_list_parse_str(avctx->codec->bsfs, &s->bsf);
+if (ret < 0) {
+av_log(avctx, AV_LOG_ERROR, "Error parsing decoder bitstream filters '%s': 
%s\n", avctx->codec->bsfs, av_err2str(ret));
+goto fail;


You should keep the AVERROR_BUG code from above. Decoders insert bsfs
with hardcoded arguments and they absolutely must be valid. Otherwise
it's a bug in our code.

Return ENOMEM if that w

Re: [FFmpeg-devel] [PATCH] avcodec/decode: use a single list bsf for codec decode bsfs

[James Almer]

On 4/25/2020 7:11 PM, Marton Balint wrote:
> Signed-off-by: Marton Balint 
> ---
>  libavcodec/cuviddec.c |   2 +-
>  libavcodec/decode.c   | 162 
> +++---
>  libavcodec/internal.h |   3 +-
>  3 files changed, 22 insertions(+), 145 deletions(-)
> 
> diff --git a/libavcodec/cuviddec.c b/libavcodec/cuviddec.c
> index 50dc8956c3..13a7db10cd 100644
> --- a/libavcodec/cuviddec.c
> +++ b/libavcodec/cuviddec.c
> @@ -946,7 +946,7 @@ static av_cold int cuvid_decode_init(AVCodecContext 
> *avctx)
>  }
>  
>  if (avctx->codec->bsfs) {
> -const AVCodecParameters *par = 
> avctx->internal->filter.bsfs[avctx->internal->filter.nb_bsfs - 1]->par_out;
> +const AVCodecParameters *par = avctx->internal->filter.bsf->par_out;
>  ctx->cuparse_ext.format.seqhdr_data_length = par->extradata_size;
>  memcpy(ctx->cuparse_ext.raw_seqhdr_data,
> par->extradata,
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index d4bdb9b1c0..167eaa6bb6 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -205,100 +205,28 @@ int ff_decode_bsfs_init(AVCodecContext *avctx)
>  {
>  AVCodecInternal *avci = avctx->internal;
>  DecodeFilterContext *s = &avci->filter;
> -const char *bsfs_str;
>  int ret;
>  
> -if (s->nb_bsfs)
> +if (s->bsf)
>  return 0;
>  
> -bsfs_str = avctx->codec->bsfs ? avctx->codec->bsfs : "null";

If i'm reading this right, if the string passed to
av_bsf_list_parse_str() results in no bsf being inserted to the list,
ff_list_bsf acts the same as if it was the null bsf, right?

> -while (bsfs_str && *bsfs_str) {
> -AVBSFContext **tmp;
> -const AVBitStreamFilter *filter;
> -char *bsf, *bsf_options_str, *bsf_name;
> -
> -bsf = av_get_token(&bsfs_str, ",");
> -if (!bsf) {
> -ret = AVERROR(ENOMEM);
> -goto fail;
> -}
> -bsf_name = av_strtok(bsf, "=", &bsf_options_str);
> -if (!bsf_name) {
> -av_freep(&bsf);
> -ret = AVERROR(ENOMEM);
> -goto fail;
> -}
> -
> -filter = av_bsf_get_by_name(bsf_name);
> -if (!filter) {
> -av_log(avctx, AV_LOG_ERROR, "A non-existing bitstream filter %s "
> -   "requested by a decoder. This is a bug, please report 
> it.\n",
> -   bsf_name);
> -av_freep(&bsf);
> -ret = AVERROR_BUG;
> -goto fail;
> -}
> -
> -tmp = av_realloc_array(s->bsfs, s->nb_bsfs + 1, sizeof(*s->bsfs));
> -if (!tmp) {
> -av_freep(&bsf);
> -ret = AVERROR(ENOMEM);
> -goto fail;
> -}
> -s->bsfs = tmp;
> -
> -ret = av_bsf_alloc(filter, &s->bsfs[s->nb_bsfs]);
> -if (ret < 0) {
> -av_freep(&bsf);
> -goto fail;
> -}
> -s->nb_bsfs++;
> -
> -if (s->nb_bsfs == 1) {
> -/* We do not currently have an API for passing the input 
> timebase into decoders,
> - * but no filters used here should actually need it.
> - * So we make up some plausible-looking number (the MPEG 90kHz 
> timebase) */
> -s->bsfs[s->nb_bsfs - 1]->time_base_in = (AVRational){ 1, 9 };
> -ret = avcodec_parameters_from_context(s->bsfs[s->nb_bsfs - 
> 1]->par_in,
> -  avctx);
> -} else {
> -s->bsfs[s->nb_bsfs - 1]->time_base_in = s->bsfs[s->nb_bsfs - 
> 2]->time_base_out;
> -ret = avcodec_parameters_copy(s->bsfs[s->nb_bsfs - 1]->par_in,
> -  s->bsfs[s->nb_bsfs - 2]->par_out);
> -}
> -if (ret < 0) {
> -av_freep(&bsf);
> -goto fail;
> -}
> -
> -if (bsf_options_str && filter->priv_class) {
> -const AVOption *opt = av_opt_next(s->bsfs[s->nb_bsfs - 
> 1]->priv_data, NULL);
> -const char * shorthand[2] = {NULL};
> -
> -if (opt)
> -shorthand[0] = opt->name;
> -
> -ret = av_opt_set_from_string(s->bsfs[s->nb_bsfs - 1]->priv_data, 
> bsf_options_str, shorthand, "=", ":");
> -if (ret < 0) {
> -if (ret != AVERROR(ENOMEM)) {
> -av_log(avctx, AV_LOG_ERROR, "Invalid options for 
> bitstream filter %s "
> -   "requested by the decoder. This is a bug, please 
> report it.\n",
> -   bsf_name);
> -ret = AVERROR_BUG;
> -}
> -av_freep(&bsf);
> -goto fail;
> -}
> -}
> -av_freep(&bsf);
> +ret = av_bsf_list_parse_str(avctx->codec->bsfs, &s->bsf);
> +if (ret < 0) {
> +av_log(avctx, AV_LOG_ERROR, "Error parsing decoder bitstream filters 
> '%s': %s\n", avctx->codec->bsfs, av_err2str(ret));