Re: [FFmpeg-devel] [PATCH 3/5] trace_headers: Fix memory leaks on syntax read failures

2018-10-06 Thread Mark Thompson 
On 05/10/18 00:39, James Almer wrote:
> On 10/4/2018 8:09 PM, Mark Thompson wrote:
>> ---
>>  libavcodec/trace_headers_bsf.c | 14 --
>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/libavcodec/trace_headers_bsf.c b/libavcodec/trace_headers_bsf.c
>> index 94a3ef72a2..839d4c 100644
>> --- a/libavcodec/trace_headers_bsf.c
>> +++ b/libavcodec/trace_headers_bsf.c
>> @@ -49,15 +49,11 @@ static int trace_headers_init(AVBSFContext *bsf)
>>  av_log(bsf, AV_LOG_INFO, "Extradata\n");
>>  
>>  err = ff_cbs_read_extradata(ctx->cbc, , bsf->par_in);
>> -if (err < 0) {
>> -av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n");
> 
> Why remove this error message?

I was thinking it's not adding anything useful - there will already be an error 
from the reading describing the actual problem.  (Since it makes the BSF init 
fail there can't be any more from the filter after that point.)

>> -return err;
>> -}
>>  
>>  ff_cbs_fragment_uninit(ctx->cbc, );
>>  }
>>  
>> -return 0;
>> +return err;
>>  }
>>  
>>  static void trace_headers_close(AVBSFContext *bsf)
>> @@ -97,14 +93,12 @@ static int trace_headers(AVBSFContext *bsf, AVPacket 
>> *pkt)
>>  av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", pkt->size, tmp);
>>  
>>  err = ff_cbs_read_packet(ctx->cbc, , pkt);
>> -if (err < 0) {
>> -av_packet_unref(pkt);
>> -return err;
>> -}
>>  
>>  ff_cbs_fragment_uninit(ctx->cbc, );
> 
> Maybe the ff_cbs_read* functions should clean whatever they were able to
> allocate before the failure by calling this function internally, instead
> of leaving it to the caller. It would be more consistent with what other
> APIs we offer do.

The reasoning for making the API this way was that partial reads are not 
useless - it might have successfully read a whole sequence of NAL units, then 
fail on some header error on the last one.

(I admit no current caller makes use of this.)

>>  
>> -return 0;
>> +if (err < 0)
>> +av_packet_unref(pkt);
>> +return err;
>>  }
>>  
>>  const AVBitStreamFilter ff_trace_headers_bsf = {
> 
> LGTM in any case.

Thanks,

- Mark
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/5] trace_headers: Fix memory leaks on syntax read failures

2018-10-04 Thread James Almer 
On 10/4/2018 8:09 PM, Mark Thompson wrote:
> ---
>  libavcodec/trace_headers_bsf.c | 14 --
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/trace_headers_bsf.c b/libavcodec/trace_headers_bsf.c
> index 94a3ef72a2..839d4c 100644
> --- a/libavcodec/trace_headers_bsf.c
> +++ b/libavcodec/trace_headers_bsf.c
> @@ -49,15 +49,11 @@ static int trace_headers_init(AVBSFContext *bsf)
>  av_log(bsf, AV_LOG_INFO, "Extradata\n");
>  
>  err = ff_cbs_read_extradata(ctx->cbc, , bsf->par_in);
> -if (err < 0) {
> -av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n");

Why remove this error message?

> -return err;
> -}
>  
>  ff_cbs_fragment_uninit(ctx->cbc, );
>  }
>  
> -return 0;
> +return err;
>  }
>  
>  static void trace_headers_close(AVBSFContext *bsf)
> @@ -97,14 +93,12 @@ static int trace_headers(AVBSFContext *bsf, AVPacket *pkt)
>  av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", pkt->size, tmp);
>  
>  err = ff_cbs_read_packet(ctx->cbc, , pkt);
> -if (err < 0) {
> -av_packet_unref(pkt);
> -return err;
> -}
>  
>  ff_cbs_fragment_uninit(ctx->cbc, );

Maybe the ff_cbs_read* functions should clean whatever they were able to
allocate before the failure by calling this function internally, instead
of leaving it to the caller. It would be more consistent with what other
APIs we offer do.

>  
> -return 0;
> +if (err < 0)
> +av_packet_unref(pkt);
> +return err;
>  }
>  
>  const AVBitStreamFilter ff_trace_headers_bsf = {

LGTM in any case.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 3/5] trace_headers: Fix memory leaks on syntax read failures

2018-10-04 Thread Mark Thompson 
---
 libavcodec/trace_headers_bsf.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/libavcodec/trace_headers_bsf.c b/libavcodec/trace_headers_bsf.c
index 94a3ef72a2..839d4c 100644
--- a/libavcodec/trace_headers_bsf.c
+++ b/libavcodec/trace_headers_bsf.c
@@ -49,15 +49,11 @@ static int trace_headers_init(AVBSFContext *bsf)
 av_log(bsf, AV_LOG_INFO, "Extradata\n");
 
 err = ff_cbs_read_extradata(ctx->cbc, , bsf->par_in);
-if (err < 0) {
-av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n");
-return err;
-}
 
 ff_cbs_fragment_uninit(ctx->cbc, );
 }
 
-return 0;
+return err;
 }
 
 static void trace_headers_close(AVBSFContext *bsf)
@@ -97,14 +93,12 @@ static int trace_headers(AVBSFContext *bsf, AVPacket *pkt)
 av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", pkt->size, tmp);
 
 err = ff_cbs_read_packet(ctx->cbc, , pkt);
-if (err < 0) {
-av_packet_unref(pkt);
-return err;
-}
 
 ff_cbs_fragment_uninit(ctx->cbc, );
 
-return 0;
+if (err < 0)
+av_packet_unref(pkt);
+return err;
 }
 
 const AVBitStreamFilter ff_trace_headers_bsf = {
-- 
2.18.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel