Created attachment 304174 [details] [review]
Add GstParentBufferMeta

A core meta which helps implement the old concept
of sub-buffering in some situations, by making it
possible for a buffer to keep a ref on a different
parent buffer. The parent buffer is unreffed when
the Meta is freed.

This meta is used to ensure that a buffer whose
memory is being shared to a child buffer isn't freed
and returned to a buffer pool until the memory
is.

(In reply to Jan Schmidt from comment #0)
> We had a problem recently with sharing GstMemory to another buffer. The
> memory gains an extra ref and stays alive fine, but the original parent can
> still be unreffed and returned to a bufferpool (and hence reused) while the
> child buffer is still using it.

This is because your pool is only checking "gst_buffer_is_writable()". It should check for gst_buffer_is_all_memory_writable() writable and discard. That definitely need fixing in that pool implementation. Is it a known one ? Maybe we could add this to the base class ?

Though, I agree a mechanism that would make this more pool friendly would be nice. Right now, a gst_buffer_copy() or any reuse of a memory coming from another buffer, will cause the original buffer to be discarded and reallocated. All this may have the side effect to keep few possibly "unused" memory alive for longer then needed (specially in the case where the buffer has no pool). I would make use of this mechanism conditional to buffer from pool.

Review of attachment 304174 [details] [review]:

::: gst/gstbuffer.c
@@ +2228,3 @@
+ * Returns: The #GstParentBufferMeta that was added to the buffer
+ *
+ * Since: 1.5.1

Should be 1.6, same elsewhere.

@@ +2262,3 @@
+    if (!copy->region) {
+      /* only copy if the complete data is copied as well */
+      dmeta = gst_buffer_add_parent_buffer_meta (dest, smeta->buffer);

I'm not sure that this is right. Looking at gst_buffer_copy_into(), you may have region = TRUE, but still have taken reference on the source buffer memories.

Also, you could have region = FALSE, and not copy over the memory, or have deeped copied that memory. In both cases, it make very little sense to keep the source buffer alive.

(In reply to Nicolas Dufresne (stormer) from comment #2)
> (In reply to Jan Schmidt from comment #0)
> > We had a problem recently with sharing GstMemory to another buffer. The
> > memory gains an extra ref and stays alive fine, but the original parent can
> > still be unreffed and returned to a bufferpool (and hence reused) while the
> > child buffer is still using it.
> 
> This is because your pool is only checking "gst_buffer_is_writable()". It
> should check for gst_buffer_is_all_memory_writable() writable and discard.
> That definitely need fixing in that pool implementation. Is it a known one ?
> Maybe we could add this to the base class ?

Hmmm. Interesting point. The default pool implementation checks gst_buffer_is_all_memory_writable() and discards. I'll retrace my steps and see where this was going wrong for us.

> Though, I agree a mechanism that would make this more pool friendly would be
> nice. Right now, a gst_buffer_copy() or any reuse of a memory coming from
> another buffer, will cause the original buffer to be discarded and
> reallocated. All this may have the side effect to keep few possibly "unused"
> memory alive for longer then needed (specially in the case where the buffer
> has no pool). I would make use of this mechanism conditional to buffer from
> pool.

A good idea - there's no need to keep non-pool parent buffers alive.

Ah yes - the problem with returning to the bufferpool in this case is that the GstMemory is being appended directly to a 2nd buffer - it's not a sub-region via gst_memory_share() that would make the region unwritable. That should be allowed, because the element has received a writable buffer (it's the only owner), and the memory is writable.

If the memory was shared using gst_memory_share(), it would be marked read-only and discarded by the buffer pool as expected.

Jan: When you add a memory to a second buffer, the refcount goes to 2, which makes it non-writable too.

(In reply to Olivier Crête from comment #6)
> Jan: When you add a memory to a second buffer, the refcount goes to 2, which
> makes it non-writable too.

No - I fell for that one too.

A GstMemory is a LOCKABLE miniobject, which doesn't change read/write status based on refcount any more - it only checks the lockstate of the miniobject.

I'm looked through this, and gst_buffer_append_region() is really the only way to reach this situation. It's the only case we can add a GstMemory to a buffer without taking the exclusive lock.

I think I understand that idea, but the gstbuffer.c is incomplete. It should take the lock if the received GstBuffer isn't writable (e.g. you took a ref to keep the second buffer). Otherwise not taking this lock is unsafe.

(In reply to Nicolas Dufresne (stormer) from comment #8)
> I'm looked through this, and gst_buffer_append_region() is really the only
> way to reach this situation. It's the only case we can add a GstMemory to a
> buffer without taking the exclusive lock.
> 
> I think I understand that idea, but the gstbuffer.c is incomplete. It should
> take the lock if the received GstBuffer isn't writable (e.g. you took a ref
> to keep the second buffer). Otherwise not taking this lock is unsafe.

Actually, nevermind, I didn't notice the call to make_writable() initially, changes everything. I don't see what can lead you to a false positive writability.

Outside of that, it remains interesting proposal to get release_buffer() called on pool only when memories are likely to be writeable again (rather then just the GstBuffer object). Would reduce a lot of cases where the pool becomes void because we discard everything.

Created attachment 305112 [details] [review]
Add GstParentBufferMeta

A core meta which helps implement the old concept
of sub-buffering in some situations, by making it
possible for a buffer to keep a ref on a different
parent buffer. The parent buffer is unreffed when
the Meta is freed.

This meta is used to ensure that a buffer whose
memory is being shared to a child buffer isn't freed
and returned to a buffer pool until the memory
is.

In the last patch, I removed the part that discards the meta when only doing partial buffer copies - it's always retained across buffer copies now. It means the parent buffer may stay alive longer than strictly needed, but seems to be the only way to make it work sensibly.

I need this to push the OpenGL stereoscopic handling code, so unless there's objections, I'll merge this patch soon.

Review of attachment 305112 [details] [review]:

It's not ideal, may bring back more starving in fixed size pools, but I can't think of anything better for now. Please, complete the doc before submitting. Most of it is misisng documentation block, and sections.txt has not been filled.

::: gst/gstbuffer.h
@@ -564,0 +564,6 @@
+typedef struct _GstParentBufferMeta GstParentBufferMeta;
+
+struct _GstParentBufferMeta
... 3 more ...

Parent should not be private, also add documentation block.

Probably for 2.0, but we should think of a model where pools tracks what they really care about, GstMemory, and stop worrying about the GstBuffer envelop.

Created attachment 305601 [details] [review]
Add GstParentBufferMeta

A core meta which helps implement the old concept
of sub-buffering in some situations, by making it
possible for a buffer to keep a ref on a different
parent buffer. The parent buffer is unreffed when
the Meta is freed.

This meta is used to ensure that a buffer whose
memory is being shared to a child buffer isn't freed
and returned to a buffer pool until the memory
is.

Pushed as #4d4e43

Thanks !