On Sun, 18 Oct 2015 17:59:47 +0200
Luca Barbato <[email protected]> wrote:
> As documented, `av_dup_packet` is broken by design, `av_packet_ref`
> matches the AVFrame ref-counted API and can be safely used instead.
>
> Deprecate `av_dup_packet`.
> ---
> avplay.c | 17 +++++++++++------
> libavcodec/avcodec.h | 5 ++++-
> libavcodec/avpacket.c | 4 ++++
> libavformat/matroskaenc.c | 7 +------
> libavformat/mux.c | 10 ++++------
> 5 files changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/avplay.c b/avplay.c
> index 7f1bbb7..5fe5f2f 100644
> --- a/avplay.c
> +++ b/avplay.c
> @@ -319,15 +319,20 @@ static void packet_queue_end(PacketQueue *q)
> static int packet_queue_put(PacketQueue *q, AVPacket *pkt)
> {
> AVPacketList *pkt1;
> -
> - /* duplicate the packet */
> - if (pkt != &flush_pkt && av_dup_packet(pkt) < 0)
> - return -1;
> + int ret;
>
> pkt1 = av_malloc(sizeof(AVPacketList));
> if (!pkt1)
> - return -1;
> - pkt1->pkt = *pkt;
> + return AVERROR(ENOMEM);
> +
> + /* duplicate the packet */
> + if (pkt != &flush_pkt) {
> + if (ret = av_packet_ref(&pkt1->pkt, pkt)) < 0)
Wait, you're calling av_packet_ref() on an uninitialized packet? But
the destination packet must be initialized before calling
av_packet_ref(). (I think I insisted that this kind of stuff should be
added to the doxygen when av_packet_ref() was added...)
So use av_mallocz().
> + return -1;
> + } else {
> + pkt1->pkt = *pkt;
> + }
> +
> pkt1->next = NULL;
>
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 4362aeb..a370e3e 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3463,12 +3463,15 @@ int av_grow_packet(AVPacket *pkt, int grow_by);
> */
> int av_packet_from_data(AVPacket *pkt, uint8_t *data, int size);
>
> +#if FF_API_AVPACKET_OLD_API
> /**
> * @warning This is a hack - the packet memory allocation stuff is broken.
> The
> * packet is allocated if it was not really allocated.
> + *
> + * @deprecated Use av_packet_ref
> */
> +attribute_deprecated
> int av_dup_packet(AVPacket *pkt);
Also add that av_dup_packet() after a read_frame() is not needed
anymore, and packets are always refcounted. Put yourself into the
situation of someone who tries to find out what they should do with an
av_dup_packet() call in code they're not entirely familiar with
(maintainers).
> -#if FF_API_AVPACKET_OLD_API
> /**
> * Free a packet.
> *
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index eaea061..cec5bf8 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -128,6 +128,8 @@ int av_packet_from_data(AVPacket *pkt, uint8_t *data, int
> size)
> return 0;
> }
>
> +#if FF_API_AVPACKET_OLD_API
> +FF_DISABLE_DEPRECATION_WARNINGS
> #define ALLOC_MALLOC(data, size) data = av_malloc(size)
> #define ALLOC_BUF(data, size) \
> do { \
> @@ -187,6 +189,8 @@ failed_alloc:
> av_packet_unref(pkt);
> return AVERROR(ENOMEM);
> }
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>
> void av_packet_free_side_data(AVPacket *pkt)
> {
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index f1bd272..baffd54 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1608,12 +1608,7 @@ static int mkv_write_packet(AVFormatContext *s,
> AVPacket *pkt)
> // buffer an audio packet to ensure the packet containing the video
> // keyframe's timecode is contained in the same cluster for WebM
> if (codec_type == AVMEDIA_TYPE_AUDIO) {
> - mkv->cur_audio_pkt = *pkt;
> - if (pkt->buf) {
> - mkv->cur_audio_pkt.buf = av_buffer_ref(pkt->buf);
> - ret = mkv->cur_audio_pkt.buf ? 0 : AVERROR(ENOMEM);
> - } else
> - ret = av_dup_packet(&mkv->cur_audio_pkt);
> + ret = av_packet_ref(&mkv->cur_audio_pkt, pkt);
Is mkv->cur_audio_pkt initialized?
> } else
> ret = mkv_write_packet_internal(s, pkt);
> return ret;
> diff --git a/libavformat/mux.c b/libavformat/mux.c
> index e86d202..4a81e36 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -416,12 +416,8 @@ int ff_interleave_add_packet(AVFormatContext *s,
> AVPacket *pkt,
> this_pktl = av_mallocz(sizeof(AVPacketList));
> if (!this_pktl)
> return AVERROR(ENOMEM);
> - this_pktl->pkt = *pkt;
> - pkt->buf = NULL;
> - pkt->side_data = NULL;
> - pkt->side_data_elems = 0;
> - // Duplicate the packet if it uses non-allocated memory
> - if ((ret = av_dup_packet(&this_pktl->pkt)) < 0) {
> +
> + if ((ret = av_packet_ref(&this_pktl->pkt, pkt)) < 0) {
Same here.
> av_free(this_pktl);
> return ret;
> }
> @@ -450,6 +446,8 @@ next_non_null:
> s->streams[pkt->stream_index]->last_in_packet_buffer =
> *next_point = this_pktl;
>
> + av_packet_unref(pkt);
> +
> return 0;
> }
>
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
