From 2ddf760c091ecaed6d6dcb4131dd2c468453a0da Mon Sep 17 00:00:00 2001 From: Romain Vimont Date: Sun, 11 Apr 2021 15:01:05 +0200 Subject: [PATCH] Make video_buffer more generic The video buffer took ownership of the producer frame (so that it could swap frames quickly). In order to support multiple sinks plugged to the decoder, the decoded frame must not be consumed by the display video buffer. Therefore, move the producer and consumer frames out of the video buffer, and use FFmpeg AVFrame refcounting to share ownership while avoiding copies. --- app/src/decoder.c | 17 ++++++++++-- app/src/decoder.h | 1 + app/src/screen.c | 14 +++++++++- app/src/screen.h | 2 ++ app/src/video_buffer.c | 62 ++++++++++++++++++++---------------------- app/src/video_buffer.h | 41 ++++++++++++---------------- 6 files changed, 77 insertions(+), 60 deletions(-) diff --git a/app/src/decoder.c b/app/src/decoder.c index f05303a3..247b459c 100644 --- a/app/src/decoder.c +++ b/app/src/decoder.c @@ -25,11 +25,20 @@ decoder_open(struct decoder *decoder, const AVCodec *codec) { return false; } + decoder->frame = av_frame_alloc(); + if (!decoder->frame) { + LOGE("Could not create decoder frame"); + avcodec_close(decoder->codec_ctx); + avcodec_free_context(&decoder->codec_ctx); + return false; + } + return true; } void decoder_close(struct decoder *decoder) { + av_frame_free(&decoder->frame); avcodec_close(decoder->codec_ctx); avcodec_free_context(&decoder->codec_ctx); } @@ -41,11 +50,13 @@ decoder_push(struct decoder *decoder, const AVPacket *packet) { LOGE("Could not send video packet: %d", ret); return false; } - ret = avcodec_receive_frame(decoder->codec_ctx, - decoder->video_buffer->producer_frame); + ret = avcodec_receive_frame(decoder->codec_ctx, decoder->frame); if (!ret) { // a frame was received - video_buffer_producer_offer_frame(decoder->video_buffer); + bool ok = video_buffer_push(decoder->video_buffer, decoder->frame); + // A frame lost should not make the whole pipeline fail. The error, if + // any, is already logged. + (void) ok; } else if (ret != AVERROR(EAGAIN)) { LOGE("Could not receive video frame: %d", ret); return false; diff --git a/app/src/decoder.h b/app/src/decoder.h index ba06583e..50dd7fe0 100644 --- a/app/src/decoder.h +++ b/app/src/decoder.h @@ -12,6 +12,7 @@ struct decoder { struct video_buffer *video_buffer; AVCodecContext *codec_ctx; + AVFrame *frame; }; void diff --git a/app/src/screen.c b/app/src/screen.c index 934e418f..de734554 100644 --- a/app/src/screen.c +++ b/app/src/screen.c @@ -378,6 +378,15 @@ screen_init(struct screen *screen, struct video_buffer *vb, return false; } + screen->frame = av_frame_alloc(); + if (!screen->frame) { + LOGC("Could not create screen frame"); + SDL_DestroyTexture(screen->texture); + SDL_DestroyRenderer(screen->renderer); + SDL_DestroyWindow(screen->window); + return false; + } + // Reset the window size to trigger a SIZE_CHANGED event, to workaround // HiDPI issues with some SDL renderers when several displays having // different HiDPI scaling are connected @@ -403,6 +412,7 @@ screen_show_window(struct screen *screen) { void screen_destroy(struct screen *screen) { + av_frame_free(&screen->frame); SDL_DestroyTexture(screen->texture); SDL_DestroyRenderer(screen->renderer); SDL_DestroyWindow(screen->window); @@ -510,7 +520,9 @@ update_texture(struct screen *screen, const AVFrame *frame) { static bool screen_update_frame(struct screen *screen) { - const AVFrame *frame = video_buffer_consumer_take_frame(screen->vb); + av_frame_unref(screen->frame); + video_buffer_consume(screen->vb, screen->frame); + AVFrame *frame = screen->frame; fps_counter_add_rendered_frame(screen->fps_counter); diff --git a/app/src/screen.h b/app/src/screen.h index dca65d41..cd849779 100644 --- a/app/src/screen.h +++ b/app/src/screen.h @@ -36,6 +36,8 @@ struct screen { bool fullscreen; bool maximized; bool mipmaps; + + AVFrame *frame; }; struct screen_params { diff --git a/app/src/video_buffer.c b/app/src/video_buffer.c index d4954afc..18a180fa 100644 --- a/app/src/video_buffer.c +++ b/app/src/video_buffer.c @@ -8,24 +8,22 @@ bool video_buffer_init(struct video_buffer *vb) { - vb->producer_frame = av_frame_alloc(); - if (!vb->producer_frame) { - goto error_0; - } - vb->pending_frame = av_frame_alloc(); if (!vb->pending_frame) { - goto error_1; + return false; } - vb->consumer_frame = av_frame_alloc(); - if (!vb->consumer_frame) { - goto error_2; + vb->tmp_frame = av_frame_alloc(); + if (!vb->tmp_frame) { + av_frame_free(&vb->pending_frame); + return false; } bool ok = sc_mutex_init(&vb->mutex); if (!ok) { - goto error_3; + av_frame_free(&vb->pending_frame); + av_frame_free(&vb->tmp_frame); + return false; } // there is initially no frame, so consider it has already been consumed @@ -36,23 +34,13 @@ video_buffer_init(struct video_buffer *vb) { vb->cbs = NULL; return true; - -error_3: - av_frame_free(&vb->consumer_frame); -error_2: - av_frame_free(&vb->pending_frame); -error_1: - av_frame_free(&vb->producer_frame); -error_0: - return false; } void video_buffer_destroy(struct video_buffer *vb) { sc_mutex_destroy(&vb->mutex); - av_frame_free(&vb->consumer_frame); av_frame_free(&vb->pending_frame); - av_frame_free(&vb->producer_frame); + av_frame_free(&vb->tmp_frame); } static inline void @@ -73,14 +61,24 @@ video_buffer_set_consumer_callbacks(struct video_buffer *vb, vb->cbs_userdata = cbs_userdata; } -void -video_buffer_producer_offer_frame(struct video_buffer *vb) { +bool +video_buffer_push(struct video_buffer *vb, const AVFrame *frame) { assert(vb->cbs); sc_mutex_lock(&vb->mutex); - av_frame_unref(vb->pending_frame); - swap_frames(&vb->producer_frame, &vb->pending_frame); + // Use a temporary frame to preserve pending_frame in case of error. + // tmp_frame is an empty frame, no need to call av_frame_unref() beforehand. + int r = av_frame_ref(vb->tmp_frame, frame); + if (r) { + LOGE("Could not ref frame: %d", r); + return false; + } + + // Now that av_frame_ref() succeeded, we can replace the previous + // pending_frame + swap_frames(&vb->pending_frame, &vb->tmp_frame); + av_frame_unref(vb->tmp_frame); bool skipped = !vb->pending_frame_consumed; vb->pending_frame_consumed = false; @@ -93,19 +91,19 @@ video_buffer_producer_offer_frame(struct video_buffer *vb) { } else { vb->cbs->on_frame_available(vb, vb->cbs_userdata); } + + return true; } -const AVFrame * -video_buffer_consumer_take_frame(struct video_buffer *vb) { +void +video_buffer_consume(struct video_buffer *vb, AVFrame *dst) { sc_mutex_lock(&vb->mutex); assert(!vb->pending_frame_consumed); vb->pending_frame_consumed = true; - swap_frames(&vb->consumer_frame, &vb->pending_frame); - av_frame_unref(vb->pending_frame); + av_frame_move_ref(dst, vb->pending_frame); + // av_frame_move_ref() resets its source frame, so no need to call + // av_frame_unref() sc_mutex_unlock(&vb->mutex); - - // consumer_frame is only written from this thread, no need to lock - return vb->consumer_frame; } diff --git a/app/src/video_buffer.h b/app/src/video_buffer.h index 48e57ff4..cdecb259 100644 --- a/app/src/video_buffer.h +++ b/app/src/video_buffer.h @@ -12,26 +12,23 @@ typedef struct AVFrame AVFrame; /** - * There are 3 frames in memory: - * - one frame is held by the producer (producer_frame) - * - one frame is held by the consumer (consumer_frame) - * - one frame is shared between the producer and the consumer (pending_frame) + * A video buffer holds 1 pending frame, which is the last frame received from + * the producer (typically, the decoder). * - * The producer generates a frame into the producer_frame (it may takes time). + * If a pending frame has not been consumed when the producer pushes a new + * frame, then it is lost. The intent is to always provide access to the very + * last frame to minimize latency. * - * Once the frame is produced, it calls video_buffer_producer_offer_frame(), - * which swaps the producer and pending frames. - * - * When the consumer is notified that a new frame is available, it calls - * video_buffer_consumer_take_frame() to retrieve it, which swaps the pending - * and consumer frames. The frame is valid until the next call, without - * blocking the producer. + * The producer and the consumer typically do not live in the same thread. + * That's the reason why the callback on_frame_available() does not provide the + * frame as parameter: the consumer might post an event to its own thread to + * retrieve the pending frame from there, and that frame may have changed since + * the callback if producer pushed a new one in between. */ struct video_buffer { - AVFrame *producer_frame; AVFrame *pending_frame; - AVFrame *consumer_frame; + AVFrame *tmp_frame; // To preserve the pending frame on error sc_mutex mutex; @@ -42,12 +39,11 @@ struct video_buffer { }; struct video_buffer_callbacks { - // Called when a new frame can be consumed by - // video_buffer_consumer_take_frame(vb) + // Called when a new frame can be consumed. // This callback is mandatory (it must not be NULL). void (*on_frame_available)(struct video_buffer *vb, void *userdata); - // Called when a pending frame has been overwritten by the producer + // Called when a pending frame has been overwritten by the producer. // This callback is optional (it may be NULL). void (*on_frame_skipped)(struct video_buffer *vb, void *userdata); }; @@ -63,13 +59,10 @@ video_buffer_set_consumer_callbacks(struct video_buffer *vb, const struct video_buffer_callbacks *cbs, void *cbs_userdata); -// set the producer frame as ready for consuming -void -video_buffer_producer_offer_frame(struct video_buffer *vb); +bool +video_buffer_push(struct video_buffer *vb, const AVFrame *frame); -// mark the consumer frame as consumed and return it -// the frame is valid until the next call to this function -const AVFrame * -video_buffer_consumer_take_frame(struct video_buffer *vb); +void +video_buffer_consume(struct video_buffer *vb, AVFrame *dst); #endif