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.rheinha...@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 _______________________________________________ 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".