[FFmpeg-devel] avcodec: parse options from AVCodec.bsfs

Submitted by Aman Gupta on July 18, 2018, 6:57 p.m.

Details

Message ID 20180718185724.10578-1-ffmpeg@tmm1.net
State New
Headers show

Commit Message

Aman Gupta July 18, 2018, 6:57 p.m.
From: Aman Gupta <aman@tmm1.net>

Fixes a bug that would prevent using multiple comma-separated filters,
and allows options to be passed to each filter.

Based on similar loop in ffmpeg_opt.c's new_output_stream().

Signed-off-by: Aman Gupta <aman@tmm1.net>
---
 libavcodec/decode.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 7 deletions(-)

Comments

James Almer July 19, 2018, 1:37 a.m.
On 7/18/2018 3:57 PM, Aman Gupta wrote:
> From: Aman Gupta <aman@tmm1.net>
> 

avcodec/decode: parse options from AVCodec.bsfs

> Fixes a bug that would prevent using multiple comma-separated filters,
> and allows options to be passed to each filter.
> 
> Based on similar loop in ffmpeg_opt.c's new_output_stream().
> 
> Signed-off-by: Aman Gupta <aman@tmm1.net>
> ---
>  libavcodec/decode.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 40 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index 6a3a4df179..67b7443b9d 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -36,6 +36,7 @@
>  #include "libavutil/imgutils.h"
>  #include "libavutil/internal.h"
>  #include "libavutil/intmath.h"
> +#include "libavutil/opt.h"
>  
>  #include "avcodec.h"
>  #include "bytestream.h"
> @@ -195,27 +196,33 @@ static int bsfs_init(AVCodecContext *avctx)
>      while (bsfs_str && *bsfs_str) {
>          AVBSFContext **tmp;
>          const AVBitStreamFilter *filter;
> -        char *bsf;
> +        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);
> +        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);
> -            ret = AVERROR_BUG;
> +                   bsf_name);
>              av_freep(&bsf);
> +            ret = AVERROR_BUG;
>              goto fail;
>          }
> -        av_freep(&bsf);
>  
>          tmp = av_realloc_array(s->bsfs, s->nb_bsfs + 1, sizeof(*s->bsfs));
>          if (!tmp) {
> +            av_freep(&bsf);
>              ret = AVERROR(ENOMEM);
>              goto fail;
>          }
> @@ -223,8 +230,10 @@ static int bsfs_init(AVCodecContext *avctx)
>          s->nb_bsfs++;
>  
>          ret = av_bsf_alloc(filter, &s->bsfs[s->nb_bsfs - 1]);
> -        if (ret < 0)
> +        if (ret < 0) {
> +            av_freep(&bsf);
>              goto fail;
> +        }
>  
>          if (s->nb_bsfs == 1) {
>              /* We do not currently have an API for passing the input timebase into decoders,
> @@ -238,12 +247,36 @@ static int bsfs_init(AVCodecContext *avctx)
>              ret = avcodec_parameters_copy(s->bsfs[s->nb_bsfs - 1]->par_in,
>                                            s->bsfs[s->nb_bsfs - 2]->par_out);
>          }
> -        if (ret < 0)
> +        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) {
> +                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);
> +                av_freep(&bsf);
> +                ret = AVERROR_BUG;
> +                goto fail;
> +            }

As i said on IRC, av_opt_set_from_string() can return ENOMEM which is
not a bug in the string contents, so do something like

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_init(s->bsfs[s->nb_bsfs - 1]);
>          if (ret < 0)
>              goto fail;
> +
> +        if (*bsfs_str)
> +            bsfs_str++;
>      }
>  
>      return 0;
> 

Should be ok otherwise.
Aman Gupta July 19, 2018, 4:19 p.m.
On Wed, Jul 18, 2018 at 6:38 PM James Almer <jamrial@gmail.com> wrote:

> On 7/18/2018 3:57 PM, Aman Gupta wrote:
> > From: Aman Gupta <aman@tmm1.net>
> >
>
> avcodec/decode: parse options from AVCodec.bsfs
>
> > Fixes a bug that would prevent using multiple comma-separated filters,
> > and allows options to be passed to each filter.
> >
> > Based on similar loop in ffmpeg_opt.c's new_output_stream().
> >
> > Signed-off-by: Aman Gupta <aman@tmm1.net>
> > ---
> >  libavcodec/decode.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 40 insertions(+), 7 deletions(-)
> >
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index 6a3a4df179..67b7443b9d 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -36,6 +36,7 @@
> >  #include "libavutil/imgutils.h"
> >  #include "libavutil/internal.h"
> >  #include "libavutil/intmath.h"
> > +#include "libavutil/opt.h"
> >
> >  #include "avcodec.h"
> >  #include "bytestream.h"
> > @@ -195,27 +196,33 @@ static int bsfs_init(AVCodecContext *avctx)
> >      while (bsfs_str && *bsfs_str) {
> >          AVBSFContext **tmp;
> >          const AVBitStreamFilter *filter;
> > -        char *bsf;
> > +        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);
> > +        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);
> > -            ret = AVERROR_BUG;
> > +                   bsf_name);
> >              av_freep(&bsf);
> > +            ret = AVERROR_BUG;
> >              goto fail;
> >          }
> > -        av_freep(&bsf);
> >
> >          tmp = av_realloc_array(s->bsfs, s->nb_bsfs + 1,
> sizeof(*s->bsfs));
> >          if (!tmp) {
> > +            av_freep(&bsf);
> >              ret = AVERROR(ENOMEM);
> >              goto fail;
> >          }
> > @@ -223,8 +230,10 @@ static int bsfs_init(AVCodecContext *avctx)
> >          s->nb_bsfs++;
> >
> >          ret = av_bsf_alloc(filter, &s->bsfs[s->nb_bsfs - 1]);
> > -        if (ret < 0)
> > +        if (ret < 0) {
> > +            av_freep(&bsf);
> >              goto fail;
> > +        }
> >
> >          if (s->nb_bsfs == 1) {
> >              /* We do not currently have an API for passing the input
> timebase into decoders,
> > @@ -238,12 +247,36 @@ static int bsfs_init(AVCodecContext *avctx)
> >              ret = avcodec_parameters_copy(s->bsfs[s->nb_bsfs -
> 1]->par_in,
> >                                            s->bsfs[s->nb_bsfs -
> 2]->par_out);
> >          }
> > -        if (ret < 0)
> > +        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) {
> > +                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);
> > +                av_freep(&bsf);
> > +                ret = AVERROR_BUG;
> > +                goto fail;
> > +            }
>
> As i said on IRC, av_opt_set_from_string() can return ENOMEM which is
> not a bug in the string contents, so do something like
>
> 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;
> }
>

Thanks, fixed and applied.


>
> > +        }
> > +        av_freep(&bsf);
> >
> >          ret = av_bsf_init(s->bsfs[s->nb_bsfs - 1]);
> >          if (ret < 0)
> >              goto fail;
> > +
> > +        if (*bsfs_str)
> > +            bsfs_str++;
> >      }
> >
> >      return 0;
> >
>
> Should be ok otherwise.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Carl Eugen Hoyos July 19, 2018, 6:44 p.m.
2018-07-19 3:37 GMT+02:00, James Almer <jamrial@gmail.com>:
> On 7/18/2018 3:57 PM, Aman Gupta wrote:

>> +            ret = av_opt_set_from_string(s->bsfs[s->nb_bsfs -
>> 1]->priv_data, bsf_options_str, shorthand, "=", ":");
>> +            if (ret < 0) {
>> +                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);
>> +                av_freep(&bsf);
>> +                ret = AVERROR_BUG;
>> +                goto fail;
>> +            }
>
> As i said on IRC, av_opt_set_from_string() can return ENOMEM which is
> not a bug in the string contents, so do something like
>
> 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_assert(ret == AVERROR(ENOMEM)); ?

Carl Eugen
Aman Gupta July 19, 2018, 7 p.m.
On Thu, Jul 19, 2018 at 11:44 AM Carl Eugen Hoyos <ceffmpeg@gmail.com>
wrote:

> 2018-07-19 3:37 GMT+02:00, James Almer <jamrial@gmail.com>:
> > On 7/18/2018 3:57 PM, Aman Gupta wrote:
>
> >> +            ret = av_opt_set_from_string(s->bsfs[s->nb_bsfs -
> >> 1]->priv_data, bsf_options_str, shorthand, "=", ":");
> >> +            if (ret < 0) {
> >> +                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);
> >> +                av_freep(&bsf);
> >> +                ret = AVERROR_BUG;
> >> +                goto fail;
> >> +            }
> >
> > As i said on IRC, av_opt_set_from_string() can return ENOMEM which is
> > not a bug in the string contents, so do something like
> >
> > 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_assert(ret == AVERROR(ENOMEM)); ?
>

I think it's preferable to show the custom error message, since it will
include the name of the filter and also the name of the codec (via avctx),
and suggest to the user that they report the bug with both of those details.

Aman


>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
James Almer July 19, 2018, 7:03 p.m.
On 7/19/2018 3:44 PM, Carl Eugen Hoyos wrote:
> 2018-07-19 3:37 GMT+02:00, James Almer <jamrial@gmail.com>:
>> On 7/18/2018 3:57 PM, Aman Gupta wrote:
> 
>>> +            ret = av_opt_set_from_string(s->bsfs[s->nb_bsfs -
>>> 1]->priv_data, bsf_options_str, shorthand, "=", ":");
>>> +            if (ret < 0) {
>>> +                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);
>>> +                av_freep(&bsf);
>>> +                ret = AVERROR_BUG;
>>> +                goto fail;
>>> +            }
>>
>> As i said on IRC, av_opt_set_from_string() can return ENOMEM which is
>> not a bug in the string contents, so do something like
>>
>> 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_assert(ret == AVERROR(ENOMEM)); ?
> 
> Carl Eugen

This is more in line with the similar error above. Aborting gracefully
by printing a message and returning an error value instead of a hard
abort() with no explanation as to why "ret == AVERROR(ENOMEM)" was expected.
Carl Eugen Hoyos July 19, 2018, 7:16 p.m.
2018-07-19 21:00 GMT+02:00, Aman Gupta <ffmpeg@tmm1.net>:
> On Thu, Jul 19, 2018 at 11:44 AM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
>
>> 2018-07-19 3:37 GMT+02:00, James Almer <jamrial@gmail.com>:
>> > On 7/18/2018 3:57 PM, Aman Gupta wrote:
>>
>> >> +            ret = av_opt_set_from_string(s->bsfs[s->nb_bsfs -
>> >> 1]->priv_data, bsf_options_str, shorthand, "=", ":");
>> >> +            if (ret < 0) {
>> >> +                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);
>> >> +                av_freep(&bsf);
>> >> +                ret = AVERROR_BUG;
>> >> +                goto fail;
>> >> +            }
>> >
>> > As i said on IRC, av_opt_set_from_string() can return ENOMEM which is
>> > not a bug in the string contents, so do something like
>> >
>> > 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_assert(ret == AVERROR(ENOMEM)); ?
>>
>
> I think it's preferable to show the custom error message, since it will
> include the name of the filter and also the name of the codec (via avctx),
> and suggest to the user that they report the bug with both of those details.

Apart from the fact that I fear the message will not trigger the
necessary report (as opposed to the assert): Iirc, you can
convince av_assert to print all this information, no?

Carl Eugen
Nicolas George July 19, 2018, 7:31 p.m.
James Almer (2018-07-19):
> This is more in line with the similar error above. Aborting gracefully
> by printing a message and returning an error value instead of a hard
> abort() with no explanation as to why "ret == AVERROR(ENOMEM)" was expected.

Explaining to whom? If it is a consistency bug within lavc, notifying
the user is worthless, the correct thing to do is to prevent the
inconsistency from getting committed, hence assert and FATE test.

Regards,
James Almer July 19, 2018, 7:47 p.m.
On 7/19/2018 4:31 PM, Nicolas George wrote:
> James Almer (2018-07-19):
>> This is more in line with the similar error above. Aborting gracefully
>> by printing a message and returning an error value instead of a hard
>> abort() with no explanation as to why "ret == AVERROR(ENOMEM)" was expected.
> 
> Explaining to whom? If it is a consistency bug within lavc, notifying
> the user is worthless, the correct thing to do is to prevent the
> inconsistency from getting committed, hence assert and FATE test.
> 
> Regards,

AVOptions available in bitstream filters, or even the range of accepted
values, may depend on configure time options (external or internal
dependencies). A simple ./configure could enable all that's needed for
the option to be available as required by the decoder auto inserting the
bsf, but ./configure --disable-everything --enable-decoder=foo may not.
It may not enable the bsf (checked earlier in this same function and
also reported as a bug), or it may not enable an optional dependency
required to have the AVOption the decoder in question wants to use
available (checked by this new code).
See commit 5a366f9770. A fate test will not suffice to detect this kind
of issue.

I don't mind if we change it into a hard av_assert0(), but it will not
prevent inconsistencies from being committed any more than what was
committed. It will only give a less helpful error message to the user.
Carl Eugen Hoyos July 19, 2018, 9:16 p.m.
2018-07-19 21:47 GMT+02:00, James Almer <jamrial@gmail.com>:
> On 7/19/2018 4:31 PM, Nicolas George wrote:
>> James Almer (2018-07-19):
>>> This is more in line with the similar error above. Aborting gracefully
>>> by printing a message and returning an error value instead of a hard
>>> abort() with no explanation as to why "ret == AVERROR(ENOMEM)" was
>>> expected.
>>
>> Explaining to whom? If it is a consistency bug within lavc, notifying
>> the user is worthless, the correct thing to do is to prevent the
>> inconsistency from getting committed, hence assert and FATE test.
>>
>> Regards,
>
> AVOptions available in bitstream filters, or even the range of accepted
> values, may depend on configure time options (external or internal
> dependencies). A simple ./configure could enable all that's needed for
> the option to be available as required by the decoder auto inserting the
> bsf, but ./configure --disable-everything --enable-decoder=foo may not.

Then I believe BUG is not the correct error value as users are allowed
not to enable all features.

Carl Eugen
James Almer July 19, 2018, 9:45 p.m.
On 7/19/2018 6:16 PM, Carl Eugen Hoyos wrote:
> 2018-07-19 21:47 GMT+02:00, James Almer <jamrial@gmail.com>:
>> On 7/19/2018 4:31 PM, Nicolas George wrote:
>>> James Almer (2018-07-19):
>>>> This is more in line with the similar error above. Aborting gracefully
>>>> by printing a message and returning an error value instead of a hard
>>>> abort() with no explanation as to why "ret == AVERROR(ENOMEM)" was
>>>> expected.
>>>
>>> Explaining to whom? If it is a consistency bug within lavc, notifying
>>> the user is worthless, the correct thing to do is to prevent the
>>> inconsistency from getting committed, hence assert and FATE test.
>>>
>>> Regards,
>>
>> AVOptions available in bitstream filters, or even the range of accepted
>> values, may depend on configure time options (external or internal
>> dependencies). A simple ./configure could enable all that's needed for
>> the option to be available as required by the decoder auto inserting the
>> bsf, but ./configure --disable-everything --enable-decoder=foo may not.
> 
> Then I believe BUG is not the correct error value as users are allowed
> not to enable all features.
> 
> Carl Eugen

We're talking about dependencies pulled in by components. mlp_decoder
needs mlp_parser, so if the former is enabled then the latter will be
automatically enabled as well as per the configure rules. The user can't
forcefully disable the latter in that case as the decoder depends on it,
either to build or to run.
vp9_decoder needs vp9_superframe_split_bsf, and the same thing happens.
If you could disable the latter, the decoder would not work. The
earlier, older AVERROR_BUG in the function would be triggered in that
case, because it means there's a missing dependency in configure.

With AVOptions the same logic applies. If some bsf option needs a
certain module so a value may become available, and a decoder
autoinserts that bsf setting that value for the option in question, then
that module becomes a dependency for the decoder that needs to be set in
configure.

When normally a missing dependency results in build time failures
because some object wasn't compiled, in here it results in run time
failures because a call to function failed in a way it was not supposed to.

Patch hide | download patch | download mbox

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 6a3a4df179..67b7443b9d 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -36,6 +36,7 @@ 
 #include "libavutil/imgutils.h"
 #include "libavutil/internal.h"
 #include "libavutil/intmath.h"
+#include "libavutil/opt.h"
 
 #include "avcodec.h"
 #include "bytestream.h"
@@ -195,27 +196,33 @@  static int bsfs_init(AVCodecContext *avctx)
     while (bsfs_str && *bsfs_str) {
         AVBSFContext **tmp;
         const AVBitStreamFilter *filter;
-        char *bsf;
+        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);
+        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);
-            ret = AVERROR_BUG;
+                   bsf_name);
             av_freep(&bsf);
+            ret = AVERROR_BUG;
             goto fail;
         }
-        av_freep(&bsf);
 
         tmp = av_realloc_array(s->bsfs, s->nb_bsfs + 1, sizeof(*s->bsfs));
         if (!tmp) {
+            av_freep(&bsf);
             ret = AVERROR(ENOMEM);
             goto fail;
         }
@@ -223,8 +230,10 @@  static int bsfs_init(AVCodecContext *avctx)
         s->nb_bsfs++;
 
         ret = av_bsf_alloc(filter, &s->bsfs[s->nb_bsfs - 1]);
-        if (ret < 0)
+        if (ret < 0) {
+            av_freep(&bsf);
             goto fail;
+        }
 
         if (s->nb_bsfs == 1) {
             /* We do not currently have an API for passing the input timebase into decoders,
@@ -238,12 +247,36 @@  static int bsfs_init(AVCodecContext *avctx)
             ret = avcodec_parameters_copy(s->bsfs[s->nb_bsfs - 1]->par_in,
                                           s->bsfs[s->nb_bsfs - 2]->par_out);
         }
-        if (ret < 0)
+        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) {
+                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);
+                av_freep(&bsf);
+                ret = AVERROR_BUG;
+                goto fail;
+            }
+        }
+        av_freep(&bsf);
 
         ret = av_bsf_init(s->bsfs[s->nb_bsfs - 1]);
         if (ret < 0)
             goto fail;
+
+        if (*bsfs_str)
+            bsfs_str++;
     }
 
     return 0;