[PATCH] qcap: implement VfwPin_CheckMediaType(), and test connecting to a NullRenderer

[Damjan Jovanovic]

Currently a video capture device cannot even connect to a NullRenderer,
as VfwPin_CheckMediaType() always return a hardcoded E_NOTIMPL,
preventing any media format from being negotiated.

Implement it and test.

Signed-off-by: Damjan Jovanovic 
---
 dlls/qcap/capture.h            |   1 +
 dlls/qcap/tests/Makefile.in    |   3 +-
 dlls/qcap/tests/videocapture.c | 172 +++++++++++++++++++++++++++++++++
 dlls/qcap/v4l.c                |  47 +++++++--
 dlls/qcap/vfwcapture.c         |   4 +-
 5 files changed, 218 insertions(+), 9 deletions(-)
 create mode 100644 dlls/qcap/tests/videocapture.c

[Zebediah Figura]

On 4/22/19 2:40 AM, Damjan Jovanovic wrote:
> +static void test_connect_null_renderer(IBaseFilter *captureDevice)
> +{
> - IGraphBuilder *graph = NULL;
> - IBaseFilter *nullRenderer = NULL;
> - IPin *captureDeviceOut = NULL;
> - IPin *nullRendererIn = NULL;
> - HRESULT hr;
> -
> - hr = CoCreateInstance(&CLSID_FilterGraph, NULL, CLSCTX_INPROC_SERVER, &IID_IGraphBuilder,
> -
>        (LPVOID*)&graph);
> - ok(SUCCEEDED(hr), "couldn't create graph builder, hr=0x%08x\n", hr);
> - if (FAILED(hr))
> -
>    goto end;
> -
> - hr = CoCreateInstance(&CLSID_NullRenderer, NULL, CLSCTX_INPROC_SERVER,
> -
>        &IID_IBaseFilter, (LPVOID*)&nullRenderer);
> - ok(SUCCEEDED(hr) ||
> -
>        /* Windows 2008: http://stackoverflow.com/questions/29410348/initialize-nullrender-failed-with-error-regdb-e-classnotreg-on-win2008-r2 */
> -
>        broken(hr == REGDB_E_CLASSNOTREG), "couldn't create NullRenderer, hr=0x%08x\n", hr);
> - if (FAILED(hr))
> -
>    goto end;
> - hr = IGraphBuilder_AddFilter(graph, nullRenderer, NULL);
> - ok(SUCCEEDED(hr), "IGraphBuilder_AddFilter() failed, hr=0x%08x\n", hr);
> - if (FAILED(hr))
> -
>    goto end;
> -
> - hr = IGraphBuilder_AddFilter(graph, captureDevice, NULL);
> - ok(SUCCEEDED(hr), "IGraphBuilder_AddFilter() failed, hr=0x%08x\n", hr);
> - if (FAILED(hr))
> -
>    goto end;
> -
> - hr = find_pin(nullRenderer, PINDIR_INPUT, &nullRendererIn);
> - ok(hr == S_OK, "couldn't find NullRenderer input pin, hr=0x%08x\n", hr);
> - if (hr != S_OK)
> -
>    goto end;
> -
> - hr = find_pin(captureDevice, PINDIR_OUTPUT, &captureDeviceOut);
> - ok(hr == S_OK, "couldn't find capture device output pin, hr=0x%08x\n", hr);
> - if (hr != S_OK)
> -
>    goto end;
> -
> - hr = IGraphBuilder_Connect(graph, captureDeviceOut, nullRendererIn);
> - ok(SUCCEEDED(hr), "couldn't connect capture device to NullRenderer, hr=0x%08x\n", hr);
> -
>
> +end:
> - if (graph != NULL)
> -
>    IGraphBuilder_Release(graph);
> - if (nullRenderer != NULL)
> -
>    IBaseFilter_Release(nullRenderer);
> - if (captureDeviceOut != NULL)
> -
>    IPin_Release(captureDeviceOut);
> - if (nullRendererIn != NULL)
> -
>    IPin_Release(nullRendererIn);
>
> +}
> -

This is not especially convincing. I know that we can't really test 
individual formats, given that devices themselves are going to vary, and 
I know also that you're using the same logic as is already present in 
IAMStreamConfig::SetFormat(), but it would be nice to test that 
IPin::QueryAccept() accepts formats returned by IEnumMediaTypes (if any) 
and by IAMStreamConfig::GetFormat() and IAMStreamConfig::GetStreamCaps() 
and rejects formats which are malformed, or use a major type other than 
MEDIATYPE_Video or GUID_NULL, or use a format type other than 
FORMAT_VideoInfo or FORMAT_NULL, etc.

Stylistically I'd prefer if you'd use the same style as the quartz tests 
I've been recently adding. This means e.g. not using camel case, 
avoiding LPJUNK typedefs, braces on a new line, messages and comments 
formatted with consistent wording and period. Additionally I'd prefer 
that you test for S_OK where possible rather than using SUCCEEDED(), and 
omit superfluous failure paths where we expect success.
> diff --git a/dlls/qcap/tests/Makefile.in b/dlls/qcap/tests/Makefile.in
> index 2b988476ad..a5b9d0e474 100644
> --- a/dlls/qcap/tests/Makefile.in
> +++ b/dlls/qcap/tests/Makefile.in
> @@ -5,4 +5,5 @@ C_SRCS = \
>    audiorecord.c \
>    avico.c \
>    qcap.c \
> - smartteefilter.c
> - smartteefilter.c \
> - videocapture.c

As long as these tests are aimed towards the WDM video capture filter 
rather than the VFW capture filter, I'd prefer a name like wdmcapture.c.

[Damjan Jovanovic]

On Mon, Apr 22, 2019 at 11:06 PM Zebediah Figura 
wrote:
> On 4/22/19 2:40 AM, Damjan Jovanovic wrote:
> > +static void test_connect_null_renderer(IBaseFilter *captureDevice)
> > +{
> > - IGraphBuilder *graph = NULL;
> > - IBaseFilter *nullRenderer = NULL;
> > - IPin *captureDeviceOut = NULL;
> > - IPin *nullRendererIn = NULL;
> > - HRESULT hr;
> > -
> > - hr = CoCreateInstance(&CLSID_FilterGraph, NULL,
>
> CLSCTX_INPROC_SERVER, &IID_IGraphBuilder,
> > -
> >        (LPVOID*)&graph);
> > - ok(SUCCEEDED(hr), "couldn't create graph builder, hr=0x%08x\n", hr);
> > - if (FAILED(hr))
> > -
> >    goto end;
> > -
> > - hr = CoCreateInstance(&CLSID_NullRenderer, NULL,
>
> CLSCTX_INPROC_SERVER,
> > -
> >        &IID_IBaseFilter, (LPVOID*)&nullRenderer);
> > - ok(SUCCEEDED(hr) ||
> > -
> >        /* Windows 2008:
>
> http://stackoverflow.com/questions/29410348/initialize-nullrender-failed-wit...
> */
> > -
> >        broken(hr == REGDB_E_CLASSNOTREG), "couldn't create
>
> NullRenderer, hr=0x%08x\n", hr);
> > - if (FAILED(hr))
> > -
> >    goto end;
> > - hr = IGraphBuilder_AddFilter(graph, nullRenderer, NULL);
> > - ok(SUCCEEDED(hr), "IGraphBuilder_AddFilter() failed, hr=0x%08x\n",
>
> hr);
> > - if (FAILED(hr))
> > -
> >    goto end;
> > -
> > - hr = IGraphBuilder_AddFilter(graph, captureDevice, NULL);
> > - ok(SUCCEEDED(hr), "IGraphBuilder_AddFilter() failed, hr=0x%08x\n",
>
> hr);
> > - if (FAILED(hr))
> > -
> >    goto end;
> > -
> > - hr = find_pin(nullRenderer, PINDIR_INPUT, &nullRendererIn);
> > - ok(hr == S_OK, "couldn't find NullRenderer input pin, hr=0x%08x\n",
>
> hr);
> > - if (hr != S_OK)
> > -
> >    goto end;
> > -
> > - hr = find_pin(captureDevice, PINDIR_OUTPUT, &captureDeviceOut);
> > - ok(hr == S_OK, "couldn't find capture device output pin,
>
> hr=0x%08x\n", hr);
> > - if (hr != S_OK)
> > -
> >    goto end;
> > -
> > - hr = IGraphBuilder_Connect(graph, captureDeviceOut, nullRendererIn);
> > - ok(SUCCEEDED(hr), "couldn't connect capture device to NullRenderer,
>
> hr=0x%08x\n", hr);
> > -
> >
> > +end:
> > - if (graph != NULL)
> > -
> >    IGraphBuilder_Release(graph);
> > - if (nullRenderer != NULL)
> > -
> >    IBaseFilter_Release(nullRenderer);
> > - if (captureDeviceOut != NULL)
> > -
> >    IPin_Release(captureDeviceOut);
> > - if (nullRendererIn != NULL)
> > -
> >    IPin_Release(nullRendererIn);
> >
> > +}
> > -
>
> This is not especially convincing. I know that we can't really test
> individual formats, given that devices themselves are going to vary, and
> I know also that you're using the same logic as is already present in
> IAMStreamConfig::SetFormat(), but it would be nice to test that
> IPin::QueryAccept() accepts formats returned by IEnumMediaTypes (if any)
> and by IAMStreamConfig::GetFormat() and IAMStreamConfig::GetStreamCaps()
> and rejects formats which are malformed, or use a major type other than
> MEDIATYPE_Video or GUID_NULL, or use a format type other than
> FORMAT_VideoInfo or FORMAT_NULL, etc.
>
> Stylistically I'd prefer if you'd use the same style as the quartz tests
> I've been recently adding. This means e.g. not using camel case,
> avoiding LPJUNK typedefs, braces on a new line, messages and comments
> formatted with consistent wording and period. Additionally I'd prefer
> that you test for S_OK where possible rather than using SUCCEEDED(), and
> omit superfluous failure paths where we expect success.

Thank you. A new version with some of those changes is out.
> > diff --git a/dlls/qcap/tests/Makefile.in b/dlls/qcap/tests/Makefile.in
> > index 2b988476ad..a5b9d0e474 100644
> > --- a/dlls/qcap/tests/Makefile.in
> > +++ b/dlls/qcap/tests/Makefile.in
> > @@ -5,4 +5,5 @@ C_SRCS = \
> >       audiorecord.c \
> >       avico.c \
> >       qcap.c \
> > -
> > smartteefilter.c
> > -
> > smartteefilter.c \
> > -
> > videocapture.c
>
> As long as these tests are aimed towards the WDM video capture filter
> rather than the VFW capture filter, I'd prefer a name like wdmcapture.c.

I don't see how that's relevant. We test DirectShow, not the kernel-mode
device drivers.

Also wdmcapture.c doesn't tell us what media is being captured.