[Bug 793705] msdk: Implement Video Post Processing element

[GStreamer (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <vjaquez at igalia.com> changed:

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

--- Comment #15 from Víctor Manuel Jáquez Leal <vjaquez at igalia.com> ---
Review of attachment 370068:
 --> (https://bugzilla.gnome.org/review?bug=793705&attachment=370068)

::: sys/msdk/Makefile.am
@@ +23,3 @@
     gstmsdk.c \
+        msdk-enums.c \
+        gstmsdkvpputil.c

nitpick: spaces vs tabs :)

@@ +51,3 @@
+    gstmsdkenc.h \
+    gstmsdkvpp.h \
+        gstmsdkvpputil.h

ditto

::: sys/msdk/gstmsdkvpp.c
@@ +58,3 @@
+        "width = (int) [ 1, MAX ], height = (int) [ 1, MAX ],"
+        "interlace-mode = (string) progressive")
+    );

Isn't better use

GST_VIDEO_CAPS_MAKE ("{ NV12, I420, YUY2, UYVY, BGRA }") ", interlace-mode =
(string) progressive" ??

@@ +69,3 @@
+        "width = (int) [ 1, MAX ], height = (int) [ 1, MAX ],"
+        "interlace-mode = (string) progressive")
+    );

Ditto

@@ +83,3 @@
+
+#define gst_msdkvpp_parent_class parent_class
+G_DEFINE_TYPE (GstMsdkVPP, gst_msdkvpp, GST_TYPE_BASE_TRANSFORM);

If msdkvpp is not going to double the frame-rate (two output frames per one
input) when deinterlacing, perhaps it would make sense to inherit from
GstVideoFilter

@@ +239,3 @@
+    goto error_pool_config;
+
+  *aligned_info = info;

I would rename "aligned_info" with "pool_info"

and add a comment before updating, something like:

/* updating pool info with allocator's info */

@@ +315,3 @@
+    pool =
+        gst_msdkvpp_create_buffer_pool (thiz, GST_PAD_SRC, caps, min_buffers);
+    thiz->srcpad_buffer_pool = pool;

AFAIU, this assignation should be moved out of this block. What would happen if
we reuse downstream pool?

@@ +339,3 @@
+  if (caps)
+    gst_caps_unref (caps);
+

Yup: caps in gst_query_parse_allocation() are not transfered:
https://developer.gnome.org/gstreamer/stable/gstreamer-GstQuery.html#gst-query-parse-allocation

@@ +381,3 @@
+  }
+
+  pool = thiz->sinkpad_buffer_pool;

AFAIU, this is not OK. Every time upstream ask for a pool, we should provide a
new instantiated pool. Consider tees or similars.

@@ +437,3 @@
+
+static MsdkSurface *
+get_msdk_surface_from_input_buffer (GstMsdkVPP * thiz, GstBuffer * inbuf)

This function really needs to be refactored and moved in to a common utility
source file.

@@ +515,3 @@
+    if (status != MFX_WRN_DEVICE_BUSY)
+      break;
+    /* If device is busy, wait 1ms and retry, as per MSDK's recomendation */

Captain grammar here: "recommendation" :D

@@ +533,3 @@
+    status = MFX_ERR_NONE;
+
+  gst_buffer_copy_into (outbuf, inbuf, GST_BUFFER_COPY_TIMESTAMPS, 0, -1);

Keep in mind if it is required to copy, not only timestamps, but flags (sync)
and non-memory metadata (such as ROI metadata)

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