[Bug 790752] msdk: supports bufferpool

[GStreamer (GNOME Bugzilla)]

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

Nicolas Dufresne (stormer) <nicolas at ndufresne.ca> changed:

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

--- Comment #4 from Nicolas Dufresne (stormer) <nicolas at ndufresne.ca> ---
Review of attachment 364259:
 --> (https://bugzilla.gnome.org/review?bug=790752&attachment=364259)

Ok, I've seen enough, I'd say just no, this need to be rewritten, it's really
messy. The lazy allocation approach is just completly bad, moving the
allocation hit at run-time rather then at preroll time. This is just a variant
of a system memory allocator. You need to manage it so the GstVideoInfo matches
what you want. As a side effect, combined with appropriate GstAllocationParams
for the alignment, you'll be able to derive from the sysmem allocator, because
it's the same thing. The hidden 32 pixel padding need to be reflected by you
GstVideoInfo, hence your GstVideoMeta (this is not the case).

::: sys/msdk/gstmsdkbufferpool.c
@@ +49,3 @@
+{
+  GstAllocator *allocator;
+  guint options;

Unused ?

@@ +69,3 @@
+  static const gchar *g_options[] = {
+    GST_BUFFER_POOL_OPTION_VIDEO_META,
+    GST_BUFFER_POOL_OPTION_VIDEO_ALIGNMENT,

You should make this a GST_TYPE_VIDEO_BUFFER_POOL, and then drop this code.
Though, you didn't really implement VIDEO_ALIGNMENT.

@@ +109,3 @@
+          GST_BUFFER_POOL_OPTION_VIDEO_META))
+    msdk_pool->add_videometa = TRUE;
+

You are missing gst_video_info_align() call somewhere. You wonder if there is a
way to get it from the VideoPool.

@@ +171,3 @@
+
+  if (!(buf = gst_buffer_new ()))
+    goto no_buffer;

GLib will abort if that fails, remove that check please.

@@ +187,3 @@
+        GST_VIDEO_INFO_WIDTH (info), GST_VIDEO_INFO_HEIGHT (info));
+    vmeta->map = gst_video_meta_map_msdk_memory;
+    vmeta->unmap = gst_video_meta_unmap_msdk_memory;

I strongly discourage using these two functions. Any reason why the map/unmap
from the memory does not work ? Are your offsets wrong maybe ?

@@ +211,3 @@
+      GST_MSDK_BUFFER_POOL_GET_PRIVATE (pool);
+
+  pool->priv = priv;

Unused ?

::: sys/msdk/gstmsdkbufferpool.h
@@ +66,3 @@
+  MsdkContext *context;
+  mfxFrameInfo frame_info;
+  gboolean add_videometa;

Why are those 3 public if you have a private structure ?

::: sys/msdk/gstmsdkmemory.c
@@ +38,3 @@
+#define GST_CAT_DEFAULT gst_debug_msdkvideomemory
+
+#ifndef GST_VIDEO_INFO_FORMAT_STRING

Why would it already be defined ? Namespace pollution caused by copy pasting ?

@@ +48,3 @@
+/* ------------------------------------------------------------------------ */
+/* --- GstMsdkMemory                                              --- */
+/* ------------------------------------------------------------------------ */

This is a bit useless as a comment.

@@ +161,3 @@
+      if (!mem->surface->Data.Locked)
+        return TRUE;
+      g_usleep (10);

What !?! That's horribly wrong, how would design such a backend API like this ?

@@ +195,3 @@
+unmap_msdk_memory (GstMsdkMemory * mem)
+{
+  g_return_if_fail (mem->refcount > 0);

That's not atomic.

@@ +198,3 @@
+
+  if (g_atomic_int_dec_and_test (&mem->refcount)) {
+    /* Release sth */

Why do you care testing, if you don't do anything here ?

@@ +199,3 @@
+  if (g_atomic_int_dec_and_test (&mem->refcount)) {
+    /* Release sth */
+  }

GstMemory already implements this correctly, respecting read/write rules, why
do you add this layer in top ?

@@ +284,3 @@
+  gst_video_info_init (&mem->surface_info);
+  gst_video_info_set_format (&mem->surface_info, vip->finfo->format,
+      GST_ROUND_UP_32 (vip->width), GST_ROUND_UP_32 (vip->height));

This padding is completly random, you need the GstVideoInfo to reflect this.

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