[FFmpeg-devel] [PATCH 50/50] fftools/ffplay: use av_packet_alloc() to allocate packets
Marton Balint
cus at passwd.hu
Sun Feb 7 14:26:10 EET 2021
On Thu, 4 Feb 2021, James Almer wrote:
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
> fftools/ffplay.c | 204 +++++++++++++++++++++++++++++------------------
> 1 file changed, 127 insertions(+), 77 deletions(-)
Thanks for this, I see a couple of things that can be simplified, maybe it
is easier if I take over this patch and take care of it directly if you
don't mind.
Thanks,
Marton
>
> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> index 9ff0425163..d48edc0797 100644
> --- a/fftools/ffplay.c
> +++ b/fftools/ffplay.c
> @@ -36,6 +36,7 @@
> #include "libavutil/pixdesc.h"
> #include "libavutil/imgutils.h"
> #include "libavutil/dict.h"
> +#include "libavutil/fifo.h"
> #include "libavutil/parseutils.h"
> #include "libavutil/samplefmt.h"
> #include "libavutil/avassert.h"
> @@ -111,13 +112,12 @@ const int program_birth_year = 2003;
> static unsigned sws_flags = SWS_BICUBIC;
>
> typedef struct MyAVPacketList {
> - AVPacket pkt;
> - struct MyAVPacketList *next;
> + AVPacket *pkt;
> int serial;
> } MyAVPacketList;
>
> typedef struct PacketQueue {
> - MyAVPacketList *first_pkt, *last_pkt;
> + AVFifoBuffer *pkt_list;
> int nb_packets;
> int size;
> int64_t duration;
> @@ -187,7 +187,8 @@ enum {
> };
>
> typedef struct Decoder {
> - AVPacket pkt;
> + AVPacket *pkt;
> + AVPacket *tmp_pkt;
> PacketQueue *queue;
> AVCodecContext *avctx;
> int pkt_serial;
> @@ -361,7 +362,7 @@ static int filter_nbthreads = 0;
> static int is_full_screen;
> static int64_t audio_callback_time;
>
> -static AVPacket flush_pkt;
> +static uint8_t flush_pkt;
>
> #define FF_QUIT_EVENT (SDL_USEREVENT + 2)
>
> @@ -427,28 +428,25 @@ int64_t get_valid_channel_layout(int64_t channel_layout, int channels)
>
> static int packet_queue_put_private(PacketQueue *q, AVPacket *pkt)
> {
> - MyAVPacketList *pkt1;
> + MyAVPacketList pkt1;
>
> if (q->abort_request)
> return -1;
>
> - pkt1 = av_malloc(sizeof(MyAVPacketList));
> - if (!pkt1)
> - return -1;
> - pkt1->pkt = *pkt;
> - pkt1->next = NULL;
> - if (pkt == &flush_pkt)
> + if (av_fifo_space(q->pkt_list) < sizeof(pkt1)) {
> + if (av_fifo_grow(q->pkt_list, sizeof(pkt1)) < 0)
> + return -1;
> + }
> +
> + if (pkt->data == &flush_pkt)
> q->serial++;
> - pkt1->serial = q->serial;
> + pkt1.pkt = pkt;
> + pkt1.serial = q->serial;
>
> - if (!q->last_pkt)
> - q->first_pkt = pkt1;
> - else
> - q->last_pkt->next = pkt1;
> - q->last_pkt = pkt1;
> + av_fifo_generic_write(q->pkt_list, &pkt1, sizeof(pkt1), NULL);
> q->nb_packets++;
> - q->size += pkt1->pkt.size + sizeof(*pkt1);
> - q->duration += pkt1->pkt.duration;
> + q->size += pkt1.pkt->size + sizeof(pkt1);
> + q->duration += pkt1.pkt->duration;
> /* XXX: should duplicate packet data in DV case */
> SDL_CondSignal(q->cond);
> return 0;
> @@ -456,32 +454,66 @@ static int packet_queue_put_private(PacketQueue *q, AVPacket *pkt)
>
> static int packet_queue_put(PacketQueue *q, AVPacket *pkt)
> {
> + AVPacket *pkt1;
> int ret;
>
> + pkt1 = av_packet_clone(pkt);
> + if (!pkt1)
> + return -1;
> SDL_LockMutex(q->mutex);
> - ret = packet_queue_put_private(q, pkt);
> + ret = packet_queue_put_private(q, pkt1);
> SDL_UnlockMutex(q->mutex);
> -
> - if (pkt != &flush_pkt && ret < 0)
> + if (ret < 0) {
> + av_packet_free(&pkt1);
> av_packet_unref(pkt);
> + }
> +
> + return ret;
> +}
> +
> +static int packet_queue_put_flushpacket(PacketQueue *q)
> +{
> + AVPacket *pkt;
> + int ret;
> +
> + pkt = av_packet_alloc();
> + if (!pkt)
> + return -1;
> + pkt->data = &flush_pkt;
> + SDL_LockMutex(q->mutex);
> + ret = packet_queue_put_private(q, pkt);
> + SDL_UnlockMutex(q->mutex);
> + if (ret < 0)
> + av_packet_free(&pkt);
>
> return ret;
> }
>
> static int packet_queue_put_nullpacket(PacketQueue *q, int stream_index)
> {
> - AVPacket pkt1, *pkt = &pkt1;
> - av_init_packet(pkt);
> - pkt->data = NULL;
> - pkt->size = 0;
> + AVPacket *pkt;
> + int ret;
> +
> + pkt = av_packet_alloc();
> + if (!pkt)
> + return -1;
> pkt->stream_index = stream_index;
> - return packet_queue_put(q, pkt);
> + SDL_LockMutex(q->mutex);
> + ret = packet_queue_put_private(q, pkt);
> + SDL_UnlockMutex(q->mutex);
> + if (ret < 0)
> + av_packet_free(&pkt);
> +
> + return ret;
> }
>
> /* packet queue handling */
> static int packet_queue_init(PacketQueue *q)
> {
> memset(q, 0, sizeof(PacketQueue));
> + q->pkt_list = av_fifo_alloc(sizeof(MyAVPacketList));
> + if (!q->pkt_list)
> + return AVERROR(ENOMEM);
> q->mutex = SDL_CreateMutex();
> if (!q->mutex) {
> av_log(NULL, AV_LOG_FATAL, "SDL_CreateMutex(): %s\n", SDL_GetError());
> @@ -498,16 +530,13 @@ static int packet_queue_init(PacketQueue *q)
>
> static void packet_queue_flush(PacketQueue *q)
> {
> - MyAVPacketList *pkt, *pkt1;
> + MyAVPacketList pkt1;
>
> SDL_LockMutex(q->mutex);
> - for (pkt = q->first_pkt; pkt; pkt = pkt1) {
> - pkt1 = pkt->next;
> - av_packet_unref(&pkt->pkt);
> - av_freep(&pkt);
> + while (av_fifo_size(q->pkt_list) >= sizeof(pkt1)) {
> + av_fifo_generic_read(q->pkt_list, &pkt1, sizeof(pkt1), NULL);
> + av_packet_free(&pkt1.pkt);
> }
> - q->last_pkt = NULL;
> - q->first_pkt = NULL;
> q->nb_packets = 0;
> q->size = 0;
> q->duration = 0;
> @@ -517,6 +546,7 @@ static void packet_queue_flush(PacketQueue *q)
> static void packet_queue_destroy(PacketQueue *q)
> {
> packet_queue_flush(q);
> + av_fifo_freep(&q->pkt_list);
> SDL_DestroyMutex(q->mutex);
> SDL_DestroyCond(q->cond);
> }
> @@ -532,18 +562,29 @@ static void packet_queue_abort(PacketQueue *q)
> SDL_UnlockMutex(q->mutex);
> }
>
> -static void packet_queue_start(PacketQueue *q)
> +static int packet_queue_start(PacketQueue *q)
> {
> + AVPacket *pkt;
> + int ret;
> +
> + pkt = av_packet_alloc();
> + if (!pkt)
> + return -1;
> + pkt->data = &flush_pkt;
> SDL_LockMutex(q->mutex);
> q->abort_request = 0;
> - packet_queue_put_private(q, &flush_pkt);
> + ret = packet_queue_put_private(q, pkt);
> SDL_UnlockMutex(q->mutex);
> + if (ret < 0)
> + av_packet_free(&pkt);
> +
> + return ret;
> }
>
> /* return < 0 if aborted, 0 if no packet and > 0 if packet. */
> static int packet_queue_get(PacketQueue *q, AVPacket *pkt, int block, int *serial)
> {
> - MyAVPacketList *pkt1;
> + MyAVPacketList pkt1;
> int ret;
>
> SDL_LockMutex(q->mutex);
> @@ -554,18 +595,15 @@ static int packet_queue_get(PacketQueue *q, AVPacket *pkt, int block, int *seria
> break;
> }
>
> - pkt1 = q->first_pkt;
> - if (pkt1) {
> - q->first_pkt = pkt1->next;
> - if (!q->first_pkt)
> - q->last_pkt = NULL;
> + if (av_fifo_size(q->pkt_list) >= sizeof(pkt1)) {
> + av_fifo_generic_read(q->pkt_list, &pkt1, sizeof(pkt1), NULL);
> q->nb_packets--;
> - q->size -= pkt1->pkt.size + sizeof(*pkt1);
> - q->duration -= pkt1->pkt.duration;
> - *pkt = pkt1->pkt;
> + q->size -= pkt1.pkt->size + sizeof(pkt1);
> + q->duration -= pkt1.pkt->duration;
> + av_packet_move_ref(pkt, pkt1.pkt);
> if (serial)
> - *serial = pkt1->serial;
> - av_free(pkt1);
> + *serial = pkt1.serial;
> + av_packet_free(&pkt1.pkt);
> ret = 1;
> break;
> } else if (!block) {
> @@ -579,21 +617,28 @@ static int packet_queue_get(PacketQueue *q, AVPacket *pkt, int block, int *seria
> return ret;
> }
>
> -static void decoder_init(Decoder *d, AVCodecContext *avctx, PacketQueue *queue, SDL_cond *empty_queue_cond) {
> +static int decoder_init(Decoder *d, AVCodecContext *avctx, PacketQueue *queue, SDL_cond *empty_queue_cond) {
> memset(d, 0, sizeof(Decoder));
> + d->pkt = av_packet_alloc();
> + d->tmp_pkt = av_packet_alloc();
> + if (!d->pkt || !d->tmp_pkt) {
> + av_packet_free(&d->pkt);
> + av_packet_free(&d->tmp_pkt);
> + return AVERROR(ENOMEM);
> + }
> d->avctx = avctx;
> d->queue = queue;
> d->empty_queue_cond = empty_queue_cond;
> d->start_pts = AV_NOPTS_VALUE;
> d->pkt_serial = -1;
> + return 0;
> }
>
> static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) {
> + AVPacket *pkt = d->tmp_pkt;
> int ret = AVERROR(EAGAIN);
>
> for (;;) {
> - AVPacket pkt;
> -
> if (d->queue->serial == d->pkt_serial) {
> do {
> if (d->queue->abort_request)
> @@ -639,18 +684,18 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) {
> if (d->queue->nb_packets == 0)
> SDL_CondSignal(d->empty_queue_cond);
> if (d->packet_pending) {
> - av_packet_move_ref(&pkt, &d->pkt);
> + av_packet_move_ref(pkt, d->pkt);
> d->packet_pending = 0;
> } else {
> - if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0)
> + if (packet_queue_get(d->queue, pkt, 1, &d->pkt_serial) < 0)
> return -1;
> }
> if (d->queue->serial == d->pkt_serial)
> break;
> - av_packet_unref(&pkt);
> + av_packet_unref(pkt);
> } while (1);
>
> - if (pkt.data == flush_pkt.data) {
> + if (pkt->data == &flush_pkt) {
> avcodec_flush_buffers(d->avctx);
> d->finished = 0;
> d->next_pts = d->start_pts;
> @@ -658,30 +703,31 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) {
> } else {
> if (d->avctx->codec_type == AVMEDIA_TYPE_SUBTITLE) {
> int got_frame = 0;
> - ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &pkt);
> + ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, pkt);
> if (ret < 0) {
> ret = AVERROR(EAGAIN);
> } else {
> - if (got_frame && !pkt.data) {
> + if (got_frame && !pkt->data) {
> d->packet_pending = 1;
> - av_packet_move_ref(&d->pkt, &pkt);
> + av_packet_move_ref(d->pkt, pkt);
> }
> - ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF);
> + ret = got_frame ? 0 : (pkt->data ? AVERROR(EAGAIN) : AVERROR_EOF);
> }
> } else {
> - if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
> + if (avcodec_send_packet(d->avctx, pkt) == AVERROR(EAGAIN)) {
> av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
> d->packet_pending = 1;
> - av_packet_move_ref(&d->pkt, &pkt);
> + av_packet_move_ref(d->pkt, pkt);
> }
> }
> - av_packet_unref(&pkt);
> + av_packet_unref(pkt);
> }
> }
> }
>
> static void decoder_destroy(Decoder *d) {
> - av_packet_unref(&d->pkt);
> + av_packet_free(&d->pkt);
> + av_packet_free(&d->tmp_pkt);
> avcodec_free_context(&d->avctx);
> }
>
> @@ -2685,7 +2731,8 @@ static int stream_component_open(VideoState *is, int stream_index)
> is->audio_stream = stream_index;
> is->audio_st = ic->streams[stream_index];
>
> - decoder_init(&is->auddec, avctx, &is->audioq, is->continue_read_thread);
> + if ((ret = decoder_init(&is->auddec, avctx, &is->audioq, is->continue_read_thread)) < 0)
> + goto fail;
> if ((is->ic->iformat->flags & (AVFMT_NOBINSEARCH | AVFMT_NOGENSEARCH | AVFMT_NO_BYTE_SEEK)) && !is->ic->iformat->read_seek) {
> is->auddec.start_pts = is->audio_st->start_time;
> is->auddec.start_pts_tb = is->audio_st->time_base;
> @@ -2698,7 +2745,8 @@ static int stream_component_open(VideoState *is, int stream_index)
> is->video_stream = stream_index;
> is->video_st = ic->streams[stream_index];
>
> - decoder_init(&is->viddec, avctx, &is->videoq, is->continue_read_thread);
> + if ((ret = decoder_init(&is->viddec, avctx, &is->videoq, is->continue_read_thread)) < 0)
> + goto fail;
> if ((ret = decoder_start(&is->viddec, video_thread, "video_decoder", is)) < 0)
> goto out;
> is->queue_attachments_req = 1;
> @@ -2707,7 +2755,8 @@ static int stream_component_open(VideoState *is, int stream_index)
> is->subtitle_stream = stream_index;
> is->subtitle_st = ic->streams[stream_index];
>
> - decoder_init(&is->subdec, avctx, &is->subtitleq, is->continue_read_thread);
> + if ((ret = decoder_init(&is->subdec, avctx, &is->subtitleq, is->continue_read_thread)) < 0)
> + goto fail;
> if ((ret = decoder_start(&is->subdec, subtitle_thread, "subtitle_decoder", is)) < 0)
> goto out;
> break;
> @@ -2760,7 +2809,7 @@ static int read_thread(void *arg)
> AVFormatContext *ic = NULL;
> int err, i, ret;
> int st_index[AVMEDIA_TYPE_NB];
> - AVPacket pkt1, *pkt = &pkt1;
> + AVPacket *pkt = NULL;
> int64_t stream_start_time;
> int pkt_in_play_range = 0;
> AVDictionaryEntry *t;
> @@ -2777,6 +2826,12 @@ static int read_thread(void *arg)
> memset(st_index, -1, sizeof(st_index));
> is->eof = 0;
>
> + pkt = av_packet_alloc();
> + if (!pkt) {
> + av_log(NULL, AV_LOG_FATAL, "Could not allocate packet.\n");
> + ret = AVERROR(ENOMEM);
> + goto fail;
> + }
> ic = avformat_alloc_context();
> if (!ic) {
> av_log(NULL, AV_LOG_FATAL, "Could not allocate context.\n");
> @@ -2962,15 +3017,15 @@ static int read_thread(void *arg)
> } else {
> if (is->audio_stream >= 0) {
> packet_queue_flush(&is->audioq);
> - packet_queue_put(&is->audioq, &flush_pkt);
> + packet_queue_put_flushpacket(&is->audioq);
> }
> if (is->subtitle_stream >= 0) {
> packet_queue_flush(&is->subtitleq);
> - packet_queue_put(&is->subtitleq, &flush_pkt);
> + packet_queue_put_flushpacket(&is->subtitleq);
> }
> if (is->video_stream >= 0) {
> packet_queue_flush(&is->videoq);
> - packet_queue_put(&is->videoq, &flush_pkt);
> + packet_queue_put_flushpacket(&is->videoq);
> }
> if (is->seek_flags & AVSEEK_FLAG_BYTE) {
> set_clock(&is->extclk, NAN, 0);
> @@ -2986,10 +3041,7 @@ static int read_thread(void *arg)
> }
> if (is->queue_attachments_req) {
> if (is->video_st && is->video_st->disposition & AV_DISPOSITION_ATTACHED_PIC) {
> - AVPacket copy;
> - if ((ret = av_packet_ref(©, &is->video_st->attached_pic)) < 0)
> - goto fail;
> - packet_queue_put(&is->videoq, ©);
> + packet_queue_put(&is->videoq, &is->video_st->attached_pic);
> packet_queue_put_nullpacket(&is->videoq, is->video_stream);
> }
> is->queue_attachments_req = 0;
> @@ -3066,6 +3118,7 @@ static int read_thread(void *arg)
> if (ic && !is->ic)
> avformat_close_input(&ic);
>
> + av_packet_free(&pkt);
> if (ret != 0) {
> SDL_Event event;
>
> @@ -3738,9 +3791,6 @@ int main(int argc, char **argv)
> SDL_EventState(SDL_SYSWMEVENT, SDL_IGNORE);
> SDL_EventState(SDL_USEREVENT, SDL_IGNORE);
>
> - av_init_packet(&flush_pkt);
> - flush_pkt.data = (uint8_t *)&flush_pkt;
> -
> if (!display_disable) {
> int flags = SDL_WINDOW_HIDDEN;
> if (alwaysontop)
> --
> 2.30.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list