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 <jamr...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
