[Bug 746529] videoaggregator: Adding support for downstream allocation negotiation

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=746529

Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #299947|none                        |needs-work
             status|                            |

--- Comment #1 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> ---
Review of attachment 299947:
 --> (https://bugzilla.gnome.org/review?bug=746529&attachment=299947)

And there is few missing doc updates.

::: gst-libs/gst/video/gstvideoaggregator.c
@@ +46,3 @@
 GST_DEBUG_CATEGORY_STATIC (gst_videoaggregator_debug);
 #define GST_CAT_DEFAULT gst_videoaggregator_debug
+#define GST_SRC_MIN_BUFFERS 16

That seems like a lot of buffer. Can you explain why an aggregator would need
at least 16 buffers to operate ?

@@ +388,3 @@
+  static const GEnumValue videoaggregator_captureiomode[] = {
+    {VIDEOAGGREGATOR_CAPTUREIOMODE_IMPORT, "Import", "import"},
+    {VIDEOAGGREGATOR_CAPTUREIOMODE_OWN, "Own", "own"},

I'm not sure everyone will like the V4L naming here. Maybe output-mode, or
buffer-mode as we only support one direction ?

@@ +445,3 @@
+  /* Allocation buffers fields */
+  GstBufferPool *pool;
+  gboolean pool_active;

I would recommend using gst_buffer_pool_is_active() instead.

@@ +610,3 @@
+gst_videoaggregator_set_allocation (GstVideoAggregator * vagg,
+    GstBufferPool * pool, GstAllocator * allocator,
+    GstAllocationParams * params, GstQuery * query)

This function looks good. One note, should there be a special case for when the
oldpool == pool ?

@@ +675,3 @@
+
+    /* by default we remove all metadata, subclasses should implement a
+     * filter_meta function */

Shouldn't that code be part of a filter_meta default implementation instead ?

@@ +709,3 @@
+      /* no pool, we can make our own */
+      GST_DEBUG_OBJECT (vagg, "no pool, making new pool");
+      pool = gst_buffer_pool_new ();

As a videoaggregator, it would be better to default to a video buffer pool
which will add VideoMeta when enabled. Though this is slightly cosmetic.

@@ +716,3 @@
+  }
+
+  /* FIXME: */

Fixme are nicer when we know what need fixing.

@@ +718,3 @@
+  /* FIXME: */
+  if (min < GST_SRC_MIN_BUFFERS &&
+      GST_SRC_MIN_BUFFERS < max)

Coding style, I believe this fits on a single line.

@@ +719,3 @@
+  if (min < GST_SRC_MIN_BUFFERS &&
+      GST_SRC_MIN_BUFFERS < max)
+    min = GST_SRC_MIN_BUFFERS;

This seems odd. E.g. if you picked a default pool, min = 1, and max = 0 would
be perfect, I don't see why you need to allocate 16 buffers here.

@@ +727,3 @@
+    config = gst_buffer_pool_get_config (pool);
+    gst_buffer_pool_config_set_params (config, outcaps, size, min, max);
+    gst_buffer_pool_config_set_allocator (config, allocator, &params);

I think it nicer to enable VideoMeta when supported.

@@ +793,3 @@
+  poolconfig = gst_buffer_pool_get_config (vagg->priv->pool);
+
+  gst_buffer_pool_config_get_params(poolconfig, &currentPoolCaps, &size, &min,

Coding style.

@@ +835,3 @@
+   */
+  if (vagg->priv->pool) {
+    result = gst_videoaggregator_check_src_pool(vagg, caps);

Coding style, space before (.

@@ +836,3 @@
+  if (vagg->priv->pool) {
+    result = gst_videoaggregator_check_src_pool(vagg, caps);
+    GST_WARNING_OBJECT(vagg, "Pool already configured %s...",

Coding style.

@@ +859,3 @@
+   * parse them */
+  if (gst_query_get_n_allocation_params (query) > 0) {
+    gst_query_parse_nth_allocation_param (query, 0, &allocator, &params);

This seems to be parsed in set_allocaiton() too.

@@ +2133,3 @@
+  /* Check if we have a pool buffer */
+  if (priv->pool) {
+    if (!priv->pool_active) {

gst_buffer_pool_is_active() would do, but in general, just call _set_active
(pool, TRUE) would do that same.

Stepping back, I think it would be better to do like basesrc here, and activate
the pool in decide_allocation() (or similar function).

@@ +2150,3 @@
+    GST_DEBUG_OBJECT (vagg, "Allocating new buffer of %d and params %"
GST_PTR_FORMAT,
+        outsize,
+        &params);

Style error here, also, I believe we always create a pool, so this case should
not exist.

::: gst-libs/gst/video/gstvideoaggregator.h
@@ +56,3 @@
+ * @VIDEOAGGREGATOR_CAPTUREIOMODE_OWNS:     Use its own allocation buffer
+ *                                          allocation method
+ */

Since marker missing.

@@ +103,3 @@
+ * @decide_allocation:        Setup the allocation parameters for allocating
output
+ *                            buffers. The passed in query contains the result
of the
+ *                            downstream allocation query.

Since marker missing.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.