[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 #364660|none                        |needs-work
             status|                            |

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

::: sys/msdk/gstmsdkbufferpool.c
@@ +49,3 @@
+struct _GstMsdkBufferPoolPrivate
+{
+  MsdkContext *context;

If the allocator doesn't need a context, the pool neither

@@ +50,3 @@
+{
+  MsdkContext *context;
+  mfxFrameInfo frame_info;

if the mxfFrameInfo can be generated internally by the allocator suing
GstVideoInfo, there's no need to keep one here

@@ +120,3 @@
+          GST_BUFFER_POOL_OPTION_VIDEO_ALIGNMENT)) {
+    GstVideoAlignment alignment;
+    priv->add_videometa = TRUE;

I don't think this is the right approach. Look at the gstvideopool.c code

The alignment is added if and only if the pool has
GST_BUFFER_POOL_VIDEO_ALIGNMENT option set, AND the GST_BUFFER_VIDEO_META
option set

video meta is not added only if GST_BUFFER_POOL_VIDEO_ALIGNMENT option is set

It would be great if we can asses we can reuse GstVideoBuffer as is, just with
our allocator in the configuration

::: sys/msdk/gstmsdkmemory.c
@@ +44,3 @@
+_init_msdk_memory_debug (void)
+{
+#ifndef GST_DISABLE_GST_DEBUG

since you are not logging anything, why do you create this debug category?

@@ +56,3 @@
+
+static inline void *
+_aligned_alloc (size_t alignment, size_t size)

rather than create a third function, just define posix_memalign() in windows
and use posix_memalign in the code.

@@ +324,3 @@
+
+GstAllocator *
+gst_msdk_allocator_new (MsdkContext * context, mfxFrameInfo * frame_info)

@frame_info can be generated from the data in GstVideoInfo, there is no need to
pass it around. In this way we'll centralize the the mfx frame configuration in
a single point.

@@ +335,3 @@
+    return NULL;
+
+  allocator->context = context;

the context is not a reference-counted structure. Thus I'm afraid that we could
end up with crashes when the context is deleted in a race condition.

we need to rethink the MsdkContext structure, perhaps using GDestroyNotify for
all the subscriptors of the structure.

Anyway, what's the use of the context here? I don't see any.

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