[Bug 747216] applemedia: implement GstAppleCoreVideoMemory

[GStreamer (GNOME Bugzilla)]

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

Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> changed:

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

--- Comment #29 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> ---
Review of attachment 305435:
 --> (https://bugzilla.gnome.org/review?bug=747216&attachment=305435)

You'll see bellow few comments. I've didn't do deeper in the code yet. But I
have the impression that you duplicate some of the stuff that we already
implemented within GStreamer. There is a strange double wrapping of the
CVPixelBuffer, while we designed GstMemory and GstMiniObject specifically so it
can be a thing and direct wrapper for you need.

I also think that you skipped an important step, deriving GstAllocator. This
would remove the weird init() spread across the code, and following this API
will provide with API for downstream element to detect if a GstMemory is a
GstAppleCoreVideoMemory.

::: sys/applemedia/corevideobuffer.c
@@ +69,1 @@
+  gst_apple_core_video_memory_init ();

Is that some kind of library init ? It's seems strange to have to call this
randomly.

::: sys/applemedia/corevideomemory.h
@@ +32,3 @@
+  GST_APPLE_CORE_VIDEO_MEMORY_LOCKED_READONLY,
+  GST_APPLE_CORE_VIDEO_MEMORY_LOCKED_READ_WRITE
+} GstAppleCoreVideoLockState;

Isn't this duplicating GstLockFlags ?

@@ +53,3 @@
+   * or for reading and writing, as reflected in @lock_state. */
+  guint lock_count;
+} GstAppleCoreVideoPixelBuffer;

Why not implementing a GstMiniObject, which already handle the this locking ?

@@ +60,3 @@
+ * Indicates a non-planar pixel buffer.
+ */
+#define GST_APPLE_CORE_VIDEO_NO_PLANE ((size_t)-1)

non-planar and NO_PLANE does not fully sound the same to me, can we use a
better name ? (I could propose GST_APPLE_CORE_VIDEO_HAS_ONE_PLANE)

@@ +75,3 @@
+  GstAppleCoreVideoPixelBuffer *gpixbuf;
+  size_t plane;
+} GstAppleCoreVideoMemory;

In fact, I don't understand why this double wrapping.

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