GNOME Bugzilla – Bug 733974
gstbufferpool::default_alloc_buffer does not fail if buffer allocation fails
Last modified: 2014-07-30 13:52:19 UTC
Currently do_alloc_buffer() calls default_alloc_buffer() which calls gst_buffer_new_allocate() If gst_buffer_new_allocate() fails to allocate a buffer and returns NULL then default_alloc_buffer() should check for that NULL buffer and return an error which is not the case currently. I am not yet attached a patch since I am not sure what to do. A proposed fix could be: diff --git a/gst/gstbufferpool.c b/gst/gstbufferpool.c index 6509225..5cd5ddc 100644 --- a/gst/gstbufferpool.c +++ b/gst/gstbufferpool.c @@ -227,6 +227,9 @@ default_alloc_buffer (GstBufferPool * pool, GstBuffer ** buffer, *buffer = gst_buffer_new_allocate (priv->allocator, priv->size, &priv->params); + if (!*buffer) + return GST_FLOW_ERROR; + return GST_FLOW_OK; } However the documentation of GStreamer says that elements returning GST_FLOW_ERROR should post an error message wth more details and I am not sure how to do this since GstBufferPool is not really a GstElement.
Created attachment 282048 [details] [review] proposed fix
Review of attachment 282048 [details] [review]: That's true, if the allocator is not the default allocator, it may fail (unlike the system allocator). Thanks for your patch.
commit 59749833bb702d59d7a6c0c6935f388dd602b2f2 Author: Mohammed Sameer <msameer@foolab.org> Date: Wed Jul 30 15:46:22 2014 +0300 bufferpool: Add missing error checking to default_alloc_buffer() default_alloc_buffer() calls gst_buffer_new_allocate() but does not check for failed allocation. This patch makes default_alloc_buffer() return an error (GST_FLOW_ERROR) if buffer allocation fails. https://bugzilla.gnome.org/show_bug.cgi?id=733974 Shall we backport this one ? Was there a failing use case for you ? For the context, this never happens with system allocator, as it will abort the application and other uses their own buffer pool, with the own allocate method.
(In reply to comment #3) > Shall we backport this one ? Was there a failing use case for you ? For the > context, this never happens with system allocator, as it will abort the > application and other uses their own buffer pool, with the own allocate method. I am using a custom allocator which apparently failed for an unknown reason and I ended up hitting this path. I am not sure if anyone else will really hit this issue or not. We already have a few allocators (dma buf and an egl one). If they can fail then I'd guess it'd be better t backport since 1.6 is definitely not in the near future.
(In reply to comment #4) > (In reply to comment #3) > > Shall we backport this one ? Was there a failing use case for you ? For the > > context, this never happens with system allocator, as it will abort the > > application and other uses their own buffer pool, with the own allocate method. > > I am using a custom allocator which apparently failed for an unknown reason and > I ended up hitting this path. I am not sure if anyone else will really hit this > issue or not. > > We already have a few allocators (dma buf and an egl one). If they can fail > then I'd guess it'd be better t backport since 1.6 is definitely not in the > near future. Thanks, I'll look at this. For your interest, there is already a EGL allocator part of gst-plugins-bad, and a dmabuf wrapper allocator part of gst-plugins-base. Any intention to improve this and share the effort ?
In 1.4 now, thanks. commit 57e3797e8891450bef6650862300ce688ee14ebd Author: Mohammed Sameer <msameer@foolab.org> Date: Wed Jul 30 15:46:22 2014 +0300 bufferpool: Add missing error checking to default_alloc_buffer() default_alloc_buffer() calls gst_buffer_new_allocate() but does not check for failed allocation. This patch makes default_alloc_buffer() return an error (GST_FLOW_ERROR) if buffer allocation fails. https://bugzilla.gnome.org/show_bug.cgi?id=733974