[FFmpeg-devel] [PATCH 06/50] avcodec/mpegvideo_enc: use av_packet_alloc() to allocate packets

[Anton Khirnov]

Quoting Andreas Rheinhardt (2021-02-08 15:50:48)
> James Almer:
> > On 2/8/2021 11:46 AM, Andreas Rheinhardt wrote:
> >> James Almer:
> >>> Signed-off-by: James Almer <jamrial at gmail.com>
> >>> ---
> >>>   libavcodec/mpegvideo_enc.c | 23 +++++++++++++----------
> >>>   1 file changed, 13 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c
> >>> index 34dcf8c313..411cadeae7 100644
> >>> --- a/libavcodec/mpegvideo_enc.c
> >>> +++ b/libavcodec/mpegvideo_enc.c
> >>> @@ -1366,23 +1366,20 @@ static int skip_check(MpegEncContext *s,
> >>> Picture *p, Picture *ref)
> >>>       return 0;
> >>>   }
> >>>   -static int encode_frame(AVCodecContext *c, AVFrame *frame)
> >>> +static int encode_frame(AVCodecContext *c, AVFrame *frame, AVPacket
> >>> *pkt)
> >>>   {
> >>> -    AVPacket pkt = { 0 };
> >>>       int ret;
> >>>       int size = 0;
> >>>   -    av_init_packet(&pkt);
> >>> -
> >>>       ret = avcodec_send_frame(c, frame);
> >>>       if (ret < 0)
> >>>           return ret;
> >>>         do {
> >>> -        ret = avcodec_receive_packet(c, &pkt);
> >>> +        ret = avcodec_receive_packet(c, pkt);
> >>>           if (ret >= 0) {
> >>> -            size += pkt.size;
> >>> -            av_packet_unref(&pkt);
> >>> +            size += pkt->size;
> >>> +            av_packet_unref(pkt);
> >>>           } else if (ret < 0 && ret != AVERROR(EAGAIN) && ret !=
> >>> AVERROR_EOF)
> >>>               return ret;
> >>>       } while (ret >= 0);
> >>> @@ -1448,6 +1445,7 @@ static int estimate_best_b_count(MpegEncContext
> >>> *s)
> >>>         for (j = 0; j < s->max_b_frames + 1; j++) {
> >>>           AVCodecContext *c;
> >>> +        AVPacket *pkt;
> >>>           int64_t rd = 0;
> >>>             if (!s->input_picture[j])
> >>> @@ -1473,10 +1471,14 @@ static int
> >>> estimate_best_b_count(MpegEncContext *s)
> >>>           if (ret < 0)
> >>>               goto fail;
> >>
> >> The av_packet_free in the fail code uses an uninitialized pointer.
> >>
> >>>   +        pkt = av_packet_alloc();
> >>
> >> You are adding s->max_b_frames + 1 allocations + frees per packet to be
> >> encoded (if I am not mistaken). I am speechless.
> > 
> > I can try to move it outside the loop. But said loop is already
> > allocating that many AVCodecContexts, so hardly that much of a difference.
> > 
> 
> Still the wrong direction even when the current state is already bad.
> 
> >>
> >>> +        if (!pkt)
> >>> +            goto fail;
> >>
> >> You forgot to set ret.
> >>
> >>> +
> >>>           s->tmp_frames[0]->pict_type = AV_PICTURE_TYPE_I;
> >>>           s->tmp_frames[0]->quality   = 1 * FF_QP2LAMBDA;
> >>>   -        out_size = encode_frame(c, s->tmp_frames[0]);
> >>> +        out_size = encode_frame(c, s->tmp_frames[0], pkt);
> >>>           if (out_size < 0) {
> >>>               ret = out_size;
> >>>               goto fail;
> >>> @@ -1491,7 +1493,7 @@ static int estimate_best_b_count(MpegEncContext
> >>> *s)
> >>>                                        AV_PICTURE_TYPE_P :
> >>> AV_PICTURE_TYPE_B;
> >>>               s->tmp_frames[i + 1]->quality   = is_p ? p_lambda :
> >>> b_lambda;
> >>>   -            out_size = encode_frame(c, s->tmp_frames[i + 1]);
> >>> +            out_size = encode_frame(c, s->tmp_frames[i + 1], pkt);
> >>>               if (out_size < 0) {
> >>>                   ret = out_size;
> >>>                   goto fail;
> >>> @@ -1501,7 +1503,7 @@ static int estimate_best_b_count(MpegEncContext
> >>> *s)
> >>>           }
> >>>             /* get the delayed frames */
> >>> -        out_size = encode_frame(c, NULL);
> >>> +        out_size = encode_frame(c, NULL, pkt);
> >>>           if (out_size < 0) {
> >>>               ret = out_size;
> >>>               goto fail;
> >>> @@ -1517,6 +1519,7 @@ static int estimate_best_b_count(MpegEncContext
> >>> *s)
> >>>     fail:
> >>>           avcodec_free_context(&c);
> >>> +        av_packet_free(&pkt);
> >>>           if (ret < 0)
> >>>               return ret;
> >>>       }
> >>>
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel at ffmpeg.org
> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >> To unsubscribe, visit link above, or email
> >> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> >>
> > 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

-- 
Anton Khirnov