Hi Ricardo,
Thank you for working on this.
On 17-Dec-24 10:06 PM, Ricardo Ribalda wrote:
> We intend to deprecate the nodrop parameter in the future and adopt the
> default behaviour of the other media drivers: drop invalid packages.
Actually the default behaviour of other media drivers is:
"return buffers with an error to userspace with V4L2_BUF_FLAG_ERROR set
in v4l2_buffer.flags".
It is not "drop invalid packages". The commit messages of patch 1/3
has some related unclear wording, please fix this.
Looking at this I actually have found what arguably is a bug in
the UVC driver when nodrop is set, or at least something which
we must change before making nodrop=1 the default.
Currently uvc_queue_buffer_complete() looks like this:
static void uvc_queue_buffer_complete(struct kref *ref)
{
struct uvc_buffer *buf = container_of(ref, struct uvc_buffer, ref);
struct vb2_buffer *vb = &buf->buf.vb2_buf;
struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
if ((queue->flags & UVC_QUEUE_DROP_CORRUPTED) && buf->error) {
uvc_queue_buffer_requeue(queue, buf);
return;
}
buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
}
Notice how the last line does not propagate buf->error to the
videobuf2 code, so when nodrop=1 is set then buffers with errors
are not only returned to userspace, they are returned to userspace
without V4L2_BUF_FLAG_ERROR getting set in v4l2_buffer.flags .
The right thing to do in this case is to set V4L2_BUF_FLAG_ERROR
IOW the last line of uvc_queue_buffer_complete() should be changed to:
vb2_buffer_done(&buf->buf.vb2_buf, buf->error ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
And this should probably be the first patch in a v2 series for this.
Regards,
Hans
