diff mbox series

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

Message ID 20200813114126.27885-1-jack.haughton@broadcom.com
State New
Headers show
Series [FFmpeg-devel] avutil/opt: Restore NULL input handling to set_string_video_rate() | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jack Haughton Aug. 13, 2020, 11:41 a.m. UTC 
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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer Aug. 14, 2020, 6:09 p.m. UTC | #1 
On Thu, Aug 13, 2020 at 12:41:26PM +0100, Jack Haughton wrote:
> 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 | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

will apply

thx

[...]
Andreas Rheinhardt Aug. 14, 2020, 6:20 p.m. UTC | #2 
Michael Niedermayer:
> On Thu, Aug 13, 2020 at 12:41:26PM +0100, Jack Haughton wrote:
>> 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 | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> will apply
> 
> thx
> 
But is a NULL default value actually invalid/nonsense? I think not. See
https://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/267444.html

I can send a patch implementing this if others agree with my reasoning.

- Andreas
Michael Niedermayer Aug. 15, 2020, 10:07 a.m. UTC | #3 
On Fri, Aug 14, 2020 at 08:20:28PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Thu, Aug 13, 2020 at 12:41:26PM +0100, Jack Haughton wrote:
> >> 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 | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > will apply
> > 
> > thx
> > 
> But is a NULL default value actually invalid/nonsense? I think not. See
> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/267444.html

replied there

thx

[...]
diff mbox series

Patch

diff --git a/libavutil/opt.c b/libavutil/opt.c
index c8413fa5e1..50bf769cbd 100644
--- a/libavutil/opt.c
+++ b/libavutil/opt.c
@@ -333,7 +333,10 @@  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;
+
+    av_assert0(val);
+    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;