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