On 01/23/19 10:48, Alexandre Courbot wrote:
> On Wed, Jan 23, 2019 at 5:30 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>
>> There really is no reason why vb2_find_timestamp can't just find
>> buffers in any state. Drop that part of the test.
>>
>> This also means that vb->timestamp should only be set to 0 when a
>> capture buffer is queued AND when the driver doesn't copy timestamps.
>>
>> This change allows for more efficient pipelining (i.e. you can use
>> a buffer for a reference frame even when it is queued).
>
> So I suppose the means the stateless codec API needs to be updated to
> reflect this? I cannot find any case where that would be a problem,
> but just out of curiosity, what triggered this change?
Two reasons, really: one was the discussion about decoding an interlaced
stream where the second field had to refer to the buffer for the first field,
requiring special code in the cedrus driver. With this change you no longer
need to do anything special.
The second was a discussion where it was pointed out that in the current
situation you cannot queue a capture buffer containing a reference frame
that is referred to by a queued, but not yet processed, output buffer.
This means you need to allocate more buffers than is strictly necessary.
The main limitation here was that the timestamp of a capture buffer was
set to 0, so you couldn't find these buffers anymore.
It's easy enough to fix in vb2 and I see no downsides to this change.
Regards,
Hans
>
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>> ---
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 75ea90e795d8..2a093bff0bf5 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -567,7 +567,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, struct vb2_plane *planes)
>> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> unsigned int plane;
>>
>> - if (!vb->vb2_queue->is_output || !vb->vb2_queue->copy_timestamp)
>> + if (!vb->vb2_queue->is_output && !vb->vb2_queue->copy_timestamp)
>> vb->timestamp = 0;
>>
>> for (plane = 0; plane < vb->num_planes; ++plane) {
>> @@ -594,14 +594,9 @@ int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
>> {
>> unsigned int i;
>>
>> - for (i = start_idx; i < q->num_buffers; i++) {
>> - struct vb2_buffer *vb = q->bufs[i];
>> -
>> - if ((vb->state == VB2_BUF_STATE_DEQUEUED ||
>> - vb->state == VB2_BUF_STATE_DONE) &&
>> - vb->timestamp == timestamp)
>> + for (i = start_idx; i < q->num_buffers; i++)
>> + if (q->bufs[i]->timestamp == timestamp)
>> return i;
>> - }
>> return -1;
>> }
>> EXPORT_SYMBOL_GPL(vb2_find_timestamp);
>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>> index a9961bc776dc..8a10889dc2fd 100644
>> --- a/include/media/videobuf2-v4l2.h
>> +++ b/include/media/videobuf2-v4l2.h
>> @@ -59,8 +59,7 @@ struct vb2_v4l2_buffer {
>> * vb2_find_timestamp() - Find buffer with given timestamp in the queue
>> *
>> * @q: pointer to &struct vb2_queue with videobuf2 queue.
>> - * @timestamp: the timestamp to find. Only buffers in state DEQUEUED or DONE
>> - * are considered.
>> + * @timestamp: the timestamp to find.
>> * @start_idx: the start index (usually 0) in the buffer array to start
>> * searching from. Note that there may be multiple buffers
>> * with the same timestamp value, so you can restart the search
