[Bug 747216] applemedia: implement GstAppleCoreVideoMemory

[GStreamer (GNOME Bugzilla)]

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

--- Comment #31 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> ---
(In reply to Ilya Konstantinov from comment #30)
> (In reply to Nicolas Dufresne (stormer) from comment #29)
> > +  gst_apple_core_video_memory_init ();
> 
> I'd have a look at how other allocators work. I copied it from GstGLMemory
> which, I understand, is not the most shining example of how to do things.
>  
> > +  GST_APPLE_CORE_VIDEO_MEMORY_LOCKED_READONLY,
> > +  GST_APPLE_CORE_VIDEO_MEMORY_LOCKED_READ_WRITE
> > +} GstAppleCoreVideoLockState;
> > 
> > Isn't this duplicating GstLockFlags ?
> 
> As comment 12 explains, locking is per-whole-CVPixelBuffer while individual
> Gst*Memory objects are per-plane.
> 
> e.g.
> 1. Plane 1 mmap -> lock the whole CVPixelBuffer
> 2. Plane 2 mmap -> CVPixelBuffer already locked
> 3. Plane 1 unmap -> shouldn't unlock CVPixelBuffer yet
> 4. Plane 2 unmap -> unlock CVPixelBuffer
> 
> Therefore, I must remember, per-whole-CVPixelBuffer, whether it's locked and
> how.
> 
> > @@ +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 ?
> 
> If GstMiniObject maps cleanly to what I'm doing here, then sure, why not.

Ok, let me know.

>  
> > @@ +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)
> 
> It is passed as an argument when a plane is expected, i.e.
> gst_apple_core_video_memory_new_wrapped (gpixbuf,
> GST_APPLE_CORE_VIDEO_NO_PLANE, size))
> 
> It's like:
> Q - Excuse me, what plane are you passing me? 1st? 2nd?
> A - I'm passing you a special plane -- a "no-plane".
> 
> Also, ..._HAS_ONE_PLANE != non-planar.

Agreed.

> 
> > @@ +75,3 @@
> > +  GstAppleCoreVideoPixelBuffer *gpixbuf;
> > +  size_t plane;
> > +} GstAppleCoreVideoMemory;
> > 
> > In fact, I don't understand why this double wrapping.
> 
> Because Gst*Memory is per-plane, see comment 12.

Ok, let's add comments the header, so at first read everyone understand the
why. No one will automatically refer to this bug in the future.

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