Re: [PATCH 2/6] qcap/videocapture: Use formats from capabilities instead of hard coding.

[Zebediah Figura]

On 4/23/20 12:28 AM, Jactry Zeng wrote:
> Signed-off-by: Jactry Zeng
>
> dlls/qcap/capture.h            |   2 +-
>  dlls/qcap/tests/videocapture.c |  42 ++++++++
>  dlls/qcap/v4l.c                | 175 +++++++++++++--------------------
>  dlls/qcap/vfwcapture.c         |   2 +-
>  4 files changed, 113 insertions(+), 108 deletions(-)
>
> diff --git a/dlls/qcap/capture.h b/dlls/qcap/capture.h
> index fa48324dd6..d3cf193e7f 100644
> --- a/dlls/qcap/capture.h
> +++ b/dlls/qcap/capture.h
> @@ -25,7 +25,7 @@ typedef struct _Capture Capture;
>
> Capture *qcap_driver_init(struct strmbase_source *,USHORT) DECLSPEC_HIDDEN;
>  HRESULT qcap_driver_destroy(Capture*) DECLSPEC_HIDDEN;
> -HRESULT qcap_driver_check_format(Capture*,const AM_MEDIA_TYPE*) DECLSPEC_HIDDEN;
> +HRESULT qcap_driver_check_format(Capture* device,const AM_MEDIA_TYPE *mt,LONG *index) DECLSPEC_HIDDEN;
>  HRESULT qcap_driver_set_format(Capture*,AM_MEDIA_TYPE*) DECLSPEC_HIDDEN;
>  HRESULT qcap_driver_get_format(const Capture*,AM_MEDIA_TYPE**) DECLSPEC_HIDDEN;
>  HRESULT qcap_driver_get_prop_range(Capture*,VideoProcAmpProperty,LONG*,LONG*,LONG*,LONG*,LONG*) DECLSPEC_HIDDEN;
> diff --git a/dlls/qcap/tests/videocapture.c b/dlls/qcap/tests/videocapture.c
> index 5532654650..a43875101c 100644
> --- a/dlls/qcap/tests/videocapture.c
> +++ b/dlls/qcap/tests/videocapture.c
> @@ -21,6 +21,7 @@
>  #define COBJMACROS
>  #include "dshow.h"
>  #include "wine/test.h"
> +#include "wine/strmbase.h"
>
> static void test_media_types(IPin *pin)
>  {
> @@ -59,6 +60,44 @@ static void test_media_types(IPin *pin)
>      ok(hr != S_OK, "Got hr %#x.\n", hr);
>  }
>
> +static void test_stream_config(IPin *pin)
> +{
> - IAMStreamConfig *stream_config;
> - VIDEOINFOHEADER *video_info;
> - AM_MEDIA_TYPE *format;
> - HRESULT hr;
> -
> - hr = IPin_QueryInterface(pin, &IID_IAMStreamConfig, (void **)&stream_config);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
> - hr = IAMStreamConfig_GetFormat(stream_config, &format);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> - ok(IsEqualGUID(&format->majortype, &MEDIATYPE_Video), "Got wrong majortype: %s.\n",
> -
>        debugstr_guid(&format->majortype));
> -
> - hr = IAMStreamConfig_SetFormat(stream_config, format);
> - ok(hr == S_OK, "Got hr %#x.\n", hr);
> -
> - format->majortype = MEDIATYPE_Audio;
> - hr = IAMStreamConfig_SetFormat(stream_config, format);
> - ok(hr == E_FAIL, "Got hr %#x.\n", hr);
> -
> - format->majortype = MEDIATYPE_Video;
> - video_info = (VIDEOINFOHEADER *)format->pbFormat;
> - video_info->bmiHeader.biWidth--;
> - video_info->bmiHeader.biHeight--;
> - hr = IAMStreamConfig_SetFormat(stream_config, format);
> - ok(hr == E_FAIL, "Got hr %#x.\n", hr);
> -
> - video_info->bmiHeader.biWidth = 10000000;
> - video_info->bmiHeader.biHeight = 10000000;
> - hr = IAMStreamConfig_SetFormat(stream_config, format);
> - ok(hr == E_FAIL, "Got hr %#x.\n", hr);
> - FreeMediaType(format);
> -
> - IAMStreamConfig_Release(stream_config);
>
> +}
> -
>
> static void test_capture(IBaseFilter *filter)
>  {
>      IEnumPins *enum_pins;
> @@ -73,7 +112,10 @@ static void test_capture(IBaseFilter *filter)
>          PIN_DIRECTION pin_direction;
>          IPin_QueryDirection(pin, &pin_direction);
>          if (pin_direction == PINDIR_OUTPUT)
> -
>    {
>        test_media_types(pin);
> -
>        test_stream_config(pin);
> -
>    }
>    IPin_Release(pin);
> }
>
> diff --git a/dlls/qcap/v4l.c b/dlls/qcap/v4l.c
> index 26eccf6ee3..db9117d593 100644
> --- a/dlls/qcap/v4l.c
> +++ b/dlls/qcap/v4l.c
> @@ -103,7 +103,7 @@ struct capabilitie
>
> struct _Capture
>  {
> - UINT width, height, bitDepth, fps, outputwidth, outputheight;
> - UINT outputwidth, outputheight;
> struct capabilitie *current_cap;
> struct capabilitie **caps;
> LONG caps_count;
>
> @@ -146,126 +146,78 @@ HRESULT qcap_driver_destroy(Capture *capBox)
>      return S_OK;
>  }
>
> -HRESULT qcap_driver_check_format(Capture *device, const AM_MEDIA_TYPE *mt)
> +HRESULT qcap_driver_check_format(Capture *device, const AM_MEDIA_TYPE *mt, LONG *index)

I'd mildly prefer to leave this function signature like it is, and
instead introduce a helper, something like "const struct capabilitie
*find_caps(Capture *device, const AM_MEDIA_TYPE *mt)".

Somewhat separately, I also think we could shrink the "qcap_driver" API
surface to qcap_driver_get_caps(), and put find_caps() and the entire
pin_query_accept()/pin_get_media_type() implementation on the
"vfwcapture.c" side.
> {
> - HRESULT hr;
> -
>
> LONG i;
> -
>
> TRACE("device %p, mt %p.\n", device, mt);
>
> if (!mt)
>        return E_POINTER;
>
> if (!IsEqualGUID(&mt->majortype, &MEDIATYPE_Video))
> -
>    return S_FALSE;
> -
>    return E_FAIL;
>
> - if (IsEqualGUID(&mt->formattype, &FORMAT_VideoInfo) && mt->pbFormat
> -
>        && mt->cbFormat >= sizeof(VIDEOINFOHEADER))
> - for (i = device->caps_count - 1; i >= 0; i--)

Is there a motivation for iterating through the array in reverse order here?
>  {
> -
>    VIDEOINFOHEADER *vih = (VIDEOINFOHEADER *)mt->pbFormat;
> -
>    if (vih->bmiHeader.biBitCount == 24 && vih->bmiHeader.biCompression == BI_RGB)
> -
>        hr = S_OK;
> -
>    else
> -
>    VIDEOINFOHEADER *video_info = (VIDEOINFOHEADER *)mt->pbFormat;
> -
>    struct capabilitie *cap = device->caps[i];
> -
> -
>    if (IsEqualIID(&mt->formattype, &cap->media_type.formattype)
> -
>            && mt->cbFormat >= sizeof(VIDEOINFOHEADER)
> -
>            && video_info
> -
>            && video_info->bmiHeader.biWidth == cap->width
> -
>            && video_info->bmiHeader.biHeight == cap->height)
>    {
> -
>        FIXME("Unsupported compression %#x, bpp %u.\n", vih->bmiHeader.biCompression,
> -
>                vih->bmiHeader.biBitCount);
> -
>        hr = S_FALSE;
> -
>        if (index)
> -
>            *index = i;
> -
>        TRACE("matched with: %d.\n", i);
> -
>        return S_OK;
>    }
> }
> -
>
> else
> -
>    hr = VFW_E_INVALIDMEDIATYPE;
>
> -
>
> return hr;
> - return E_FAIL;
>
> }