[FFmpeg-devel] [PATCH] ffmpeg: add verbose consistency checks in case of filtering inconsistent options

[Nicolas George]

Le tridi 13 brumaire, an CCXXII, Stefano Sabatini a écrit :
> In particular, warn in case -filter and streamcopy is used at the same
> time, fix trac ticket #678.
> ---
>  ffmpeg_opt.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> index c9283d6..12bfb34 100644
> --- a/ffmpeg_opt.c
> +++ b/ffmpeg_opt.c
> @@ -1171,6 +1171,26 @@ static char *get_ost_filters(OptionsContext *o, AVFormatContext *oc,
>                       "null" : "anull");
>  }
>  
> +#define CHECK_STREAMCOPY_FILTERS(ost, type) do {                        \
> +    if (ost->stream_copy) {                                             \
> +        char *filter_script = NULL, *filter = NULL;                     \
> +        MATCH_PER_STREAM_OPT(filter_scripts, str, filter_script, oc, st); \
> +        MATCH_PER_STREAM_OPT(filters,        str, filter,        oc, st); \
> +                                                                        \
> +        if (filter_script || filter) {                                  \
> +            av_log(NULL, AV_LOG_ERROR,                                  \
> +                   "Filter '%s' or filter_script '%s' was defined for %s output stream " \
> +                   "%d:%d but codec copy was selected.\n"               \
> +                   "Filtering and streamcopy cannot be used together.", \
> +                   (char *)av_x_if_null(filter, "(none)"),              \
> +                   (char *)av_x_if_null(filter_script, "(none)"),       \
> +                   av_get_media_type_string(AVMEDIA_TYPE_##type),       \
> +                   ost->file_index, ost->index);                        \
> +            exit_program(1);                                            \
> +        }                                                               \
> +    }                                                                   \
> +} while (0)
> +
>  static OutputStream *new_video_stream(OptionsContext *o, AVFormatContext *oc, int source_index)
>  {
>      AVStream *st;
> @@ -1311,6 +1331,7 @@ static OutputStream *new_video_stream(OptionsContext *o, AVFormatContext *oc, in
>          MATCH_PER_STREAM_OPT(copy_initial_nonkeyframes, i, ost->copy_initial_nonkeyframes, oc ,st);
>      }
>  
> +    CHECK_STREAMCOPY_FILTERS(ost, VIDEO);
>      return ost;
>  }
>  
> @@ -1364,6 +1385,7 @@ static OutputStream *new_audio_stream(OptionsContext *o, AVFormatContext *oc, in
>          }
>      }
>  
> +    CHECK_STREAMCOPY_FILTERS(ost, AUDIO);
>      return ost;
>  }

I suspect this whole block can be made simpler by moving the check in
get_ost_filters():

	if ((filter || filter_script) && ost->stream_copy) {
	    error
	}

>  
> @@ -1548,6 +1570,21 @@ static void init_output_filter(OutputFilter *ofilter, OptionsContext *o,
>          exit_program(1);
>      }
>  
> +    if (ost->avfilter) {
> +        char *filter_script = NULL, *filter = NULL;
> +        MATCH_PER_STREAM_OPT(filter_scripts, str, filter_script, oc, ost->st);
> +        MATCH_PER_STREAM_OPT(filters, str, filter, oc, ost->st);
> +
> +        av_log(NULL, AV_LOG_ERROR,
> +               "Filter graph '%s' or filter script '%s' was specified through the -filter/-filter_script/-vf/-af option "
> +               "for output stream %d:%d, which is fed from a complex filtergraph.\n"
> +               "-filter/-filter_script and -filter_complex cannot be used together.\n",
> +               (char *)av_x_if_null(filter, "(none)"),
> +               (char *)av_x_if_null(filter_script, "(none)"),
> +               ost->file_index, ost->index);
> +        exit_program(1);
> +    }
> +

If I read this code correctly, you are re-doing the matching just to print
the filter in the error message. IMHO, you could just print the number of
output stream, and possibly the name of the complex filter output, and leave
out the filter graph, especially since filter options can be quite long and
make the error message unreadable, possibly truncated.

If you really want to print the filter graph, I believe re-matching the
options is to be avoided: this is a recipe that usually leads to desynced
code. The cleanest way would probably be to move the local variables
"filter" and "filter_script" of get_ost_filters() into the OutputStream
structure.

Also, since you are adding a lot of details anyway, you can customize the
message more:

    "Filter %s '%s' was specified through the %s option",
    filters ? "graph" : "script",
    filters ? filters : filter_script,
    filters ? "-filter/-vf/-af" : "-filter_script",

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131103/dd44ac08/attachment.asc>