GNOME Bugzilla – Bug 741420
video pools: should update size in configuration after applying alignment
Last modified: 2015-02-19 12:32:52 UTC
o Buffer pool add size check when release buffer to buffer pool. Will release the buffer if check fail. o Video buffer size will change if video has padding, the video buffer size will re-calculate in video buffer pool. o Reset size value in video buffer pool to fix the issue.
Created attachment 292586 [details] [review] patch to fix the issue.
What if the implementation wants more then that ?
Video buffer pool will allocate buffer based on info.size which included alignment padding. So the buffer size will be info.size. Video buffer pool user just need set video property, the video buffer size will calculate in video buffer pool. If set different size with info.size, buffer will always be freed when release buffer to buffer pool which isn't our use case expectation. For video buffer pool, video buffer should respect video property. What kind of use case need user set different size with info.size with respect video property? Our use case use video buffer pool with video alignment setting. It works fine on Gstreamer 1.2.3. when porting to 1.4.1, found buffer pool add size checking when release buffer to buffer pool as user set size differ with video buffer size. User set size without video padding, but video buffer pool allocate video buffer size with video padding.
I'll have a closer look later, my comment was because VideoBufferPool is also used as a base class at few places, where the size comes from the HW, hence may have extra padding that aren't part or even related to VideoInfo.size. I want to make sure you have tested and verified the other scenario and not just yours.
Thank you very much. Waiting for your feedback.
Ok, I understand this one is rather critical as it makes a GstVideoPool useless in the context where it has been video aligned. A real life example: source ! avdec_videcodec ! element_with_videopool This will result in continuous reallocation of buffers. Clearly the size should be kept in sync. My only concern is again the _set_config() documentation and the negotiation pattern describe there. I apology for the delay, there is a slightly lack in the design around these. I wonder if GstV4l2BufferPool isn't already doing this behind the scene, as I have not observed this issue, even though the drivers rarely use prescribed alignment.
Ok, this is a wider bug in fact. A well known pipeline is currently affected, every frames get discarded and reallocated: ... ! avdec_h264 ! xvimagesink Same goes for patched version of glbufferpool in bug #740900. V4l2Buffer is most likely affected for OUTPUT device, for capture it bypass default_release_buffer. The problem here is that there might be cases where we don't know the HW alignment before we actually have a buffer allocated.
*** Bug 735386 has been marked as a duplicate of this bug. ***
I'm starting to think the right fix here would be to relax the check, or memorise allocation size upon first allocation. As stated by Arnaud in bug #735386, there might be cases where you don't really know what size is going to be the buffer before you actually have allocated it. This could make sense, since the original intention is to validate that the size is back to normal before it's being stored back into the pool.
/* size should have been reset. This is not a catch all, pool with * size requirement per memory should do their own check. */ if (gst_buffer_get_size (buffer) != pool->priv->size) goto discard; *buffer = gst_buffer_new_allocate (priv->allocator, info->size, &priv->params); 1. gst_buffer_get_size (buffer) will be info->size, not including alignment in &priv->params. 2. pool->priv->size is set by gst_buffer_pool_config_set_params(). So it is easy to keep it same. 3. * @size: the size of each buffer, not including prefix and padding the comments on function: gst_buffer_pool_config_set_params(). is it mean memory padding or video padding? it is mean memory alignment padding. Is my understand right? What kind of use case don't know the acturally size allocated? You mean don't know memory size or the size you set to gst_buffer_new_allocate()?
I see 2 bugs: 1) video-info should update the info->size after alignment. 2) gstvideopool should update the config with the new size after alignment before calling the parent ->set_config With that fixed, it should work fine, I think.
(In reply to comment #11) > 1) video-info should update the info->size after alignment. Actually, this is done correctly because it calls fill_planes with the padded width/height that sets the size.
(In reply to comment #9) > I'm starting to think the right fix here would be to relax the check, or > memorise allocation size upon first allocation. As stated by Arnaud in bug > #735386, there might be cases where you don't really know what size is going to > be the buffer before you actually have allocated it. This could make sense, > since the original intention is to validate that the size is back to normal > before it's being stored back into the pool. I think the size check could be removed. I think you should just restore the size of the buffer to it's original expected size (that you configured in set_config) in the reset_buffer function (or release_buffer). You should, in fact, not only restore the size of the buffer but also any of the metadata on the buffer (in case the metadata is not marked readonly) and possibly the prefix etc. It would be nice if we could mark all buffer metadata as readonly somehow..
commit 8baf1ec50076bd4ea13477875069eb48d7f2a337 Author: Song Bing <b06498@freescale.com> Date: Tue Dec 16 12:10:53 2014 +0100 videopool: update buffer size after video alignment Update the new buffer size after alignment in the pool configuration before calling the parent set_config. This ensures that the parent knows about the buffer size that we will allocate and makes the size check work in the release_buffer method. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=741420
Sorry, reopening, as mention, the XImagePool, the XVImagePool, V4L2BufferPool and possibly OMX buffer pool is affected ;-P
(These don't use VideoPool as a baseclass for various reason)
I updated and checked ximagesink and xvimagesink. commit 8baf1ec50076bd4ea13477875069eb48d7f2a337 Author: Song Bing <b06498@freescale.com> Date: Tue Dec 16 12:10:53 2014 +0100 videopool: update buffer size after video alignment Update the new buffer size after alignment in the pool configuration before calling the parent set_config. This ensures that the parent knows about the buffer size that we will allocate and makes the size check work in the release_buffer method. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=741420
thanks, these are also useful when we'll back port this: I didn't notice you patched ximagepool and xvimagepool too, so never mind. Adding these to the list (will be useful for backport). commit a8d665303757db75da32aebbd2a5d9716e9c528a Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Tue Dec 16 10:13:03 2014 -0500 bufferpool: Don't check size in config validation Pools are allowed to change the size in order to adapt padding. So don't check the size. Normally pool will change the size without failing set_config(), but it they endup changing the size before the validate method may fail on a false positive. https://bugzilla.gnome.org/show_bug.cgi?id=741420 commit 6f136b539951ad9ea75aae608fe63561b43fdd28 Author: Wim Taymans <wtaymans@redhat.com> Date: Tue Dec 16 12:21:59 2014 +0100 bufferpool: log reason for discarded buffers PERFORMANCE log the reason why a buffer could not be recycled in the bufferpool.
commit 2a1459c88fefc6d27cb5b3282efa2179902512cd Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri Dec 19 12:30:03 2014 -0500 v4l2pool: Update configuration size We already update our copy of VideoInfo.size to proper size, now also the configuration so the size matches on release. https://bugzilla.gnome.org/show_bug.cgi?id=741420 Now we just need to backport this.
All patches pushed. So close it.