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

@@ -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,
+    ret = avcodec_receive_frame(decoder->codec_ctx, decoder->frame);
-                                decoder->video_buffer->producer_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;

app/src/decoder.h

@@ -12,6 +12,7 @@ struct decoder {
     struct video_buffer *video_buffer;
 
     AVCodecContext *codec_ctx;
+    AVFrame *frame;
 };
 
 void

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);
 

app/src/screen.h

@@ -36,6 +36,8 @@ struct screen {
     bool fullscreen;
     bool maximized;
     bool mipmaps;
+
+    AVFrame *frame;
 };
 
 struct screen_params {

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();
+    vb->tmp_frame = av_frame_alloc();
-    if (!vb->consumer_frame) {
+    if (!vb->tmp_frame) {
-        goto error_2;
+        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
+bool
-video_buffer_producer_offer_frame(struct video_buffer *vb) {
+video_buffer_push(struct video_buffer *vb, const AVFrame *frame) {
     assert(vb->cbs);
 
     sc_mutex_lock(&vb->mutex);
 
-    av_frame_unref(vb->pending_frame);
+    // Use a temporary frame to preserve pending_frame in case of error.
-    swap_frames(&vb->producer_frame, &vb->pending_frame);
+    // 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 *
+void
-video_buffer_consumer_take_frame(struct video_buffer *vb) {
+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_move_ref(dst, vb->pending_frame);
-    av_frame_unref(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;
 }

app/src/video_buffer.h

@@ -12,26 +12,23 @@
 typedef struct AVFrame AVFrame;
 
 /**
- * There are 3 frames in memory:
+ * A video buffer holds 1 pending frame, which is the last frame received from
- *  - one frame is held by the producer (producer_frame)
+ * the producer (typically, the decoder).
- *  - one frame is held by the consumer (consumer_frame)
- *  - one frame is shared between the producer and the consumer (pending_frame)
  *
- * 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(),
+ * The producer and the consumer typically do not live in the same thread.
- * which swaps the producer and pending frames.
+ * 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
- * When the consumer is notified that a new frame is available, it calls
+ * retrieve the pending frame from there, and that frame may have changed since
- * video_buffer_consumer_take_frame() to retrieve it, which swaps the pending
+ * the callback if producer pushed a new one in between.
- * and consumer frames. The frame is valid until the next call, without
- * blocking the producer.
  */
 
 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
+    // Called when a new frame can be consumed.
-    // video_buffer_consumer_take_frame(vb)
     // 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
+bool
-void
+video_buffer_push(struct video_buffer *vb, const AVFrame *frame);
-video_buffer_producer_offer_frame(struct video_buffer *vb);
 
-// mark the consumer frame as consumed and return it
+void
-// the frame is valid until the next call to this function
+video_buffer_consume(struct video_buffer *vb, AVFrame *dst);
-const AVFrame *
-video_buffer_consumer_take_frame(struct video_buffer *vb);
 
 #endif