[FFmpeg-devel] avcodec/encode: don't return immediately on failure

Submitted by James Almer on Oct. 3, 2017, 4:55 a.m.

Details

Message ID 20171003045544.6632-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Oct. 3, 2017, 4:55 a.m.
Fixes memleaks introduced by skipping cleanup at the end of the
functions.

Regression since a22c6a4796ca1f2cbee6784262515da876fbec22.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/encode.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

wm4 Oct. 3, 2017, 8:23 a.m.
On Tue,  3 Oct 2017 01:55:44 -0300
James Almer <jamrial@gmail.com> wrote:

> Fixes memleaks introduced by skipping cleanup at the end of the
> functions.
> 
> Regression since a22c6a4796ca1f2cbee6784262515da876fbec22.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/encode.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/libavcodec/encode.c b/libavcodec/encode.c
> index c152228c92..841a185738 100644
> --- a/libavcodec/encode.c
> +++ b/libavcodec/encode.c
> @@ -225,10 +225,10 @@ int attribute_align_arg avcodec_encode_audio2(AVCodecContext *avctx,
>          } else if (!avpkt->buf) {
>              AVPacket tmp = { 0 };
>              ret = av_packet_ref(&tmp, avpkt);
> -            if (ret < 0)
> -                return ret;
> -            av_packet_unref(avpkt);
> -            *avpkt = tmp;
> +            if (!ret) {
> +                av_packet_unref(avpkt);
> +                *avpkt = tmp;
> +            }
>          }
>      }
>  
> @@ -324,10 +324,10 @@ int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx,
>          } else if (!avpkt->buf) {
>              AVPacket tmp = { 0 };
>              ret = av_packet_ref(&tmp, avpkt);
> -            if (ret < 0)
> -                return ret;
> -            av_packet_unref(avpkt);
> -            *avpkt = tmp;
> +            if (!ret) {
> +                av_packet_unref(avpkt);
> +                *avpkt = tmp;
> +            }
>          }
>      }
>  

Seems ok if ret is actually checked in the code below. Something like
"goto cleanup;" on error would be less confusing though.

Also why did you essentially change "!(ret<0)" to "!ret"? In theory, all
positive return values indicate success.
James Almer Oct. 3, 2017, 5:04 p.m.
On 10/3/2017 5:23 AM, wm4 wrote:
> On Tue,  3 Oct 2017 01:55:44 -0300
> James Almer <jamrial@gmail.com> wrote:
> 
>> Fixes memleaks introduced by skipping cleanup at the end of the
>> functions.
>>
>> Regression since a22c6a4796ca1f2cbee6784262515da876fbec22.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/encode.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/libavcodec/encode.c b/libavcodec/encode.c
>> index c152228c92..841a185738 100644
>> --- a/libavcodec/encode.c
>> +++ b/libavcodec/encode.c
>> @@ -225,10 +225,10 @@ int attribute_align_arg avcodec_encode_audio2(AVCodecContext *avctx,
>>          } else if (!avpkt->buf) {
>>              AVPacket tmp = { 0 };
>>              ret = av_packet_ref(&tmp, avpkt);
>> -            if (ret < 0)
>> -                return ret;
>> -            av_packet_unref(avpkt);
>> -            *avpkt = tmp;
>> +            if (!ret) {
>> +                av_packet_unref(avpkt);
>> +                *avpkt = tmp;
>> +            }
>>          }
>>      }
>>  
>> @@ -324,10 +324,10 @@ int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx,
>>          } else if (!avpkt->buf) {
>>              AVPacket tmp = { 0 };
>>              ret = av_packet_ref(&tmp, avpkt);
>> -            if (ret < 0)
>> -                return ret;
>> -            av_packet_unref(avpkt);
>> -            *avpkt = tmp;
>> +            if (!ret) {
>> +                av_packet_unref(avpkt);
>> +                *avpkt = tmp;
>> +            }
>>          }
>>      }
>>  
> 
> Seems ok if ret is actually checked in the code below. Something like
> "goto cleanup;" on error would be less confusing though.

Did that instead and pushed. Thanks.

Patch hide | download patch | download mbox

diff --git a/libavcodec/encode.c b/libavcodec/encode.c
index c152228c92..841a185738 100644
--- a/libavcodec/encode.c
+++ b/libavcodec/encode.c
@@ -225,10 +225,10 @@  int attribute_align_arg avcodec_encode_audio2(AVCodecContext *avctx,
         } else if (!avpkt->buf) {
             AVPacket tmp = { 0 };
             ret = av_packet_ref(&tmp, avpkt);
-            if (ret < 0)
-                return ret;
-            av_packet_unref(avpkt);
-            *avpkt = tmp;
+            if (!ret) {
+                av_packet_unref(avpkt);
+                *avpkt = tmp;
+            }
         }
     }
 
@@ -324,10 +324,10 @@  int attribute_align_arg avcodec_encode_video2(AVCodecContext *avctx,
         } else if (!avpkt->buf) {
             AVPacket tmp = { 0 };
             ret = av_packet_ref(&tmp, avpkt);
-            if (ret < 0)
-                return ret;
-            av_packet_unref(avpkt);
-            *avpkt = tmp;
+            if (!ret) {
+                av_packet_unref(avpkt);
+                *avpkt = tmp;
+            }
         }
     }