Created attachment 273020 [details] [review]
Only set TAG_MEMPORY if memory has been replaced

My reasoning was that changing the size of any of the memory objects would need to be detected and undone by the pool in most cases.

If you remove the TAG on resize, you would need to check and reset the memory sizes before releasing the buffer into the pool again (or when you acquire it).

(In reply to comment #2)
> My reasoning was that changing the size of any of the memory objects would need
> to be detected and undone by the pool in most cases.
> 
> If you remove the TAG on resize, you would need to check and reset the memory
> sizes before releasing the buffer into the pool again (or when you acquire it).

When I look at that, the size of a buffer represent the amount of bytes used. When you acquire a buffer from a pool, there is no valid bytes there. Ideally the value should be "zero". For fix size buffer, we could then validate that our expectation match what has been set (something we don't do, and often lead to crash). Though I know this is not how it was thought, I'm just trying to make better sense out of size and maxsize.

The buffer pool already knows the size of the buffers, so it should probably just hard reset the buffers to that size when they come back, no need to involve the memory tag flag if all of the original memories haven't moved.

Just to recap, and correct me if I'm wrong. But the conclusion of our IRC discussion is that because this is TAG_MEMORY, and the Memory have not change, this patch should be fine. Though it could be nice to consider adding something similar, specifc to the fact the size change. When this tag is set, and reset() didn't remove it, we could try doing a resize to original size and finally check the TAG_MEMORY.

Btw, the patch I've attached here break the unit test, just so everyone are aware. A solution I see is:

default_reset() {
  if (!TAG_MEMORY)
    gst_buffer_resize (...)
  }
}

Combine with the change I already attached. This way, if the size changed in a way that cannot be fixed, gst_buffer_resize() will set the TAG_MEMORY, and from there we'll know that there was nothing we could do about it.

Pool holding planar images in multiple memory will definitely want to implement their own resize() (per memory) if they have padding for each memory.

There is one thing I really don't like though, it's the side effect of copying buffer before right before freeing them when NO_SHARE is present. This would become a huge overhead. For this reason, it would be good to check that all no-share enabled pool implement their own reset/resize routine to avoid this.

Went for something much closer to the original idea, only set TAG_MEMORY if memory has been replaced, and do a light check in release to discard buffer which size has not bee set back to default. More complex pool should check each memory individually.

commit ec69ad4e8adddc83d4cdc8c5608099d74f7397d3
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Wed Mar 26 15:56:08 2014 -0400

    buffer: Only set TAG_MEMORY if the memory has been replaced
    
    Currently we set TAG_MEMORY as soon a resize changes the size of one
    of the memory. This has the side effect that buffer pool cannot know if
    the memory have simply been resized, or if the memorys has been replaced.
    This make it hard to actually implement _reset(). Instead, only set the
    TAG_MEMORY if one or more memory has been replaced, and do a light
    sanity check of the size.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=727109

Comment on attachment 273020 [details] [review]
Only set TAG_MEMPORY if memory has been replaced

With light size check added.