[FFmpeg-devel] [PATCH 23/50] avformat/mux: use av_packet_alloc() to allocate packets

[James Almer]

On 2/8/2021 3:50 PM, James Almer wrote:
> On 2/8/2021 3:22 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> Signed-off-by: James Almer<jamrial at gmail.com>
>>> ---
>>>   libavformat/internal.h |  5 +++++
>>>   libavformat/mux.c      | 44 ++++++++++++++++++++++--------------------
>>>   libavformat/options.c  |  6 ++++++
>>>   libavformat/utils.c    |  1 +
>>>   4 files changed, 35 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>>> index d0db331b96..69a7caff93 100644
>>> --- a/libavformat/internal.h
>>> +++ b/libavformat/internal.h
>>> @@ -92,6 +92,11 @@ struct AVFormatInternal {
>>>        */
>>>       struct AVPacketList *parse_queue;
>>>       struct AVPacketList *parse_queue_end;
>>> +
>>> +    /**
>>> +     * Used to hold temporary packets.
>>> +     */
>>> +    AVPacket *pkt;
>>>       /**
>>>        * Remaining size available for raw_packet_buffer, in bytes.
>>>        */
>>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>>> index 84c56ac6ba..3600e74a50 100644
>>> --- a/libavformat/mux.c
>>> +++ b/libavformat/mux.c
>>> @@ -1052,7 +1052,9 @@ int ff_interleaved_peek(AVFormatContext *s, int 
>>> stream,
>>>       AVPacketList *pktl = s->internal->packet_buffer;
>>>       while (pktl) {
>>>           if (pktl->pkt.stream_index == stream) {
>>> -            *pkt = pktl->pkt;
>>> +            int ret = av_packet_ref(pkt, &pktl->pkt);
>>> +            if (ret < 0)
>>> +                return ret;
>> This will lead to memleaks, because up until now the callers just
>> received a non-ownership packets, ergo they did not unref the packet.
>> (The fate-movenc test fails with this when run under valgrind/asan.)
>>
>> Returning a pointer to a const AVPacket and signaling the offsetted
>> timestamps via other pointer arguments would avoid this problem.
> 
> There's a single user of ff_interleaved_peek(), which is the movenc one, 
> and i made it unref the packet right after using it. But that's done in 
> the following patch, so i guess i should change this function and its 
> one caller at the same time.
> 
> Also, i could just return the offsetted pts and dts values and not 
> bother at all with packets. No allocations that way.

Decided to implement your idea with a few changes in a separate patchset.