[PATCH] qcap: fix recent regression that limited Wine's v4l2 support to devices that read()

[Damjan Jovanovic]

On Sat, Apr 20, 2019 at 6:07 PM Zebediah Figura <z.figura12 at gmail.com>
wrote:

> On 4/20/19 10:56 AM, Damjan Jovanovic wrote:
> >
> >
> > On Sat, Apr 20, 2019 at 5:44 PM Zebediah Figura <z.figura12 at gmail.com
> > <mailto:z.figura12 at gmail.com>> wrote:
> >
> >     On 4/20/19 10:31 AM, Damjan Jovanovic wrote:
> >      >
> >      >
> >      > On Sat, Apr 20, 2019 at 4:52 PM Zebediah Figura
> >     <z.figura12 at gmail.com <mailto:z.figura12 at gmail.com>
> >      > <mailto:z.figura12 at gmail.com <mailto:z.figura12 at gmail.com>>>
> wrote:
> >      >
> >      >     On 4/20/19 8:15 AM, Damjan Jovanovic wrote:
> >      >      > Recent changes to qcap/v4l.c resulted in the removal of
> mmap()
> >      >      > for v4l2 devices, but read() is optional for device
> >     drivers and
> >      >      > some don't support it. This isn't a major problem with the
> >      >      > presense of libv4l2 though, as it can emulate read() on
> top of
> >      >      > mmap(). However the code checks whether the device can
> read(),
> >      >      > and if not, doesn't even try to use it, even though it can.
> >      >      >
> >      >      > Fix this by only warning that read() is being emulated, and
> >      >      > continuing with libv4l2 if available.
> >      >      >
> >      >      > Also clarifies the logging.
> >      >      >
> >      >      > Signed-off-by: Damjan Jovanovic <damjan.jov at gmail.com
> >     <mailto:damjan.jov at gmail.com>
> >      >     <mailto:damjan.jov at gmail.com <mailto:damjan.jov at gmail.com>>>
> >      >      > ---
> >      >      >   dlls/qcap/v4l.c | 13 ++++++++-----
> >      >      >   1 file changed, 8 insertions(+), 5 deletions(-)
> >      >      >
> >      >      >
> >      >      >
> >      >
> >      >     This doesn't seem right; libv4l2 should massage the result of
> >      >     VIDIOC_QUERYCAP to include V4L2_CAP_READWRITE. See
> >      >
> >       <
> https://git.linuxtv.org/v4l-utils.git/tree/lib/libv4l2/libv4l2.c#n1210>.
> >      >
> >      >
> >      > On FreeBSD 11.2 with libv4l-1.6.3_4, that is certainly NOT the
> >     case. It
> >      > could be a newer or not yet released feature.
> >      >
> >      >
> >      >
> >
> >     It's been the case since libv4l2 0.5:
> >     <
> https://git.linuxtv.org/v4l-utils.git/commit/lib/libv4l2/libv4l2.c?id=0215f2ac08c3ad0dc66ad8036f4e186a5e8f56d6
> >
> >
> >     If FreeBSD is using 1.6.3, then I think something else is wrong. Are
> >     you
> >     sure that libv4l2 is actually being loaded?
> >
> >
> > int v4l2_ioctl(int fd, unsigned long int request, ...)
> > {
> > ...
> >          if (devices[index].convert == NULL)
> >                  goto no_capture_request;
> >
> > <your code nippet from above>
> >
> > no_capture_request:
> >
> >
> >
> > So the code that would set that flag, is skipped if the .convert field
> > is NULL.
> >
> > When is it NULL?
> >
> >
> >
>
> If my reading of the code is correct, only when v4l2_fd_open() is called
> manually with V4L2_DISABLE_CONVERSION. Obviously we don't do that.
>
> Perhaps setting LIBV4L2_LOG_FILENAME might give some hints as to what's
> going wrong?
>
>
>
int v4l2_fd_open(int fd, int v4l2_flags)
{
...
<The actual ioctl() returns cap.capabilities = 0x84a00001, cap.device_caps
= 0x04a00000>

        if (cap.capabilities & V4L2_CAP_DEVICE_CAPS)
                cap.capabilities = cap.device_caps;
        if (!(cap.capabilities & V4L2_CAP_VIDEO_CAPTURE) ||
            !(cap.capabilities & (V4L2_CAP_STREAMING |
V4L2_CAP_READWRITE))) {
                goto no_capture;
        }

0x000000001 = V4L2_CAP_VIDEO_CAPTURE
0x00200000 = V4L2_CAP_EXT_PIX_FORMAT
0x00800000 = V4L2_CAP_META_CAPTURE
0x040000000 = V4L2_CAP_STREAMING
0x800000000 = V4L2_CAP_DEVICE_CAPS

So cap.capabilities have all those flags set, but cap.device_caps lacks
V4L2_CAP_VIDEO_CAPTURE.
cap.capabilities is overwritten by cap.device_caps, thus
V4L2_CAP_VIDEO_CAPTURE is cleared.
Since libv4l2 only manages devices with V4L2_CAP_VIDEO_CAPTURE set, and
that flag is now missing, the first condition in the next "if" takes the
goto, and .convert remains NULL.
With .convert NULL, libv4l2 no longer manages the device, so communication
is directly between Wine and kernel.
Therefore the V4L2_CAP_READWRITE flag is not going to be set in
v4l2_ioctl(), and libv4l2 cannot be made to do any conversions for us.
But then again we can't use a device without V4L2_CAP_VIDEO_CAPTURE either,
libv4l2 or not.

I think the fundamental problem is that Wine is trying to use /dev/video1
which is broken, due to a bug in devenum that overwrites the working
/dev/video0 registry entry with it, because their descriptions are the same.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20190420/28c64db3/attachment.html>