3 Sept 2021, 17:23 by andreas.rheinha...@outlook.com:
> Andreas Rheinhardt:
>
>> Up until now, ff_write_chained() copied the packet (manually, not with
>> av_packet_move_ref()) from a packet given to it to a stack packet whose
>> timing and stream_index is then modified before being sent to another
>> muxer via av_(interleaved_)write_frame(). Afterwards it is intended to
>> sync the fields of the packet relevant to freeing again; yet this only
>> encompasses buf, side_data and side_data_elems and not the newly added
>> opaque_ref. The other fields are not synced so that the returned packet
>> can have a size > 0 and data != NULL despite its buf being NULL (this
>> always happens in the interleaved codepath; before commit
>> fe251f77c80b0512ab8907902e1dbed3f4fe1aad it could also happen in the
>> noninterleaved one). This leads to double-frees if the interleaved
>> codepath is used and opaque_ref is set.
>>
>> This commit therefore changes this by directly reusing the packet
>> instead of a spare packet. Given that av_write_frame() does not
>> change the packet given to it, one only needs to restore the timing
>> information to return it as it was; for the interleaved codepath
>> it is not possible to do likewise*, because av_interleaved_write_frame()
>> takes ownership of the packets given to it and returns blank packets.
>> But precisely because of this users of the interleaved codepath
>> have no legitimate expectation that their packet will be returned
>> unchanged. In line with av_interleaved_write_frame() ff_write_chained()
>> therefore returns blank packets when using the interleaved codepath.
>>
>> Making the only user of said codepath compatible with this was trivial.
>>
>> *: Unless one wanted to create a full new reference.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@outlook.com>
>> ---
>> ff_write_chained() will be redundant when we allow muxers to convert
>> timestamps internally (we can then make the slave muxer do the
>> conversion). So I am not really worried about the case of adding
>> a new timestamp-related field and forgetting to reset it in
>> ff_write_chained().
>>
>> libavformat/internal.h | 3 ++-
>> libavformat/mux.c | 35 ++++++++++++++++++++++-------------
>> libavformat/segment.c | 4 +++-
>> 3 files changed, 27 insertions(+), 15 deletions(-)
>>
>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>> index 4fc1154b9d..9d7312c0e2 100644
>> --- a/libavformat/internal.h
>> +++ b/libavformat/internal.h
>> @@ -506,7 +506,8 @@ void ff_sdp_write_media(char *buff, int size, AVStream
>> *st, int idx,
>> *
>> * @param dst the muxer to write the packet to
>> * @param dst_stream the stream index within dst to write the packet to
>> - * @param pkt the packet to be written
>> + * @param pkt the packet to be written. It will be returned blank when
>> + * av_interleaved_write_frame() is used, unchanged otherwise.
>> * @param src the muxer the packet originally was intended for
>> * @param interleave 0->use av_write_frame, 1->av_interleaved_write_frame
>> * @return the value av_write_frame returned
>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>> index b1ad0dd561..6ba1306f2b 100644
>> --- a/libavformat/mux.c
>> +++ b/libavformat/mux.c
>> @@ -643,12 +643,12 @@ static void guess_pkt_duration(AVFormatContext *s,
>> AVStream *st, AVPacket *pkt)
>> */
>> static int write_packet(AVFormatContext *s, AVPacket *pkt)
>> {
>> + AVStream *const st = s->streams[pkt->stream_index];
>> int ret;
>>
>> // If the timestamp offsetting below is adjusted, adjust
>> // ff_interleaved_peek similarly.
>> if (s->output_ts_offset) {
>> - AVStream *st = s->streams[pkt->stream_index];
>> int64_t offset = av_rescale_q(s->output_ts_offset, AV_TIME_BASE_Q,
>> st->time_base);
>>
>> if (pkt->dts != AV_NOPTS_VALUE)
>> @@ -658,7 +658,6 @@ static int write_packet(AVFormatContext *s, AVPacket
>> *pkt)
>> }
>>
>> if (s->avoid_negative_ts > 0) {
>> - AVStream *st = s->streams[pkt->stream_index];
>> int64_t offset = st->internal->mux_ts_offset;
>> int64_t ts = s->internal->avoid_negative_ts_use_pts ? pkt->pts : pkt->dts;
>>
>> @@ -719,7 +718,7 @@ static int write_packet(AVFormatContext *s, AVPacket
>> *pkt)
>> }
>>
>> if (ret >= 0)
>> - s->streams[pkt->stream_index]->nb_frames++;
>> + st->nb_frames++;
>>
>> return ret;
>> }
>> @@ -1192,6 +1191,7 @@ int av_write_frame(AVFormatContext *s, AVPacket *in)
>> pkt = in;
>> } else {
>> /* We don't own in, so we have to make sure not to modify it.
>> + * (ff_write_chained() relies on this fact.)
>> * The following avoids copying in's data unnecessarily.
>> * Copying side data is unavoidable as a bitstream filter
>> * may change it, e.g. free it on errors. */
>> @@ -1291,21 +1291,30 @@ int av_get_output_timestamp(struct AVFormatContext
>> *s, int stream,
>> int ff_write_chained(AVFormatContext *dst, int dst_stream, AVPacket *pkt,
>> AVFormatContext *src, int interleave)
>> {
>> - AVPacket local_pkt;
>> + int64_t pts = pkt->pts, dts = pkt->dts, duration = pkt->duration;
>> + int stream_index = pkt->stream_index;
>> + AVRational time_base = pkt->time_base;
>> int ret;
>>
>> - local_pkt = *pkt;
>> - local_pkt.stream_index = dst_stream;
>> + pkt->stream_index = dst_stream;
>>
>> - av_packet_rescale_ts(&local_pkt,
>> - src->streams[pkt->stream_index]->time_base,
>> + av_packet_rescale_ts(pkt,
>> + src->streams[stream_index]->time_base,
>> dst->streams[dst_stream]->time_base);
>>
>> - if (interleave) ret = av_interleaved_write_frame(dst, &local_pkt);
>> - else ret = av_write_frame(dst, &local_pkt);
>> - pkt->buf = local_pkt.buf;
>> - pkt->side_data = local_pkt.side_data;
>> - pkt->side_data_elems = local_pkt.side_data_elems;
>> + if (!interleave) {
>> + ret = av_write_frame(dst, pkt);
>> + /* We only have to backup and restore the fields that
>> + * we changed ourselves, because av_write_frame() does not
>> + * modify the packet given to it. */
>> + pkt->pts = pts;
>> + pkt->dts = dts;
>> + pkt->duration = duration;
>> + pkt->stream_index = stream_index;
>> + pkt->time_base = time_base;
>> + } else
>> + ret = av_interleaved_write_frame(dst, pkt);
>> +
>> return ret;
>> }
>>
>> diff --git a/libavformat/segment.c b/libavformat/segment.c
>> index ed671353d0..7c171b6fa4 100644
>> --- a/libavformat/segment.c
>> +++ b/libavformat/segment.c
>> @@ -952,7 +952,9 @@ calc_times:
>> seg->initial_offset || seg->reset_timestamps ||
>> seg->avf->oformat->interleave_packet);
>>
>> fail:
>> - if (pkt->stream_index == seg->reference_stream_index) {
>> + /* Use st->index here as the packet returned from ff_write_chained()
>> + * is blank if interleaving has been used. */
>> + if (st->index == seg->reference_stream_index) {
>> seg->frame_count++;
>> seg->segment_frame_count++;
>> }
>>
> Will apply tomorrow unless there are objections.
>
Messy to read, but LGTM.
_______________________________________________
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".
