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

[Andreas Rheinhardt]

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.

>  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.)