GNOME Bugzilla – Bug 776936
v4l2videodec: bufferpool size is fixed to 2
Last modified: 2017-03-05 15:10:55 UTC
When the videodec element activates the v4l2 bufferpool, it sets the size of the input internal pool to a fixed value of 2. However, the videodec element already configured the bufferpool size according to the minimum required by the v4l2 driver. Instead of using the fixed value, the videodec element should ask the bufferpool for its configured size and use that one.
Created attachment 343003 [details] [review] v4l2videodec: Use negotiated bufferpool size when activating pool The bufferpool size is already negotiated when setting the format on the v4l2 object including the minimum bufferpool size required by the v4l2 driver. Do not use a fixed bufferpool size of 2, but ask the bufferpool for its configured size and use it.
Review of attachment 343003 [details] [review]: In the particular case, the buffers will be copied, there is no need to respect the negotiated number of buffers. I agree it should respect the minimum suggested by the driver though. On the other end, it's driver role to enforce this minimum through when REQBUFS is called. Maybe I'm missing something, it would be nice to clarify.
(In reply to Nicolas Dufresne (stormer) from comment #2) > Review of attachment 343003 [details] [review] [review]: > > In the particular case, the buffers will be copied, there is no need to > respect the negotiated number of buffers. I agree it should respect the > minimum suggested by the driver though. On the other end, it's driver role > to enforce this minimum through when REQBUFS is called. Maybe I'm missing > something, it would be nice to clarify. The minimum was OK, but I was more concerned about the maximum, but I missed that the buffers are copied. I now understand that the V4l2Allocator maintains an internal queue and that this prevents the allocation of additional buffers in the buffer pool. Is this correct? Maybe a comment that explains the 2 or using GST_V4L2_MIN_BUFFERS instead of the 2 would be helpful. Anyway, thanks for the clarification. This patch is not needed.