[FFmpeg-devel,4/6] avcodec/trace_headers: move the reference in the bsf internal buffer

Submitted by James Almer on March 11, 2018, 5:58 p.m.

Details

Message ID 20180311175854.10096-4-jamrial@gmail.com
State Accepted
Headers show

Commit Message

James Almer March 11, 2018, 5:58 p.m.
There's no need to allocate a new packet for it.

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

Comments

Mark Thompson March 11, 2018, 6:32 p.m.
On 11/03/18 17:58, James Almer wrote:
> There's no need to allocate a new packet for it.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/trace_headers_bsf.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/trace_headers_bsf.c b/libavcodec/trace_headers_bsf.c
> index 93d04cb509..d59bc828a9 100644
> --- a/libavcodec/trace_headers_bsf.c
> +++ b/libavcodec/trace_headers_bsf.c
> @@ -71,41 +71,39 @@ static int trace_headers(AVBSFContext *bsf, AVPacket *out)
>  {
>      TraceHeadersContext *ctx = bsf->priv_data;
>      CodedBitstreamFragment au;
> -    AVPacket *in;
>      char tmp[256] = { 0 };
>      int err;
>  
> -    err = ff_bsf_get_packet(bsf, &in);
> +    err = ff_bsf_get_packet_ref(bsf, out);
>      if (err < 0)
>          return err;
>  
> -    if (in->flags & AV_PKT_FLAG_KEY)
> +    if (out->flags & AV_PKT_FLAG_KEY)
>          av_strlcat(tmp, ", key frame", sizeof(tmp));
> -    if (in->flags & AV_PKT_FLAG_CORRUPT)
> +    if (out->flags & AV_PKT_FLAG_CORRUPT)
>          av_strlcat(tmp, ", corrupt", sizeof(tmp));
>  
> -    if (in->pts != AV_NOPTS_VALUE)
> -        av_strlcatf(tmp, sizeof(tmp), ", pts %"PRId64, in->pts);
> +    if (out->pts != AV_NOPTS_VALUE)
> +        av_strlcatf(tmp, sizeof(tmp), ", pts %"PRId64, out->pts);
>      else
>          av_strlcat(tmp, ", no pts", sizeof(tmp));
> -    if (in->dts != AV_NOPTS_VALUE)
> -        av_strlcatf(tmp, sizeof(tmp), ", dts %"PRId64, in->dts);
> +    if (out->dts != AV_NOPTS_VALUE)
> +        av_strlcatf(tmp, sizeof(tmp), ", dts %"PRId64, out->dts);
>      else
>          av_strlcat(tmp, ", no dts", sizeof(tmp));
> -    if (in->duration > 0)
> -        av_strlcatf(tmp, sizeof(tmp), ", duration %"PRId64, in->duration);
> +    if (out->duration > 0)
> +        av_strlcatf(tmp, sizeof(tmp), ", duration %"PRId64, out->duration);
>  
> -    av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", in->size, tmp);
> +    av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", out->size, tmp);
>  
> -    err = ff_cbs_read_packet(ctx->cbc, &au, in);
> -    if (err < 0)
> +    err = ff_cbs_read_packet(ctx->cbc, &au, out);
> +    if (err < 0) {
> +        av_packet_unref(out);
>          return err;
> +    }
>  
>      ff_cbs_fragment_uninit(ctx->cbc, &au);
>  
> -    av_packet_move_ref(out, in);
> -    av_packet_free(&in);
> -
>      return 0;
>  }
>  
> 

Rename the packet to something like "pkt" throughout?  The "out" name looks kindof weird after this change.

(That could probably apply to all of the patches, but this one makes the most use of the packet.)

Whole series looks fine to me.

- Mark
James Almer March 11, 2018, 6:50 p.m.
On 3/11/2018 3:32 PM, Mark Thompson wrote:
> On 11/03/18 17:58, James Almer wrote:
>> There's no need to allocate a new packet for it.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/trace_headers_bsf.c | 30 ++++++++++++++----------------
>>  1 file changed, 14 insertions(+), 16 deletions(-)
>>
>> diff --git a/libavcodec/trace_headers_bsf.c b/libavcodec/trace_headers_bsf.c
>> index 93d04cb509..d59bc828a9 100644
>> --- a/libavcodec/trace_headers_bsf.c
>> +++ b/libavcodec/trace_headers_bsf.c
>> @@ -71,41 +71,39 @@ static int trace_headers(AVBSFContext *bsf, AVPacket *out)
>>  {
>>      TraceHeadersContext *ctx = bsf->priv_data;
>>      CodedBitstreamFragment au;
>> -    AVPacket *in;
>>      char tmp[256] = { 0 };
>>      int err;
>>  
>> -    err = ff_bsf_get_packet(bsf, &in);
>> +    err = ff_bsf_get_packet_ref(bsf, out);
>>      if (err < 0)
>>          return err;
>>  
>> -    if (in->flags & AV_PKT_FLAG_KEY)
>> +    if (out->flags & AV_PKT_FLAG_KEY)
>>          av_strlcat(tmp, ", key frame", sizeof(tmp));
>> -    if (in->flags & AV_PKT_FLAG_CORRUPT)
>> +    if (out->flags & AV_PKT_FLAG_CORRUPT)
>>          av_strlcat(tmp, ", corrupt", sizeof(tmp));
>>  
>> -    if (in->pts != AV_NOPTS_VALUE)
>> -        av_strlcatf(tmp, sizeof(tmp), ", pts %"PRId64, in->pts);
>> +    if (out->pts != AV_NOPTS_VALUE)
>> +        av_strlcatf(tmp, sizeof(tmp), ", pts %"PRId64, out->pts);
>>      else
>>          av_strlcat(tmp, ", no pts", sizeof(tmp));
>> -    if (in->dts != AV_NOPTS_VALUE)
>> -        av_strlcatf(tmp, sizeof(tmp), ", dts %"PRId64, in->dts);
>> +    if (out->dts != AV_NOPTS_VALUE)
>> +        av_strlcatf(tmp, sizeof(tmp), ", dts %"PRId64, out->dts);
>>      else
>>          av_strlcat(tmp, ", no dts", sizeof(tmp));
>> -    if (in->duration > 0)
>> -        av_strlcatf(tmp, sizeof(tmp), ", duration %"PRId64, in->duration);
>> +    if (out->duration > 0)
>> +        av_strlcatf(tmp, sizeof(tmp), ", duration %"PRId64, out->duration);
>>  
>> -    av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", in->size, tmp);
>> +    av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", out->size, tmp);
>>  
>> -    err = ff_cbs_read_packet(ctx->cbc, &au, in);
>> -    if (err < 0)
>> +    err = ff_cbs_read_packet(ctx->cbc, &au, out);
>> +    if (err < 0) {
>> +        av_packet_unref(out);
>>          return err;
>> +    }
>>  
>>      ff_cbs_fragment_uninit(ctx->cbc, &au);
>>  
>> -    av_packet_move_ref(out, in);
>> -    av_packet_free(&in);
>> -
>>      return 0;
>>  }
>>  
>>
> 
> Rename the packet to something like "pkt" throughout?  The "out" name looks kindof weird after this change.
> 
> (That could probably apply to all of the patches, but this one makes the most use of the packet.)
> 
> Whole series looks fine to me.

Changed in all patches and pushed. Thanks!

> 
> - Mark

Patch hide | download patch | download mbox

diff --git a/libavcodec/trace_headers_bsf.c b/libavcodec/trace_headers_bsf.c
index 93d04cb509..d59bc828a9 100644
--- a/libavcodec/trace_headers_bsf.c
+++ b/libavcodec/trace_headers_bsf.c
@@ -71,41 +71,39 @@  static int trace_headers(AVBSFContext *bsf, AVPacket *out)
 {
     TraceHeadersContext *ctx = bsf->priv_data;
     CodedBitstreamFragment au;
-    AVPacket *in;
     char tmp[256] = { 0 };
     int err;
 
-    err = ff_bsf_get_packet(bsf, &in);
+    err = ff_bsf_get_packet_ref(bsf, out);
     if (err < 0)
         return err;
 
-    if (in->flags & AV_PKT_FLAG_KEY)
+    if (out->flags & AV_PKT_FLAG_KEY)
         av_strlcat(tmp, ", key frame", sizeof(tmp));
-    if (in->flags & AV_PKT_FLAG_CORRUPT)
+    if (out->flags & AV_PKT_FLAG_CORRUPT)
         av_strlcat(tmp, ", corrupt", sizeof(tmp));
 
-    if (in->pts != AV_NOPTS_VALUE)
-        av_strlcatf(tmp, sizeof(tmp), ", pts %"PRId64, in->pts);
+    if (out->pts != AV_NOPTS_VALUE)
+        av_strlcatf(tmp, sizeof(tmp), ", pts %"PRId64, out->pts);
     else
         av_strlcat(tmp, ", no pts", sizeof(tmp));
-    if (in->dts != AV_NOPTS_VALUE)
-        av_strlcatf(tmp, sizeof(tmp), ", dts %"PRId64, in->dts);
+    if (out->dts != AV_NOPTS_VALUE)
+        av_strlcatf(tmp, sizeof(tmp), ", dts %"PRId64, out->dts);
     else
         av_strlcat(tmp, ", no dts", sizeof(tmp));
-    if (in->duration > 0)
-        av_strlcatf(tmp, sizeof(tmp), ", duration %"PRId64, in->duration);
+    if (out->duration > 0)
+        av_strlcatf(tmp, sizeof(tmp), ", duration %"PRId64, out->duration);
 
-    av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", in->size, tmp);
+    av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", out->size, tmp);
 
-    err = ff_cbs_read_packet(ctx->cbc, &au, in);
-    if (err < 0)
+    err = ff_cbs_read_packet(ctx->cbc, &au, out);
+    if (err < 0) {
+        av_packet_unref(out);
         return err;
+    }
 
     ff_cbs_fragment_uninit(ctx->cbc, &au);
 
-    av_packet_move_ref(out, in);
-    av_packet_free(&in);
-
     return 0;
 }