| Message ID | 20180718185724.10578-1-ffmpeg@tmm1.net |
|---|---|
| State | New |
| Headers | show |
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.
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 >
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
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 >
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.
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
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,
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.
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
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.
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;