Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Capture DQBUF ioctl returns wrong buffer #304

Open
MikePlayle opened this issue May 3, 2020 · 12 comments
Open

Capture DQBUF ioctl returns wrong buffer #304

MikePlayle opened this issue May 3, 2020 · 12 comments

Comments

@MikePlayle
Copy link

I'm having a range of problems with v4l2loopback and so today I started trying to write a basic test harness for it so I could investigate them, however I'm falling at the first hurdle.

I've attached a fairly minimal program which provokes the DQBUF ioctl to return an illegal buffer ID which should be owned by userspace already. With top-of-tree v4l2loopback (as of this morning) I get the following output from it:

Source  QBUF index 0
Sink    QBUF index 0
Sink   DQBUF index 0
Sink    QBUF index 1
Source  QBUF index 1
Sink   DQBUF index 1
Sink    QBUF index 0
Source DQBUF index 1
Source  QBUF index 1
Sink   DQBUF index 1
a.out: minimal.c:132: opener_dqbuf: Assertion `owned_by_kernel(t, buf.index)' failed.
Aborted

minimal.c.txt

The "Source" lines are feeding the loopback device's output side, and the "Sink" lines are reading from the associated capture side. Here the capture side has returned buffer index 1, where according to my reading of the documentation, it should have returned buffer 0 which was previously queued for it.

(If I get anywhere with this test harness I'll be happy to upstream it, if it's of any interest to the project.)

@MikePlayle MikePlayle changed the title Output DQBUF ioctl returns wrong buffer Capture DQBUF ioctl returns wrong buffer May 3, 2020
@umlaeute
Copy link
Owner

umlaeute commented May 4, 2020

If I get anywhere with this test harness I'll be happy to upstream

totally. bug fixes are always welcome :-)

sidenote: rather than using time/space relative versions (e.g. "top-of-tree v4l2loopback (as of this morning)"), i'd prefer if you used the actual commit (e.g. d3f15e3) which still can be identified in 3 months.

@MikePlayle
Copy link
Author

Yes, that's the commit I meant, and yes, I should have said that. Sorry.

@dumblob
Copy link

dumblob commented Sep 18, 2020

@umlaeute it seems we've found a similar (same?) issue when trying to stream from v4l2loopback using ustreamer. ustreamer dumps core as it has an assert for a case when it decides to skip a frame ("return a buffer").

Reproducer is very simple:

  1. create v4l2loopback device (v4l2loopback 0.12.5 compiled using DKMS for Linux 5.4.61-1-lts)
  2. point ffmpeg output to it like this: ffmpeg -f x11grab -r 30 -s 1680x1050 -i :1.0+0,0 -vcodec rawvideo -pix_fmt rgb565 -threads 4 -f v4l2 /dev/video2
  3. run ustreamer with more than one thread worker: ustreamer -w 4 -q 97 -m RGB565 -r 1680x1050 -d /dev/video2

(if the reproducer won't work for you as is, try higher frame rate or higher resolution or change rgb565 to rgb24 as chances are you have too beefy machine 😉)

More details in pikvm/ustreamer#43 .

Any ideas why v4l2loopback returns the previous buffer before the skipped buffer? How could this surprising (wrong?) behavior be tamed?

@mdevaev
Copy link

mdevaev commented Sep 18, 2020

For reproducing I recommend to checkout ustreamer at this commit: https://github.com/pikvm/ustreamer/tree/278645ce5104e5d6c8e39f238d8a13199b12cfe2

It adds conditions and the error message, which clearly indicates that the buffer is being dequeued twice.

Master branch of ustreamer is fine for this now.

@alexhartl
Copy link

In fact, on the capture side the QBUF ioctl doesn't have any effect currently. It sets V4L2_BUF_FLAG_QUEUED, but as far as I see, the QUEUED and DONE flags are not used anywhere. When both output and capture side use the QBUF/DQBUF API, directly setting these flags seems to not make a lot sense anyway, because there's only one buffers array for both capture side and output side.

I think if both capture and output side use the QBUF/DQBUF API, it would be unavoidable to have separate queues for capture and output side and do some memory copying, since you can't be sure that a certain index that is provided by the output side has been queued by the capture side.

I'm experiencing this issue with the Linphone SIP client, which uses mediastreamer2. Mediastreamer2 uses the returned buffer indices for its own frame management, so dequeueing buffers that have not been queued results in problems. In my case, the problem can be worked around by increasing max_buffers to 4.

@movepointsolutions
Copy link

Hi everyone,
I've created branch https://github.com/movepointsolutions/v4l2loopback/tree/queuing
From my vision, the whole bufpos2index machinery is broken.
dev->used_buffers is currently set to min(opener->buffers_number)
This actually breaks the whole point of multi-reader case.
I'm diving into the issue, at least minimal.c being broken, is crazy.

@movepointsolutions
Copy link

Does it make sense to allow working in-kernel buffer of frames?
This way, regardless of output/capture mode of opener, its requested buffers
shall map to max_buffers, and cycle through them.

@movepointsolutions
Copy link

movepointsolutions commented Sep 4, 2023

requested buffers shall map to max_buffers, and cycle through them.

this works only for userptr mode, I think.
Is there way to fill exactly max_buffers (and rename it), and do some magic for mmap mode?
Because for userptr this involves full copying of frames, and userptr mode doesn't care how buffers are stored in kernel.

@movepointsolutions
Copy link

movepointsolutions commented Sep 4, 2023

The plain approach is to rename max_buffers to n_buffers
And make reqbufs always return its value for mmap I/O mode, regardless of what the userspace requested.

@movepointsolutions
Copy link

The details could be different,
but to fix this issue cleanly,
I suggest that we force all mmap openers to use the same number of buffers,
which would map to internal storage in a simple way.
Then I would implement #146

@movepointsolutions
Copy link

https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/mmap.html#mmap
says we can force userspace to use specific number of buffers,
regardless of is it being less or more than it requested

@samlii
Copy link

samlii commented Jun 8, 2024

Just wondering if there has been any movement on this front recently?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants