| Message ID | 20200414203733.4578-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | e7eb379d98f6b8be9b19afd71fc5b3473eca4a47 |
| Headers | show |
| Series | [FFmpeg-devel] avformat/movenc: remove call to av_copy_packet_side_data() when concatenatic eac3 syncframes | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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@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.)
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@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.
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);
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. Keep all the properties from the first packet only in the output packet instead. Signed-off-by: James Almer <jamrial@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. libavformat/movenc.c | 2 -- 1 file changed, 2 deletions(-)