From: Tim-Philipp Müller <tim@centricular.com>
Date: Fri, 24 Jul 2020 15:14:00 +0000 (+0100)
Subject: rpicamsrc: fix nal alignment of output buffers
X-Git-Tag: 1.19.3~509^2~468
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=9588108d6e548964c07bd1cf0c8a3337ea803155;p=platform%2Fupstream%2Fgstreamer.git

rpicamsrc: fix nal alignment of output buffers

We claim output buffers are nal-aligned, but that wasn't
actually true: We would push out a partial nal in case
the nal doesn't fit into the max encoder-selected output
buffer size, and then the next buffer would not start
with a sync marker. That's not right and makes h264parse
unhappy.

Instead accumulate buffers until we have a full frame
(we can't rely on the NAL_END flag, it's always set).

Fixes #768

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/682>
---

diff --git a/sys/rpicamsrc/RaspiCapture.c b/sys/rpicamsrc/RaspiCapture.c
index 07e234a..c7d3809 100644
--- a/sys/rpicamsrc/RaspiCapture.c
+++ b/sys/rpicamsrc/RaspiCapture.c
@@ -971,11 +971,11 @@ raspi_capture_fill_buffer(RASPIVID_STATE *state, GstBuffer **bufp,
       if (runtime >= offset)
         gst_pts = runtime - offset;
     }
-    GST_LOG ("Buf (uS) PTS %" G_GINT64_FORMAT " DTS %" G_GINT64_FORMAT
-        " STC %" G_GINT64_FORMAT " (latency %" G_GINT64_FORMAT
-        "uS) TS %" GST_TIME_FORMAT,
-        buffer->pts, buffer->dts, param.value, param.value - buffer->pts,
-        GST_TIME_ARGS (gst_pts));
+    GST_LOG ("Buf %05u bytes FLAGS 0x%05x (uS) PTS %" G_GINT64_FORMAT
+        " DTS %" G_GINT64_FORMAT " STC %" G_GINT64_FORMAT
+        " (latency %" G_GINT64_FORMAT "uS) TS %" GST_TIME_FORMAT,
+        buffer->length, buffer->flags, buffer->pts, buffer->dts, param.value,
+        param.value - buffer->pts, GST_TIME_ARGS (gst_pts));
   }
   else {
     GST_LOG ("use-stc=false. Not applying STC to buffer");
@@ -988,7 +988,12 @@ raspi_capture_fill_buffer(RASPIVID_STATE *state, GstBuffer **bufp,
         GST_BUFFER_DTS(buf) = GST_BUFFER_PTS(buf) = gst_pts;
     /* FIXME: Can we avoid copies and give MMAL our own buffers to fill? */
     gst_buffer_fill(buf, 0, buffer->data, buffer->length);
-    ret = GST_FLOW_OK;
+
+    /* NAL_END is bogus and can't be trusted */
+    if ((buffer->flags & MMAL_BUFFER_HEADER_FLAG_FRAME_END))
+      ret = GST_FLOW_OK;
+    else
+      ret = GST_FLOW_KEEP_ACCUMULATING;
   }
 
   mmal_buffer_header_mem_unlock(buffer);
diff --git a/sys/rpicamsrc/RaspiCapture.h b/sys/rpicamsrc/RaspiCapture.h
index d426fed..20e052c 100644
--- a/sys/rpicamsrc/RaspiCapture.h
+++ b/sys/rpicamsrc/RaspiCapture.h
@@ -67,6 +67,7 @@ GST_DEBUG_CATEGORY_EXTERN (gst_rpi_cam_src_debug);
 #define vcos_log_warn GST_WARNING
 
 #define GST_FLOW_ERROR_TIMEOUT GST_FLOW_CUSTOM_ERROR
+#define GST_FLOW_KEEP_ACCUMULATING GST_FLOW_CUSTOM_SUCCESS
 
 G_BEGIN_DECLS
 
diff --git a/sys/rpicamsrc/gstrpicamsrc.c b/sys/rpicamsrc/gstrpicamsrc.c
index ffe175f..a148d41 100644
--- a/sys/rpicamsrc/gstrpicamsrc.c
+++ b/sys/rpicamsrc/gstrpicamsrc.c
@@ -1480,16 +1480,32 @@ gst_rpi_cam_src_create (GstPushSrc * parent, GstBuffer ** buf)
   }
   g_mutex_unlock (&src->config_lock);
 
-  /* FIXME: Use custom allocator */
-  ret = raspi_capture_fill_buffer (src->capture_state, buf, clock, base_time);
-  if (*buf) {
-    GST_LOG_OBJECT (src, "Made buffer of size %" G_GSIZE_FORMAT,
-        gst_buffer_get_size (*buf));
-    /* Only set the duration when we have a PTS update from the rpi encoder.
-     * not every buffer is a frame */
-    if (GST_BUFFER_PTS_IS_VALID (*buf))
-      GST_BUFFER_DURATION (*buf) = src->duration;
-  }
+  *buf = NULL;
+
+  do {
+    GstBuffer *cbuf = NULL;
+
+    /* FIXME: Use custom allocator */
+    ret =
+        raspi_capture_fill_buffer (src->capture_state, &cbuf, clock, base_time);
+
+    if (cbuf != NULL) {
+      GST_LOG_OBJECT (src, "Made buffer of size %" G_GSIZE_FORMAT,
+          gst_buffer_get_size (cbuf));
+
+      if (*buf == NULL) {
+        /* Only set the duration when we have a PTS update from the rpi encoder.
+         * not every buffer is a frame */
+        if (GST_BUFFER_PTS_IS_VALID (cbuf))
+          GST_BUFFER_DURATION (cbuf) = src->duration;
+
+        *buf = cbuf;
+      } else {
+        *buf = gst_buffer_append (*buf, cbuf);
+      }
+      cbuf = NULL;
+    }
+  } while (ret == GST_FLOW_KEEP_ACCUMULATING);
 
   if (ret == GST_FLOW_ERROR_TIMEOUT) {
     GST_ELEMENT_ERROR (src, STREAM, FAILED,
@@ -1498,6 +1514,9 @@ gst_rpi_cam_src_create (GstPushSrc * parent, GstBuffer ** buf)
     ret = GST_FLOW_ERROR;
   }
 
+  if (ret != GST_FLOW_OK)
+    gst_clear_buffer (buf);
+
   if (clock)
     gst_object_unref (clock);
   return ret;