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.
- Andreas
_______________________________________________
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".
