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

[Michael Niedermayer]

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


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200523/7448e04e/attachment.sig>