[FFmpeg-devel] [PATCH] avformat/movenc: remove call to av_copy_packet_side_data() when concatenatic eac3 syncframes

[James Almer]

On 4/14/2020 6:21 PM, Andreas Rheinhardt wrote:
> James Almer:
>> This generates a potential memory leak if info->pkt already contains side data
>> elements, and may mix side data from the last packet with other properties from
>> the first.
> 
> Now that you mention it: Yes, there is a potential memleak.* So remove this.
>>
>> Keep all the properties from the first packet only in the output packet
>> instead.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> The alternative is to keep all the properties of the last packet instead of the
>> first, or merge the side data from all packets into the output packet, giving
>> priority to the either the first or the last in case of duplicate side data
>> types.
>>
>> I have no idea what would be ideal since i don't have a sample that triggers
>> this code, and i don't know what kind of side data these eac3 packets could
>> contain that one would need to choose between them.
>>
> Right now any side data these packets might contain is ignored (with the
> potential exception of AVProducerReferenceTime stuff). Relevant side
> data would be AV_PKT_DATA_SKIP_SAMPLES (which the mov muxer ignores) or
> maybe AV_PKT_DATA_PARAM_CHANGE.

The only AVProducerReferenceTime we care about is the first for a given
fragment, so this patch might in fact fix a bug in that regard.

Will push then. Thanks.

> 
>>  libavformat/movenc.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>> index bc8d08044e..bf3e4fa2ce 100644
>> --- a/libavformat/movenc.c
>> +++ b/libavformat/movenc.c
>> @@ -520,8 +520,6 @@ concatenate:
>>          memcpy(info->pkt.data + info->pkt.size - pkt->size, pkt->data, pkt->size);
>>          info->num_blocks += num_blocks;
>>          info->pkt.duration += pkt->duration;
>> -        if ((ret = av_copy_packet_side_data(&info->pkt, pkt)) < 0)
>> -            goto end;
>>          if (info->num_blocks != 6)
>>              goto end;
>>          av_packet_unref(pkt);
>>
> 
> - Andreas
> 
> *: And anyway, av_copy_packet_side_data() is even more broken: It
> allocates a new side-data-array and only then checks for whether dst and
> src coincide. Was the intention behind this to enable a packet's side
> data to be made independently allocated in a scenario like AVPacket
> pkt1, pkt2; ... pkt1 = pkt2; av_copy_packet_side_data(&pkt1, &pkt1);? If
> so that is very dangerous given the av_packet_unref() of the destination
> packet in case of failure.
> (And if src != dst, then the already allocated side data leaks in case
> of allocation failure because dst->side_data_elems does not accurately
> reflect how many side data elements there are right now.)

Yes, it's broken and why it's deprecated and scheduled for removal.
Unfortunately there not direct replacement, as av_packet_copy_props()
does a lot more than copying side data.
I suggested one using the av_packet namespace some time ago, but it was
rejected.