Re: [PATCH 1/6] qcap/videocapture: Storage capabilities information.

[Zebediah Figura]

On 4/23/20 12:28 AM, Jactry Zeng wrote:
> Signed-off-by: Jactry Zeng
>
> dlls/qcap/v4l.c | 158 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 155 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/qcap/v4l.c b/dlls/qcap/v4l.c
> index 6fe92d2d0a..26eccf6ee3 100644
> --- a/dlls/qcap/v4l.c
> +++ b/dlls/qcap/v4l.c
> @@ -93,9 +93,20 @@ static BOOL video_init(void)
>  #endif
>  }
>
> +struct capabilitie

"capability", but "caps" would be shorter and also sensible.
> +{
> - __u32 pixelformat;
> - UINT width, height, depth;
> - AM_MEDIA_TYPE media_type;
> - VIDEO_STREAM_CONFIG_CAPS config;
>
> +};
> -
>
> struct _Capture
>  {
>      UINT width, height, bitDepth, fps, outputwidth, outputheight;
> - struct capabilitie *current_cap;

This can be a const pointer, right?
> - struct capabilitie **caps;

This could be an array of structs rather than an array of pointers,
right? I suspect that'd end up being simpler.
> -
>
> LONG caps_count;
> BOOL swresize;
>
> struct strmbase_source *pin;
>
> @@ -116,13 +127,22 @@ static int xioctl(int fd, int request, void * arg)
>      return r;
>  }
>
> +static void free_capabilitie(struct capabilitie *cap)
> +{
> - heap_free(cap->media_type.pbFormat);
> - heap_free(cap);
>
> +}
> -
>
> HRESULT qcap_driver_destroy(Capture *capBox)
>  {
>      TRACE("%p\n", capBox);
> - if( capBox->fd != -1 )
> - if (capBox->fd != -1)
>     video_close(capBox->fd);
> - CoTaskMemFree(capBox);
> - while (--capBox->caps_count >= 0)
> -
>    free_capabilitie(capBox->caps[capBox->caps_count]);
> - heap_free(capBox);
> - return S_OK;
>
> }
>
> @@ -495,9 +515,83 @@ void qcap_driver_cleanup_stream(Capture *device)
>          ERR("Failed to decommit allocator, hr %#x.\n", hr);
>  }
> -
>
> +static HRESULT create_capabilitie(__u32 pixelformat, __u32 width, __u32 height,
> -
>    __u32 max_fps, __u32 min_fps, struct capabilitie **out)
>
> +{
> - VIDEOINFOHEADER *video_info;
> - struct capabilitie *cap;
> - DWORD compression;
> - BOOL compressed;
> - GUID subtype;
> - LONG depth;
> -
> - *out = NULL;
> - switch (pixelformat)
> - {
> -
>    case V4L2_PIX_FMT_BGR24:
> -
>        subtype = MEDIASUBTYPE_RGB24;
> -
>        compression = BI_RGB;
> -
>        compressed = FALSE;
> -
>        depth = 24;
> -
>        break;
> -
>    default:
> -
>        FIXME("Unknown format: %#x (%dx%d).\n", pixelformat, width, height);
> -
>        return E_FAIL;
> - }

So in general we can always rely on libv4l2 to expose V4L2_PIX_FMT_BGR24
(unless the native format is something really bizarre).

One conclusion to draw from this is that we don't even need to worry
about enumerating other formats, we can just set frmsize.pixel_format
below to V4L2_PIX_FMT_BGR24 and skip VIDIOC_ENUM_FMT entirely. That's
not necessarily saying we should, but it seems potentially easier and
it's not clear we'll run into any problems thereby. (It doesn't seem
particularly worthwhile to me to work around missing libv4l2.)

(At least, I'm assuming this patch series wasn't motivated by the need
to expose formats other than MEDIASUBTYPE_RGB24 from the capture filter,
since there's no support introduced in this series at least. If that's
not the case, feel free to ignore.)

Otherwise, it seems like at least a good idea to downgrade that FIXME to
a WARN. Currently this patch introduces a long list of FIXME messages
for formats that my camera (and libv4l2) supports, that we don't really
care about.
> -
> - cap = heap_alloc_zero(sizeof(*cap));
> - if (!cap)
> -
>    return E_OUTOFMEMORY;
> -
> - video_info = heap_alloc_zero(sizeof(*video_info));
> - if (!video_info)
> - {
> -
>    heap_free(cap);
> -
>    return E_OUTOFMEMORY;
> - }

I think we could just put the VIDEOINFOHEADER inline in "struct
capabilitie", and obviate this extra allocation.
> -
> - video_info->rcSource.right = width;
> - video_info->rcSource.bottom = height;
> - video_info->rcTarget.right = width;
> - video_info->rcTarget.bottom = height;

The Windows driver for my camera (an Anivia W5; 0c45:6366) doesn't set
these rects, and in general I don't think there's a reason to.
> - video_info->dwBitRate = max_fps * width * height * depth;
> - video_info->bmiHeader.biSize = sizeof(video_info->bmiHeader);
> - video_info->bmiHeader.biWidth = width;
> - video_info->bmiHeader.biHeight = height;
> - video_info->bmiHeader.biPlanes = 1;
> - video_info->bmiHeader.biBitCount = depth;
> - video_info->bmiHeader.biCompression = compression;
> - video_info->bmiHeader.biSizeImage = width * height * depth / 8;
> - cap->media_type.majortype = MEDIATYPE_Video;
> - cap->media_type.subtype = subtype;
> - cap->media_type.bFixedSizeSamples = TRUE;
> - cap->media_type.bTemporalCompression = compressed;

Probably "temporal_compression" (since it's not equivalent to just
asking whether the stream is compressed—e.g. MJPG is compressed but not
temporally.)
> - cap->media_type.lSampleSize = width * height * depth / 8;
> - cap->media_type.formattype = FORMAT_VideoInfo;
> - cap->media_type.pUnk = NULL;
> - cap->media_type.cbFormat = sizeof(VIDEOINFOHEADER);
> - cap->media_type.pbFormat = (void *)video_info;
> - cap->config.MaxFrameInterval = 10000000 * max_fps;
> - cap->config.MinFrameInterval = 10000000 * min_fps;;
> - cap->config.MaxOutputSize.cx = width;
> - cap->config.MaxOutputSize.cy = height;
> - cap->config.MinOutputSize.cx = width;
> - cap->config.MinOutputSize.cy = height;
> - cap->config.guid = FORMAT_VideoInfo;

It'd probably be a good idea to set {Min,Max}CroppingSize here too.
Specifying granularity and alignment of 1 may not be a bad idea either.

We should be able to derive {Min,Max}BitsPerSecond from the other fields
as well.
> - cap->pixelformat = pixelformat;
> - cap->width = width;
> - cap->height = height;
> - cap->depth = depth;

I don't particularly object to these being separate fields, but they are
basically duplicating BITMAPINFOHEADER fields (which, if you move
VIDEOINFOHEADER inline into struct capabilitie, is reasonably accessible.)
> -
> - *out = cap;
> - return S_OK;
>
> +}
> -
>
> Capture *qcap_driver_init(struct strmbase_source *pin, USHORT card)
>  {
>      struct v4l2_capability caps = {{0}};
> - struct v4l2_fmtdesc fmtdesc = {0};
> struct v4l2_format format = {0};
> Capture *device = NULL;
> BOOL have_libv4l2;
>
> @@ -506,7 +600,7 @@ Capture *qcap_driver_init(struct strmbase_source *pin, USHORT card)
>  have_libv4l2 = video_init();
>
> - if (!(device = CoTaskMemAlloc(sizeof(*device))))
> -
>
> if (!(device = heap_alloc_zero(sizeof(*device))))
>     return NULL;
>
> sprintf(path, "/dev/video%i", card);
>
> @@ -559,6 +653,64 @@ Capture *qcap_driver_init(struct strmbase_source *pin, USHORT card)
>          goto error;
>      }
> - /* Feeding a RGB format to VIDIOC_TRY_FMT is helpful and necessary for
> -
>   making VIDIOC_ENUM_FMT return more supproted RGB formats. */

Where is this coming from? Does a device you've tested with refuse to
enumerate some formats until it receives VIDIOC_TRY_FMT? Is this
documented somewhere? It's not obvious to me that it is, and if that is
happening it seems like potentially a kernel bug.
> - format.fmt.pix.pixelformat = V4L2_PIX_FMT_BGR24;
> - if (xioctl(fd, VIDIOC_TRY_FMT, &format) == -1
> -
>        || format.fmt.pix.pixelformat != V4L2_PIX_FMT_BGR24)
> -
>    WARN("This device doesn't support V4L2_PIX_FMT_BGR24.\n");
> -
> - fmtdesc.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> - while(xioctl(fd, VIDIOC_ENUM_FMT, &fmtdesc) != -1)
> - {
> -
>    struct v4l2_frmsizeenum frmsize = {0};
> -
> -
>    frmsize.pixel_format = fmtdesc.pixelformat;
> -
>    while(xioctl(fd, VIDIOC_ENUM_FRAMESIZES, &frmsize) != -1)
> -
>    {
> -
>        struct v4l2_frmivalenum frmival = {0};
> -
>        struct capabilitie *cap, **new_caps;
> -
>        __u32 max_fps = 30, min_fps = 30;
> -
> -
>        frmival.pixel_format = fmtdesc.pixelformat;
> -
>        frmival.width = frmsize.discrete.width;
> -
>        frmival.height = frmsize.discrete.height;

This should at least check frmsize.type first—it's not necessarily worth
handling anything other than V4L2_FRMSIZE_TYPE_DISCRETE, but we'd
probably want to print a FIXME and skip to the next iteration in that case.
> -
>        if (xioctl(fd, VIDIOC_ENUM_FRAMEINTERVALS, &frmival) != -1)
> -
>        {
> -
>            if (frmival.type == V4L2_FRMIVAL_TYPE_DISCRETE)
> -
>            {
> -
>                max_fps = frmival.discrete.denominator / frmival.discrete.numerator;
> -
>                min_fps = max_fps;
> -
>            }
> -
>            else if (frmival.type == V4L2_FRMIVAL_TYPE_STEPWISE)
> -
>            {
> -
>                max_fps = frmival.stepwise.max.denominator / frmival.stepwise.max.numerator;
> -
>                min_fps = frmival.stepwise.min.denominator / frmival.stepwise.min.numerator;
> -
>            }
> -
>        }
> -
>        else
> -
>            ERR("Failed to get fps: %s.\n", strerror(errno));
> -
> -
>        if (SUCCEEDED(create_capabilitie(fmtdesc.pixelformat, frmsize.discrete.width, frmsize.discrete.height,
> -
>                                max_fps, min_fps, &cap)))
> -
>        {
> -
>            new_caps = heap_realloc(device->caps, (device->caps_count + 1) * sizeof(*device->caps));
> -
>            if (!new_caps)
> -
>            {
> -
>                free_capabilitie(cap);
> -
>                goto error;
> -
>            }
> -
>            device->caps = new_caps;
> -
>            device->caps[device->caps_count] = cap;
> -
>            device->caps_count++;
> -
>        }
> -
>        frmsize.index++;
> -
>    }
> -
>    fmtdesc.index++;
> - }
> -
> - device->current_cap = device->caps[0];
> - format.fmt.pix.pixelformat = V4L2_PIX_FMT_BGR24;
>    if (xioctl(fd, VIDIOC_S_FMT, &format) == -1
>            || format.fmt.pix.pixelformat != V4L2_PIX_FMT_BGR24)