[FFmpeg-devel] [PATCH] avutil/opt: Restore NULL input handling to set_string_video_rate()

[Andreas Rheinhardt]

Jack Haughton:
> Commit a500b975 removed NULL input handling from this function,
> moving the check higher up the call tree in one branch. However,
> there is another call to set_string_video_rate() which may pass
> NULL, and future users of the function may not be clear that
> a NULL check is required. This patch restores the NULL check to
> avoid these problems.
> ---
>  libavutil/opt.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index c8413fa5e1..9f36dc89a9 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -333,7 +333,13 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val,
>  
>  static int set_string_video_rate(void *obj, const AVOption *o, const char *val, AVRational *dst)
>  {
> -    int ret = av_parse_video_rate(dst, val);
> +    int ret;
> +
> +    if (!val) {
> +        av_log(obj, AV_LOG_ERROR, "NULL passed as video rate\n");
> +        return AVERROR(EINVAL);
> +    }
> +    ret = av_parse_video_rate(dst, val);
>      if (ret < 0)
>          av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as video rate\n", val);
>      return ret;
> 
I pondered this other caller back then [1] and thought that a NULL
default value for a framerate was illegal and that therefore the
function should fail hard in such a case to force the programmer to fix
the code. But upon rereading the doc I couldn't find any documentation
that actually said that a default value of NULL is illegal. I have
probably misunderstood the "offset must point to AVRational" comment in
opt.h (which only says how the value will be stored and says nothing
about the default value)).

There is even a usecase for allowing a NULL as default value: When one
wants to know whether the user has set a value or not. (Right now this
is not possible (at least not without a second option that says that the
other framerate option is to be used), because av_parse_video_rate()
does not allow illegal values (numerator or denumerator <= 0) for the
framerate.) So a possible usecase could be as follows: If a file
contains a framerate, but if one wants to allow the user to override the
framerate, then not using a default value is natural.

Of course in this case set_string_video_rate() should not be called at
all; instead the case should be handled in av_opt_set_default2() which
should set the value explicitly to (AVRational){ 0, 0 } in this case.

What do others think about this?

- Andreas

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/259250.html