diff mbox series

[FFmpeg-devel,8/9] avfilter/vf_subtitles: Return early upon avformat_open_input() failure

Message ID 20200910214901.25401-8-andreas.rheinhardt@gmail.com
State Withdrawn
Headers show
Series [FFmpeg-devel,1/9] avfilter/lavfutils: Don't use uninitialized pointers for freeing
Related show

Checks

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

Commit Message

Andreas Rheinhardt Sept. 10, 2020, 9:49 p.m. UTC
There is nothing to free at this point.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavfilter/vf_subtitles.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paul B Mahol Sept. 11, 2020, 9:18 a.m. UTC | #1
On Thu, Sep 10, 2020 at 11:49:00PM +0200, Andreas Rheinhardt wrote:
> There is nothing to free at this point.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavfilter/vf_subtitles.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

probably ok

> 
> diff --git a/libavfilter/vf_subtitles.c b/libavfilter/vf_subtitles.c
> index 08b4d4efca..f9733c3935 100644
> --- a/libavfilter/vf_subtitles.c
> +++ b/libavfilter/vf_subtitles.c
> @@ -322,7 +322,7 @@ static av_cold int init_subtitles(AVFilterContext *ctx)
>      ret = avformat_open_input(&fmt, ass->filename, NULL, NULL);
>      if (ret < 0) {
>          av_log(ctx, AV_LOG_ERROR, "Unable to open %s\n", ass->filename);
> -        goto end;
> +        return ret;
>      }
>      ret = avformat_find_stream_info(fmt, NULL);
>      if (ret < 0)
> -- 
> 2.20.1
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George Sept. 11, 2020, 9:19 a.m. UTC | #2
Andreas Rheinhardt (12020-09-10):
> There is nothing to free at this point.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavfilter/vf_subtitles.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Does it help anything? If something gets allocated earlier, we will need
to add it back. When there is a comprehensive cleanup label, using it
always seems more robust than guessing if something needs freeing or
not.

Regards,
Andreas Rheinhardt Sept. 11, 2020, 10:20 a.m. UTC | #3
Nicolas George:
> Andreas Rheinhardt (12020-09-10):
>> There is nothing to free at this point.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavfilter/vf_subtitles.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Does it help anything? If something gets allocated earlier, we will need
> to add it back. When there is a comprehensive cleanup label, using it
> always seems more robust than guessing if something needs freeing or
> not.
> 
> Regards,
> 
It avoids cleaning up three inexistent objects; I actually regarded this
as enough benefit to merit the change.

- Andreas
Nicolas George Sept. 11, 2020, 10:38 a.m. UTC | #4
Andreas Rheinhardt (12020-09-11):
> It avoids cleaning up three inexistent objects; I actually regarded this
> as enough benefit to merit the change.

Cleaning-up inexistent objects is a negligible cost. In this case, it
happens in a part of the code that is particularly not speed-critical. I
think the robustness of the code takes precedence here.

Regards,
diff mbox series

Patch

diff --git a/libavfilter/vf_subtitles.c b/libavfilter/vf_subtitles.c
index 08b4d4efca..f9733c3935 100644
--- a/libavfilter/vf_subtitles.c
+++ b/libavfilter/vf_subtitles.c
@@ -322,7 +322,7 @@  static av_cold int init_subtitles(AVFilterContext *ctx)
     ret = avformat_open_input(&fmt, ass->filename, NULL, NULL);
     if (ret < 0) {
         av_log(ctx, AV_LOG_ERROR, "Unable to open %s\n", ass->filename);
-        goto end;
+        return ret;
     }
     ret = avformat_find_stream_info(fmt, NULL);
     if (ret < 0)