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

[James Almer]

On 2/8/2021 11:50 AM, Andreas Rheinhardt wrote:
> 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.

I agree. I could also just withdraw this patch altogether (or rather, 
just remove the av_init_packet() call), since it's in lavc, where 
sizeof(AVPacket) can be used just fine.

> 
>>>
>>>> +        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".
>