[FFmpeg-devel] [PATCH v2 7/8] avformat/utils: Remove unnecessary packet copies
Andriy Gelman
andriy.gelman at gmail.com
Wed Aug 28 18:15:16 EEST 2019
On Mon, 19. Aug 23:56, Andreas Rheinhardt wrote:
> Up until now, read_frame_internal in avformat/utils.c uses a spare
> packet on the stack that serves no real purpose: At no point in this
> function is there a need for another packet besides the packet destined
> for output:
> 1. If the packet doesn't need a parser, but is output as is, the content
> of the spare packet (that at this point contains a freshly read packet)
> is simply copied into the output packet (via simple assignment, not
> av_packet_move_ref, thereby confusing ownership).
> 2. If the packet needs parsing, the spare packet will be reset after
> parsing and any packets resulting from the packet read will be put into
> a packet list; the output packet is not used here at all.
> 3. If the stream should be discarded, the spare packet will be
> unreferenced; the output packet is not used here at all.
>
> Therefore the spare packet and the copies can be removed in priniple.
typo: principle
> In practice, one more thing needs to be taken care of: If ff_read_packet
> failed, this did not affect the output packet, now it does. This
> potential problem is solved by making sure that ff_read_packet always
> resets the output packet in case of errors.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
> Side note: This skip_to_keyframe stuff which is touched in this commit
> has been introduced in 4a95876f to be able to drop non-keyframes after
> the parser in case the keyframes are wrongly marked in the file. But the
> parser returns packets by putting them in the packet queue and not by
> returning them via its pkt parameter, so that st->skip_to_keyframe will
> never be unset and no packet will be dropped because of it for a stream
> that gets parsed. It actually only works ("works" means that an error
> message will be displayed for a broken file where the keyframes are not
> correctly marked) for the file for which it was intended (the one from
> issue 1003) by accident. Maybe it should be removed altogether?
I agree, when the parser is called, the skip_to_keyframe code is currently
skipped.
Would it make sense for the skip_to_keyframe code to also be added after pkt
is retrieved from ff_packet_list_get ?
>
> libavformat/utils.c | 51 ++++++++++++++++++++++-----------------------
> 1 file changed, 25 insertions(+), 26 deletions(-)
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index db0f53869f..d6d615b690 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -830,6 +830,10 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
> int ret, i, err;
> AVStream *st;
>
> + pkt->data = NULL;
> + pkt->size = 0;
> + av_init_packet(pkt);
> +
> for (;;) {
> AVPacketList *pktl = s->internal->raw_packet_buffer;
> const AVPacket *pkt1;
> @@ -847,11 +851,11 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
> }
> }
>
> - pkt->data = NULL;
> - pkt->size = 0;
> - av_init_packet(pkt);
> ret = s->iformat->read_packet(s, pkt);
> if (ret < 0) {
> + pkt->data = NULL;
> + pkt->size = 0;
> + av_init_packet(pkt);
> /* Some demuxers return FFERROR_REDO when they consume
> data and discard it (ignored streams, junk, extradata).
> We must re-call the demuxer to get the real packet. */
> @@ -1579,10 +1583,9 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
>
> while (!got_packet && !s->internal->parse_queue) {
> AVStream *st;
> - AVPacket cur_pkt;
>
> /* read next packet */
> - ret = ff_read_packet(s, &cur_pkt);
> + ret = ff_read_packet(s, pkt);
> if (ret < 0) {
> if (ret == AVERROR(EAGAIN))
> return ret;
> @@ -1597,7 +1600,7 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
> break;
> }
> ret = 0;
> - st = s->streams[cur_pkt.stream_index];
> + st = s->streams[pkt->stream_index];
>
> /* update context if required */
> if (st->internal->need_context_update) {
> @@ -1615,7 +1618,7 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
>
> ret = avcodec_parameters_to_context(st->internal->avctx, st->codecpar);
> if (ret < 0) {
> - av_packet_unref(&cur_pkt);
> + av_packet_unref(pkt);
> return ret;
> }
>
> @@ -1624,7 +1627,7 @@ FF_DISABLE_DEPRECATION_WARNINGS
> /* update deprecated public codec context */
> ret = avcodec_parameters_to_context(st->codec, st->codecpar);
> if (ret < 0) {
> - av_packet_unref(&cur_pkt);
> + av_packet_unref(pkt);
> return ret;
> }
> FF_ENABLE_DEPRECATION_WARNINGS
> @@ -1633,23 +1636,23 @@ FF_ENABLE_DEPRECATION_WARNINGS
> st->internal->need_context_update = 0;
> }
>
> - if (cur_pkt.pts != AV_NOPTS_VALUE &&
> - cur_pkt.dts != AV_NOPTS_VALUE &&
> - cur_pkt.pts < cur_pkt.dts) {
> + if (pkt->pts != AV_NOPTS_VALUE &&
> + pkt->dts != AV_NOPTS_VALUE &&
> + pkt->pts < pkt->dts) {
> av_log(s, AV_LOG_WARNING,
> "Invalid timestamps stream=%d, pts=%s, dts=%s, size=%d\n",
> - cur_pkt.stream_index,
> - av_ts2str(cur_pkt.pts),
> - av_ts2str(cur_pkt.dts),
> - cur_pkt.size);
> + pkt->stream_index,
> + av_ts2str(pkt->pts),
> + av_ts2str(pkt->dts),
> + pkt->size);
> }
> if (s->debug & FF_FDEBUG_TS)
> av_log(s, AV_LOG_DEBUG,
> "ff_read_packet stream=%d, pts=%s, dts=%s, size=%d, duration=%"PRId64", flags=%d\n",
> - cur_pkt.stream_index,
> - av_ts2str(cur_pkt.pts),
> - av_ts2str(cur_pkt.dts),
> - cur_pkt.size, cur_pkt.duration, cur_pkt.flags);
> + pkt->stream_index,
> + av_ts2str(pkt->pts),
> + av_ts2str(pkt->dts),
> + pkt->size, pkt->duration, pkt->flags);
>
> if (st->need_parsing && !st->parser && !(s->flags & AVFMT_FLAG_NOPARSE)) {
> st->parser = av_parser_init(st->codecpar->codec_id);
> @@ -1669,7 +1672,6 @@ FF_ENABLE_DEPRECATION_WARNINGS
>
> if (!st->need_parsing || !st->parser) {
> /* no parsing needed: we just output the packet as is */
> - *pkt = cur_pkt;
> compute_pkt_fields(s, st, NULL, pkt, AV_NOPTS_VALUE, AV_NOPTS_VALUE);
> if ((s->iformat->flags & AVFMT_GENERIC_INDEX) &&
> (pkt->flags & AV_PKT_FLAG_KEY) && pkt->dts != AV_NOPTS_VALUE) {
> @@ -1679,7 +1681,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
> }
> got_packet = 1;
> } else if (st->discard < AVDISCARD_ALL) {
> - if ((ret = parse_packet(s, &cur_pkt, cur_pkt.stream_index)) < 0)
> + if ((ret = parse_packet(s, pkt, pkt->stream_index)) < 0)
> return ret;
> st->codecpar->sample_rate = st->internal->avctx->sample_rate;
> st->codecpar->bit_rate = st->internal->avctx->bit_rate;
> @@ -1688,15 +1690,12 @@ FF_ENABLE_DEPRECATION_WARNINGS
> st->codecpar->codec_id = st->internal->avctx->codec_id;
> } else {
> /* free packet */
> - av_packet_unref(&cur_pkt);
> + av_packet_unref(pkt);
> }
> if (pkt->flags & AV_PKT_FLAG_KEY)
> st->skip_to_keyframe = 0;
> if (st->skip_to_keyframe) {
> - av_packet_unref(&cur_pkt);
> - if (got_packet) {
> - *pkt = cur_pkt;
> - }
> + av_packet_unref(pkt);
> got_packet = 0;
> }
> }
I went over all the commits in this patchset. They look good to me.
(I had a minor comment on Fix memleaks II).
Andriy
More information about the ffmpeg-devel
mailing list