diff mbox series

[FFmpeg-devel] lavc/decode: do not use a context variable for function-local data

Message ID 20200522165642.8766-1-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel] lavc/decode: do not use a context variable for function-local data | expand

Checks

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

Commit Message

Anton Khirnov May 22, 2020, 4:56 p.m. UTC 
---
 libavcodec/decode.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

James Almer May 22, 2020, 5:32 p.m. UTC | #1 
On 5/22/2020 1:56 PM, Anton Khirnov wrote:
> ---
>  libavcodec/decode.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index f3327d74af..dbecdf3f46 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -586,6 +586,7 @@ static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
>  int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacket *avpkt)
>  {
>      AVCodecInternal *avci = avctx->internal;
> +    AVPacket buffer_pkt = { NULL };
>      int ret;
>  
>      if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
> @@ -597,16 +598,15 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
>      if (avpkt && !avpkt->size && avpkt->data)
>          return AVERROR(EINVAL);
>  
> -    av_packet_unref(avci->buffer_pkt);
>      if (avpkt && (avpkt->data || avpkt->side_data_elems)) {
> -        ret = av_packet_ref(avci->buffer_pkt, avpkt);
> +        ret = av_packet_ref(&buffer_pkt, avpkt);
>          if (ret < 0)
>              return ret;
>      }
>  
> -    ret = av_bsf_send_packet(avci->bsf, avci->buffer_pkt);
> +    ret = av_bsf_send_packet(avci->bsf, &buffer_pkt);
>      if (ret < 0) {
> -        av_packet_unref(avci->buffer_pkt);
> +        av_packet_unref(&buffer_pkt);
>          return ret;
>      }

What's the gain here? You're not removing the context variable since
it's used in the encode framework as well, and you're introducing a
stack AVPacket (that, while harmless, is not even properly initialized).
Michael Niedermayer May 22, 2020, 7:22 p.m. UTC | #2 
On Fri, May 22, 2020 at 02:32:07PM -0300, James Almer wrote:
> On 5/22/2020 1:56 PM, Anton Khirnov wrote:
> > ---
> >  libavcodec/decode.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index f3327d74af..dbecdf3f46 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -586,6 +586,7 @@ static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
> >  int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacket *avpkt)
> >  {
> >      AVCodecInternal *avci = avctx->internal;
> > +    AVPacket buffer_pkt = { NULL };
> >      int ret;
> >  
> >      if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
> > @@ -597,16 +598,15 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
> >      if (avpkt && !avpkt->size && avpkt->data)
> >          return AVERROR(EINVAL);
> >  
> > -    av_packet_unref(avci->buffer_pkt);
> >      if (avpkt && (avpkt->data || avpkt->side_data_elems)) {
> > -        ret = av_packet_ref(avci->buffer_pkt, avpkt);
> > +        ret = av_packet_ref(&buffer_pkt, avpkt);
> >          if (ret < 0)
> >              return ret;
> >      }
> >  
> > -    ret = av_bsf_send_packet(avci->bsf, avci->buffer_pkt);
> > +    ret = av_bsf_send_packet(avci->bsf, &buffer_pkt);
> >      if (ret < 0) {
> > -        av_packet_unref(avci->buffer_pkt);
> > +        av_packet_unref(&buffer_pkt);
> >          return ret;
> >      }
> 
> What's the gain here? You're not removing the context variable since
> it's used in the encode framework as well, and you're introducing a

> stack AVPacket (that, while harmless, is not even properly initialized).

It would be nice if we could avoid all such code, so that we can extend
AVPacket like other structs without Major bumping

thx

[...]
James Almer May 22, 2020, 7:51 p.m. UTC | #3 
On 5/22/2020 4:22 PM, Michael Niedermayer wrote:
> On Fri, May 22, 2020 at 02:32:07PM -0300, James Almer wrote:
>> On 5/22/2020 1:56 PM, Anton Khirnov wrote:
>>> ---
>>>  libavcodec/decode.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>>> index f3327d74af..dbecdf3f46 100644
>>> --- a/libavcodec/decode.c
>>> +++ b/libavcodec/decode.c
>>> @@ -586,6 +586,7 @@ static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
>>>  int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacket *avpkt)
>>>  {
>>>      AVCodecInternal *avci = avctx->internal;
>>> +    AVPacket buffer_pkt = { NULL };
>>>      int ret;
>>>  
>>>      if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
>>> @@ -597,16 +598,15 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
>>>      if (avpkt && !avpkt->size && avpkt->data)
>>>          return AVERROR(EINVAL);
>>>  
>>> -    av_packet_unref(avci->buffer_pkt);
>>>      if (avpkt && (avpkt->data || avpkt->side_data_elems)) {
>>> -        ret = av_packet_ref(avci->buffer_pkt, avpkt);
>>> +        ret = av_packet_ref(&buffer_pkt, avpkt);
>>>          if (ret < 0)
>>>              return ret;
>>>      }
>>>  
>>> -    ret = av_bsf_send_packet(avci->bsf, avci->buffer_pkt);
>>> +    ret = av_bsf_send_packet(avci->bsf, &buffer_pkt);
>>>      if (ret < 0) {
>>> -        av_packet_unref(avci->buffer_pkt);
>>> +        av_packet_unref(&buffer_pkt);
>>>          return ret;
>>>      }
>>
>> What's the gain here? You're not removing the context variable since
>> it's used in the encode framework as well, and you're introducing a
> 
>> stack AVPacket (that, while harmless, is not even properly initialized).
> 
> It would be nice if we could avoid all such code, so that we can extend
> AVPacket like other structs without Major bumping

Yes, some relatively recent commits went in the direction of removing as
much AVPacket on stack/heap usage as possible precisely to remove
sizeof(AVPacket) from the ABI, but at least within lavc it's not a problem.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
Anton Khirnov May 23, 2020, 8:57 a.m. UTC | #4 
Quoting James Almer (2020-05-22 19:32:07)
> On 5/22/2020 1:56 PM, Anton Khirnov wrote:
> > ---
> >  libavcodec/decode.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index f3327d74af..dbecdf3f46 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -586,6 +586,7 @@ static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
> >  int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacket *avpkt)
> >  {
> >      AVCodecInternal *avci = avctx->internal;
> > +    AVPacket buffer_pkt = { NULL };
> >      int ret;
> >  
> >      if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
> > @@ -597,16 +598,15 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
> >      if (avpkt && !avpkt->size && avpkt->data)
> >          return AVERROR(EINVAL);
> >  
> > -    av_packet_unref(avci->buffer_pkt);
> >      if (avpkt && (avpkt->data || avpkt->side_data_elems)) {
> > -        ret = av_packet_ref(avci->buffer_pkt, avpkt);
> > +        ret = av_packet_ref(&buffer_pkt, avpkt);
> >          if (ret < 0)
> >              return ret;
> >      }
> >  
> > -    ret = av_bsf_send_packet(avci->bsf, avci->buffer_pkt);
> > +    ret = av_bsf_send_packet(avci->bsf, &buffer_pkt);
> >      if (ret < 0) {
> > -        av_packet_unref(avci->buffer_pkt);
> > +        av_packet_unref(&buffer_pkt);
> >          return ret;
> >      }
> 
> What's the gain here? You're not removing the context variable since
> it's used in the encode framework as well, and you're introducing a
> stack AVPacket (that, while harmless, is not even properly initialized).

The gain is clarity/readability. A context variable suggests it is used
to some longer-term state. This change makes it clear that the packets
is only used within this function. Even the name is rather confusing,
since the packet does not get buffered there.
Michael Niedermayer May 23, 2020, 9:52 a.m. UTC | #5 
On Fri, May 22, 2020 at 04:51:05PM -0300, James Almer wrote:
> On 5/22/2020 4:22 PM, Michael Niedermayer wrote:
> > On Fri, May 22, 2020 at 02:32:07PM -0300, James Almer wrote:
> >> On 5/22/2020 1:56 PM, Anton Khirnov wrote:
> >>> ---
> >>>  libavcodec/decode.c | 8 ++++----
> >>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> >>> index f3327d74af..dbecdf3f46 100644
> >>> --- a/libavcodec/decode.c
> >>> +++ b/libavcodec/decode.c
> >>> @@ -586,6 +586,7 @@ static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
> >>>  int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacket *avpkt)
> >>>  {
> >>>      AVCodecInternal *avci = avctx->internal;
> >>> +    AVPacket buffer_pkt = { NULL };
> >>>      int ret;
> >>>  
> >>>      if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
> >>> @@ -597,16 +598,15 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
> >>>      if (avpkt && !avpkt->size && avpkt->data)
> >>>          return AVERROR(EINVAL);
> >>>  
> >>> -    av_packet_unref(avci->buffer_pkt);
> >>>      if (avpkt && (avpkt->data || avpkt->side_data_elems)) {
> >>> -        ret = av_packet_ref(avci->buffer_pkt, avpkt);
> >>> +        ret = av_packet_ref(&buffer_pkt, avpkt);
> >>>          if (ret < 0)
> >>>              return ret;
> >>>      }
> >>>  
> >>> -    ret = av_bsf_send_packet(avci->bsf, avci->buffer_pkt);
> >>> +    ret = av_bsf_send_packet(avci->bsf, &buffer_pkt);
> >>>      if (ret < 0) {
> >>> -        av_packet_unref(avci->buffer_pkt);
> >>> +        av_packet_unref(&buffer_pkt);
> >>>          return ret;
> >>>      }
> >>
> >> What's the gain here? You're not removing the context variable since
> >> it's used in the encode framework as well, and you're introducing a
> > 
> >> stack AVPacket (that, while harmless, is not even properly initialized).
> > 
> > It would be nice if we could avoid all such code, so that we can extend
> > AVPacket like other structs without Major bumping
> 
> Yes, some relatively recent commits went in the direction of removing as
> much AVPacket on stack/heap usage as possible precisely to remove
> sizeof(AVPacket) from the ABI, but at least within lavc it's not a problem.

sizeof() may be ok in libavcodec (though still IMHO not great style)
the other issue is NULL initialization. We may want to add new fields
which need to be initialized to a non 0 default. timestamps and AV_NOPTS_VALUE
come to mind as examples of such fields

thx


[...]
diff mbox series

Patch

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index f3327d74af..dbecdf3f46 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -586,6 +586,7 @@  static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
 int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacket *avpkt)
 {
     AVCodecInternal *avci = avctx->internal;
+    AVPacket buffer_pkt = { NULL };
     int ret;
 
     if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec))
@@ -597,16 +598,15 @@  int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
     if (avpkt && !avpkt->size && avpkt->data)
         return AVERROR(EINVAL);
 
-    av_packet_unref(avci->buffer_pkt);
     if (avpkt && (avpkt->data || avpkt->side_data_elems)) {
-        ret = av_packet_ref(avci->buffer_pkt, avpkt);
+        ret = av_packet_ref(&buffer_pkt, avpkt);
         if (ret < 0)
             return ret;
     }
 
-    ret = av_bsf_send_packet(avci->bsf, avci->buffer_pkt);
+    ret = av_bsf_send_packet(avci->bsf, &buffer_pkt);
     if (ret < 0) {
-        av_packet_unref(avci->buffer_pkt);
+        av_packet_unref(&buffer_pkt);
         return ret;
     }