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

[Michael Niedermayer]

On Thu, Aug 06, 2020 at 02:43:08PM +0200, Andreas Rheinhardt wrote:
> 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?

I think the handling of "not set" should be consistent between types
and should follow a few properties
For example Value <-> String should maintain that "not set"

for Rationals, 0/0 is the natural mathetmatical way of saying not set.

There is also the aspect that we may not want to allow every AVOption
to be "not set"

For integers a valid value has to be used for a not set form and that
can then easily be bounded by the min/max so as to specify it as (not)allowed
but for oters based on AVRational or floats the support for NaN is not so
natural to specify.
So we should probably add a AV_OPT_FLAG_ALLOW_NAN or equivalent

[...]

thx
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200815/b5819e45/attachment.sig>