Message ID | 20190926000558.8410-1-andreas.rheinhardt@gmail.com |
---|---|
State | Superseded |
Headers | show |
On 9/25/2019 9:05 PM, Andreas Rheinhardt wrote: > Up until now, ff_packet_list_put had a flaw: When it moved a packet to > the list (meaning, when it ought to move the reference to the packet > list instead of creating a new one via av_packet_ref), it did not reset > the original packet, confusing the ownership of the data in the packet. > This has been done because some callers of this function were not > compatible with resetting the packet. > > This commit changes these callers and fixes this flaw. In order to > indicate that the ownership of the packet has moved to the packet list, > pointers to constant AVPackets are used whenever the target of the > pointer might already be owned by the packet list. > > Furthermore, the packet is always made refcounted so that its data is > really owned by the packet. This was already true for the current > callers. Ah, my bad. When you said "I can add a commit doing this" i assumed you'd do it in a separate patch. I already committed the "v4 3/9" one. I'll apply the doxy changes and the av_packet_make_refcounted() call in a new patch in a moment. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/internal.h | 3 ++- > libavformat/utils.c | 37 ++++++++++++++++++++++--------------- > 2 files changed, 24 insertions(+), 16 deletions(-) > > diff --git a/libavformat/internal.h b/libavformat/internal.h > index 163587f416..a37404d823 100644 > --- a/libavformat/internal.h > +++ b/libavformat/internal.h > @@ -763,7 +763,8 @@ void ff_format_set_url(AVFormatContext *s, char *url); > * > * @param head List head element > * @param tail List tail element > - * @param pkt The packet being appended > + * @param pkt The packet being appended. It will be made refcounted > + * if it isn't already. > * @param flags Any combination of FF_PACKETLIST_FLAG_* flags > * @return 0 on success, negative AVERROR value on failure. On failure, > the list is unchanged > diff --git a/libavformat/utils.c b/libavformat/utils.c > index 4657ba2642..9f8a5bfb63 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -460,10 +460,12 @@ int ff_packet_list_put(AVPacketList **packet_buffer, > return ret; > } > } else { > - // TODO: Adapt callers in this file so the line below can use > - // av_packet_move_ref() to effectively move the reference > - // to the list. > - pktl->pkt = *pkt; > + ret = av_packet_make_refcounted(pkt); > + if (ret < 0) { > + av_free(pktl); > + return ret; > + } > + av_packet_move_ref(&pktl->pkt, pkt); > } > > if (*packet_buffer) > @@ -835,6 +837,7 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) > > for (;;) { > AVPacketList *pktl = s->internal->raw_packet_buffer; > + const AVPacket *pkt1; > > if (pktl) { > st = s->streams[pktl->pkt.stream_index]; > @@ -922,9 +925,10 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) > av_packet_unref(pkt); > return err; > } > - s->internal->raw_packet_buffer_remaining_size -= pkt->size; > + pkt1 = &s->internal->raw_packet_buffer_end->pkt; > + s->internal->raw_packet_buffer_remaining_size -= pkt1->size; > > - if ((err = probe_codec(s, st, pkt)) < 0) > + if ((err = probe_codec(s, st, pkt1)) < 0) > return err; > } > } > @@ -3032,8 +3036,8 @@ static int has_codec_parameters(AVStream *st, const char **errmsg_ptr) > } > > /* returns 1 or 0 if or if not decoded data was returned, or a negative error */ > -static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket *avpkt, > - AVDictionary **options) > +static int try_decode_frame(AVFormatContext *s, AVStream *st, > + const AVPacket *avpkt, AVDictionary **options) > { > AVCodecContext *avctx = st->internal->avctx; > const AVCodec *codec; > @@ -3525,7 +3529,7 @@ fail: > return ret; > } > > -static int extract_extradata(AVStream *st, AVPacket *pkt) > +static int extract_extradata(AVStream *st, const AVPacket *pkt) > { > AVStreamInternal *sti = st->internal; > AVPacket *pkt_ref; > @@ -3588,7 +3592,7 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options) > int64_t read_size; > AVStream *st; > AVCodecContext *avctx; > - AVPacket pkt1, *pkt; > + AVPacket pkt1; > int64_t old_offset = avio_tell(ic->pb); > // new streams might appear, no options for those > int orig_nb_streams = ic->nb_streams; > @@ -3707,6 +3711,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > > read_size = 0; > for (;;) { > + const AVPacket *pkt; > int analyzed_all_streams; > if (ff_check_interrupt(&ic->interrupt_callback)) { > ret = AVERROR_EXIT; > @@ -3800,14 +3805,16 @@ FF_ENABLE_DEPRECATION_WARNINGS > break; > } > > - pkt = &pkt1; > - > if (!(ic->flags & AVFMT_FLAG_NOBUFFER)) { > ret = ff_packet_list_put(&ic->internal->packet_buffer, > &ic->internal->packet_buffer_end, > - pkt, 0); > + &pkt1, 0); > if (ret < 0) > goto find_stream_info_err; > + > + pkt = &ic->internal->packet_buffer_end->pkt; > + } else { > + pkt = &pkt1; > } > > st = ic->streams[pkt->stream_index]; > @@ -3885,7 +3892,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > limit, > t, pkt->stream_index); > if (ic->flags & AVFMT_FLAG_NOBUFFER) > - av_packet_unref(pkt); > + av_packet_unref(&pkt1); > break; > } > if (pkt->duration) { > @@ -3922,7 +3929,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > (options && i < orig_nb_streams) ? &options[i] : NULL); > > if (ic->flags & AVFMT_FLAG_NOBUFFER) > - av_packet_unref(pkt); > + av_packet_unref(&pkt1); > > st->codec_info_nb_frames++; > count++; >
diff --git a/libavformat/internal.h b/libavformat/internal.h index 163587f416..a37404d823 100644 --- a/libavformat/internal.h +++ b/libavformat/internal.h @@ -763,7 +763,8 @@ void ff_format_set_url(AVFormatContext *s, char *url); * * @param head List head element * @param tail List tail element - * @param pkt The packet being appended + * @param pkt The packet being appended. It will be made refcounted + * if it isn't already. * @param flags Any combination of FF_PACKETLIST_FLAG_* flags * @return 0 on success, negative AVERROR value on failure. On failure, the list is unchanged diff --git a/libavformat/utils.c b/libavformat/utils.c index 4657ba2642..9f8a5bfb63 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -460,10 +460,12 @@ int ff_packet_list_put(AVPacketList **packet_buffer, return ret; } } else { - // TODO: Adapt callers in this file so the line below can use - // av_packet_move_ref() to effectively move the reference - // to the list. - pktl->pkt = *pkt; + ret = av_packet_make_refcounted(pkt); + if (ret < 0) { + av_free(pktl); + return ret; + } + av_packet_move_ref(&pktl->pkt, pkt); } if (*packet_buffer) @@ -835,6 +837,7 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) for (;;) { AVPacketList *pktl = s->internal->raw_packet_buffer; + const AVPacket *pkt1; if (pktl) { st = s->streams[pktl->pkt.stream_index]; @@ -922,9 +925,10 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) av_packet_unref(pkt); return err; } - s->internal->raw_packet_buffer_remaining_size -= pkt->size; + pkt1 = &s->internal->raw_packet_buffer_end->pkt; + s->internal->raw_packet_buffer_remaining_size -= pkt1->size; - if ((err = probe_codec(s, st, pkt)) < 0) + if ((err = probe_codec(s, st, pkt1)) < 0) return err; } } @@ -3032,8 +3036,8 @@ static int has_codec_parameters(AVStream *st, const char **errmsg_ptr) } /* returns 1 or 0 if or if not decoded data was returned, or a negative error */ -static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket *avpkt, - AVDictionary **options) +static int try_decode_frame(AVFormatContext *s, AVStream *st, + const AVPacket *avpkt, AVDictionary **options) { AVCodecContext *avctx = st->internal->avctx; const AVCodec *codec; @@ -3525,7 +3529,7 @@ fail: return ret; } -static int extract_extradata(AVStream *st, AVPacket *pkt) +static int extract_extradata(AVStream *st, const AVPacket *pkt) { AVStreamInternal *sti = st->internal; AVPacket *pkt_ref; @@ -3588,7 +3592,7 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options) int64_t read_size; AVStream *st; AVCodecContext *avctx; - AVPacket pkt1, *pkt; + AVPacket pkt1; int64_t old_offset = avio_tell(ic->pb); // new streams might appear, no options for those int orig_nb_streams = ic->nb_streams; @@ -3707,6 +3711,7 @@ FF_ENABLE_DEPRECATION_WARNINGS read_size = 0; for (;;) { + const AVPacket *pkt; int analyzed_all_streams; if (ff_check_interrupt(&ic->interrupt_callback)) { ret = AVERROR_EXIT; @@ -3800,14 +3805,16 @@ FF_ENABLE_DEPRECATION_WARNINGS break; } - pkt = &pkt1; - if (!(ic->flags & AVFMT_FLAG_NOBUFFER)) { ret = ff_packet_list_put(&ic->internal->packet_buffer, &ic->internal->packet_buffer_end, - pkt, 0); + &pkt1, 0); if (ret < 0) goto find_stream_info_err; + + pkt = &ic->internal->packet_buffer_end->pkt; + } else { + pkt = &pkt1; } st = ic->streams[pkt->stream_index]; @@ -3885,7 +3892,7 @@ FF_ENABLE_DEPRECATION_WARNINGS limit, t, pkt->stream_index); if (ic->flags & AVFMT_FLAG_NOBUFFER) - av_packet_unref(pkt); + av_packet_unref(&pkt1); break; } if (pkt->duration) { @@ -3922,7 +3929,7 @@ FF_ENABLE_DEPRECATION_WARNINGS (options && i < orig_nb_streams) ? &options[i] : NULL); if (ic->flags & AVFMT_FLAG_NOBUFFER) - av_packet_unref(pkt); + av_packet_unref(&pkt1); st->codec_info_nb_frames++; count++;
Up until now, ff_packet_list_put had a flaw: When it moved a packet to the list (meaning, when it ought to move the reference to the packet list instead of creating a new one via av_packet_ref), it did not reset the original packet, confusing the ownership of the data in the packet. This has been done because some callers of this function were not compatible with resetting the packet. This commit changes these callers and fixes this flaw. In order to indicate that the ownership of the packet has moved to the packet list, pointers to constant AVPackets are used whenever the target of the pointer might already be owned by the packet list. Furthermore, the packet is always made refcounted so that its data is really owned by the packet. This was already true for the current callers. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/internal.h | 3 ++- libavformat/utils.c | 37 ++++++++++++++++++++++--------------- 2 files changed, 24 insertions(+), 16 deletions(-)