[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 #364991|reviewed                    |needs-work
             status|                            |

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

::: sys/msdk/gstmsdkenc.c
@@ +426,3 @@
+    thiz->can_direct_map = FALSE;
+  } else {
+    thiz->can_direct_map = TRUE;

This distinction doesn't make much sense for me. That's something that shall be
handled inside the bufferpool... am I wrong?

@@ +946,3 @@
+  if (GST_IS_MSDK_MEMORY (mem)) {
+    /* This is the case 1.1 */
+    surface = GST_MSDK_MEMORY_CAST (mem)->surface;

why don't use the early return here (goto done), instead of nesting more the
code??

@@ +959,3 @@
+            msdk_get_free_surface (thiz->vpp_surfaces,
thiz->num_vpp_surfaces);
+      else
+        surface = msdk_get_free_surface (thiz->surfaces, thiz->num_surfaces);

all these free surfaces shall be handled, in my opinion, by the bufferpool,
because this is a buffer pool.

The problem here is the use of VPP... perhaps we could move that logic into the
bufferpool.. dunno...

The interesting part here it that we could reuse the input buffer avoiding a
memcopy. For that we could use GstBufferPoolAcquireFlags in
GstBufferPoolAcquireParams saying "don't allocate memory, just grab the
surface"

the number of surface to create are defined in
gst_buffer_pool_config_set_params()

@@ +1204,3 @@
+
+  if (need_pool) {
+    if (thiz->msdk_pool)

don't reuse pool in propose_allocation(), that breaks the renegotiation. create
one every time it is requested. **that's a bug in gstreamer-vaapi

@@ +1206,3 @@
+    if (thiz->msdk_pool)
+      gst_object_unref (thiz->msdk_pool);
+    thiz->msdk_pool = pool;

why this assignation is needed?

@@ +1220,3 @@
+  if (pool) {
+    GstStructure *config;
+    GstAllocationParams params = { 0, 31, 0, 0, };

in the buffer pool you don't use these params.. why do you define them?

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