[Bug 790752] msdk: supports bufferpool

[GStreamer (GNOME Bugzilla)]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #368113|none                        |reviewed
             status|                            |

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

I would like a function to set the video info to the allocator, so it we could
change the resolution of the allocator without reinstantiating it.

::: sys/msdk/gstmsdkbufferpool.c
@@ +95,3 @@
+      && (g_strcmp0 (allocator->mem_type, GST_MSDK_SYSTEM_MEMORY_NAME) != 0
+          && g_strcmp0 (allocator->mem_type,
+              GST_MSDK_VIDEO_MEMORY_NAME) != 0)) {

I've just learned that, as mem_type points to the defined string macro, it more
efficient to use g_str_equal. But it is still safe use g_strcmp0. (I will
change it for gstreamer-vaapi)

@@ +124,3 @@
+      allocator =
+          gst_msdk_video_allocator_new (priv->context, &video_info,
+          priv->alloc_response);

what if the API use pases NULL to this parameters in buffer_pool_new()??
isn't better to fail on set_config() requesting for a real allocator?

::: sys/msdk/gstmsdksystemmemory.c
@@ +137,3 @@
+      &msdk_system_allocator->image_info);
+
+  memcpy (&surface->Info, &frame_info, sizeof (mfxFrameInfo));

is there need of this memcpy??? just fill the structure directly, isn't?

@@ +215,3 @@
+    GstAllocationParams * params)
+{
+  return gst_msdk_system_memory_new (allocator);

you need to pass @size to gst_msdk_system_memory_new() in order to use it in
gst_memory_init(), not the video info size, if the request size is not enough
for the frame it is better to inform the user.

::: sys/msdk/gstmsdkvideomemory.c
@@ +74,3 @@
+      &msdk_video_allocator->image_info);
+
+  memcpy (&surface->Info, &frame_info, sizeof (mfxFrameInfo));

ditto  (no need for memcopy imo)

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