gstreamer 1.20: v4l2bufferpool spams "newly allocated buffer is not free"

[Michiel Konstapel]

On 30-06-2022 18:04, Nicolas Dufresne via gstreamer-devel wrote:
>
>>
>> I'd be curious to see the value or &pool->buffer_state[group->buffer.index] when
>> that warning triggers. I fail to see how we actually endup in that situation. I
>> wonder if there is a relation with:
>>
>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1087
>>
>> And associated merge request. Did you already tried ?
>>
>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/2007


I've changed the logging to

GST_WARNING_OBJECT (pool, "newly allocated buffer %u is not free: %x",
           group->buffer.index, &pool->buffer_state[group->buffer.index]);

and this prints

gstv4l2bufferpool.c:477:gst_v4l2_buffer_pool_alloc_buffer:<v4l2src0:pool0:src> 
newly allocated buffer 0 is not free: 32b48
v4l2bufferpool 
gstv4l2bufferpool.c:477:gst_v4l2_buffer_pool_alloc_buffer:<v4l2src0:pool0:src> 
newly allocated buffer 1 is not free: 32b4c
v4l2bufferpool 
gstv4l2bufferpool.c:477:gst_v4l2_buffer_pool_alloc_buffer:<v4l2src0:pool0:src> 
newly allocated buffer 2 is not free: 32b50
v4l2bufferpool 
gstv4l2bufferpool.c:477:gst_v4l2_buffer_pool_alloc_buffer:<v4l2src0:pool0:src> 
newly allocated buffer 3 is not free: 32b54

That seems... incorrect. A gint is just an int, so %x is correct, right? 
This has a lot more bits set than are defined in the _GstV4l2BufferState 
enum.


> Looking deeper, it might just a minor bug, and not something as problematic as
> above. I can't say why, but what I believe happens is that after v4l2src pushed
> the buffer, it get release, but it is now invalid:
>
>
> static void
> gst_v4l2_buffer_pool_release_buffer (GstBufferPool * bpool, GstBuffer * buffer)
> {
>    GstV4l2BufferPool *pool = GST_V4L2_BUFFER_POOL (bpool);
>    GstV4l2MemoryGroup *group;
>    gboolean queued = FALSE;
>
>    if (gst_v4l2_is_buffer_valid (buffer, &group)) {
>      gint old_buffer_state =
>          g_atomic_int_and (&pool->buffer_state[group->buffer.index],
>          ~BUFFER_STATE_OUTSTANDING);
>      queued = (old_buffer_state & BUFFER_STATE_QUEUED) != 0;
>      GST_LOG_OBJECT (pool, "mark buffer %u not outstanding",
>          group->buffer.index);
>    }
>
>    gst_v4l2_buffer_pool_complete_release_buffer (bpool, buffer, queued);
> }
>
> Not "valid buffer" simply means it was modified in a way we could not find back
> which GstV4l2MemoryGroup is was associated with. Without the group, we can't
> know the buffer index, hence we can't clear the outstanding state.
>
> Later on, the group will be released to the allocator, which the pool is aware
> as it registered a callback
>
>      pool->group_released_handler =
>          g_signal_connect_swapped (pool->vallocator, "group-released",
>          G_CALLBACK (gst_v4l2_buffer_pool_resurrect_buffer), pool);
>
>
> But the callback only say that "a group" was released, it does not say which
> one. And we have currently no mean to clear the OUTSTANDING flag, which
> eventually leads us to think we just allocated an outstanding buffer. Now that I
> understand how this is possible, it might be possible to fix it. Though I can
> confirm it will have no side effect other then this spammy trace.
>
> Nicolas

That's good to hear! At least then I can move forward with the new 
version :-)
Michiel