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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".