Re: Wine msn webcam patch (Attempt 2)

[Robert Shearman]

Maarten Lankhorst wrote:
> I've been trying to get msn webcam to work again, and I found out that 
> WINEDEBUG=+quartz gives a crash when msn creates a pin, and then does 
> some stuff with it, I can only presume some TRACE or WARNING causes 
> the error, but the effect is I can't use those commands, this patch 
> currently allows MSN preview screen to become gray, but it's not 
> actually sending any information (not even fake images) to msn's 
> webcam pin, if someone can find where exactly it crashes, it would 
> help a lot, and make it a lot easier for me to actually implement a 
> working webcam (Right now it will probably only work for msn 
> messenger, but I can imagine other programs will work with this too in 
> a future version)
> This file makes changes to qcap, but they are not needed for the 
> process as far as I know

Hi Maarten,

Thanks for your work on devenum,quartz and qcap. I have some comments in 
addition to the ones Mike has already made.
> diff -Nru wine-old/dlls/devenum/createdevenum.c wine-new/dlls/devenum/createdevenum.c
> --- wine-old/dlls/devenum/createdevenum.c	2005-01-25 11:56:39.000000000 +0100
> +++ wine-new/dlls/devenum/createdevenum.c	2005-04-09 16:10:47.000000000 +0200
> @@ -117,6 +117,7 @@
> if (IsEqualGUID(clsidDeviceClass, &CLSID_AudioRendererCategory) ||
>     IsEqualGUID(clsidDeviceClass, &CLSID_AudioInputDeviceCategory) ||
> -
>    IsEqualGUID(clsidDeviceClass, &CLSID_VideoInputDeviceCategory) ||
>   IsEqualGUID(clsidDeviceClass, &CLSID_MidiRendererCategory))
> {
>       hbasekey = HKEY_CURRENT_USER;
>
> @@ -142,6 +143,7 @@
>     {
>         if (IsEqualGUID(clsidDeviceClass, &CLSID_AudioRendererCategory) ||
>             IsEqualGUID(clsidDeviceClass, &CLSID_AudioInputDeviceCategory) ||
> -
>        IsEqualGUID(clsidDeviceClass, &CLSID_VideoInputDeviceCategory) ||
>       IsEqualGUID(clsidDeviceClass, &CLSID_MidiRendererCategory))
>   {
>        HRESULT hr = DEVENUM_CreateSpecialCategories();
>
> @@ -413,6 +415,7 @@
>                 CoTaskMemFree(pTypes);
>        }
>    }
> -
>    res = DEVENUM_CreateAMCategoryKey(&CLSID_VideoInputDeviceCategory);

I'm not sure these changes are correct. If you go down the same route as 
done for the MIDI, wave in and wave out devices then you have to 
enumerate the video input devices and create keys for them here, but 
there is no clean way of doing this.
>     pMapper = (IFilterMapper2*)mapvptr;
>
> -
>    IFilterMapper2_CreateCategory(pMapper, &CLSID_VideoInputDeviceCategory, MERIT_DO_NOT_USE, friendlyvidcap);
> -
>    IFilterMapper2_CreateCategory(pMapper, &CLSID_VideoInputDeviceCategory, MERIT_NORMAL, friendlyvidcap);
>   IFilterMapper2_CreateCategory(pMapper, &CLSID_LegacyAmFilterCategory, MERIT_NORMAL, friendlydshow);
>   IFilterMapper2_CreateCategory(pMapper, &CLSID_VideoCompressorCategory, MERIT_DO_NOT_USE, friendlyvidcomp);
>   IFilterMapper2_CreateCategory(pMapper, &CLSID_AudioInputDeviceCategory, MERIT_DO_NOT_USE, friendlyaudcap);

This change is wrong. The video input category is MERIT_DO_NOT_USE on 
both my Win2k and WinXP systems (tested with a webcam on the WinXP 
system too). This makes sense because you don't want capture devices to 
be automatically included in a filter graph to satisfy some requirement 
of some filter. If the MSN webcam won't work without this change then 
there is a bug elsewhere.
> +/* AMStreamConfig interface, we only need to implement {G,S}etFormat */
> +static HRESULT WINAPI AMStreamConfig_QueryInterface(IAMStreamConfig * iface, REFIID riid, LPVOID * ppv)
> +{
> - FIXME("%p: stub, not worth the effort\n", iface);
> - return S_OK;
>
> +}

I'll implement it for you:

{
    if (IsEqualIID(riid, &IID_IUnknown) ||
        IsEqualIID(riid, &IID_IAMStreamConfig))
    {
        *ppv = (LPVOID)iface;
        return S_OK;
    }

FIXME("No interface for iid %s\n", debugstr_guid(riid));
    return E_NOINTERFACE;
}
> -
>
> +static ULONG WINAPI AMStreamConfig_AddRef(IAMStreamConfig * iface)
> +{
> - FIXME("%p: stub, not worth the effort\n", iface);
> - return 1;
>
> +}

Again, you should implement this:
return InterlockedIncrement(&This->refs);
> -
>
> +static ULONG WINAPI AMStreamConfig_Release(IAMStreamConfig * iface)
> +{
> - FIXME("%p: stub, not worth the effort\n", iface);
> - return 1;
>
> +}
> -

You should implement this too.
> +static HRESULT WINAPI AMStreamConfig_SetFormat(IAMStreamConfig *iface,
> -
>                                          AM_MEDIA_TYPE *pmt) {
> - ERR("%p: %p stub !!\n", iface, pmt);
> - return E_NOTIMPL;
>
> +}

Use FIXME instead of ERR.
> -
>
> +static HRESULT WINAPI AMStreamConfig_GetFormat(IAMStreamConfig *iface,
> -
>                                          AM_MEDIA_TYPE **pmt) {
> - ERR("%p: %p stub !!\n", iface, pmt);
> - // FIXME: The stuff below is just there so it contains something

Again, use FIXME. Don't use C99 comments. Use /* comment */ instead.
> -
> - VIDEOINFOHEADER *pvi;
> -
> - pmt[0] = CoTaskMemAlloc(sizeof (AM_MEDIA_TYPE));
> - memcpy (&pmt[0]->formattype, &FORMAT_VideoInfo, sizeof (GUID));
> - memcpy (&pmt[0]->majortype, &MEDIATYPE_Video, sizeof (GUID));
> - memcpy (&pmt[0]->subtype, &MEDIASUBTYPE_RGB24, sizeof (GUID));
> - pmt[0]->bTemporalCompression = 0;
> - pmt[0]->bFixedSizeSamples = 1;
> - pmt[0]->lSampleSize = 1;
> -
> - pmt[0]->cbFormat = sizeof(VIDEOINFOHEADER);
> - pmt[0]->pbFormat = CoTaskMemAlloc(pmt[0]->cbFormat);
> - ZeroMemory(pmt[0]->pbFormat, pmt[0]->cbFormat);
> - pvi = (VIDEOINFOHEADER *)pmt[0]->pbFormat;
> - pvi->rcSource.right = 640; pvi->rcSource.bottom = 480;
> - pvi->rcTarget.right = 640; pvi->rcTarget.bottom = 480;
> - pvi->dwBitRate = 10;
> - pvi->dwBitErrorRate = 0;
> - pvi->AvgTimePerFrame = (LONGLONG)(10000000.0 / 25); // 25 is hardcoded
> -
> - // FIXME: The stuff below is just there so it contains something
> -
> - pvi->bmiHeader.biWidth = 640;
> - pvi->bmiHeader.biHeight = 480;
> - pvi->bmiHeader.biBitCount = 24;
> - pvi->bmiHeader.biCompression = BI_RGB;
> - pvi->bmiHeader.biSizeImage = 0;
> - pvi->bmiHeader.biXPelsPerMeter = 640;
> - pvi->bmiHeader.biYPelsPerMeter = 480;
> - pvi->bmiHeader.biClrUsed = 0;
> - pvi->bmiHeader.biClrImportant = 0;
> -
> - return S_OK;
>
> +}
> -
>
> +typedef struct _VIDEO_STREAM_CONFIG_CAPS {
> - GUID guid;
> - ULONG VideoStandard;
> - SIZE InputSize;
> - SIZE MinCroppingSize;
> - SIZE MaxCroppingSize;
> - int CropGranularityX;
> - int CropGranularityY;
> - int CropAlignX;
> - int CropAlignY;
> - SIZE MinOutputSize;
> - SIZE MaxOutputSize;
> - int OutputGranularityX;
> - int OutputGranularityY;
> - int StretchTapsX;
> - int StretchTapsY;
> - int ShrinkTapsX;
> - int ShrinkTapsY;
> - LONGLONG MinFrameInterval;
> - LONGLONG MaxFrameInterval;
> - LONG MinBitsPerSecond;
> - LONG MaxBitsPerSecond;
>
> +} VIDEO_STREAM_CONFIG_CAPS;
> -
>
> +static HRESULT WINAPI AMStreamConfig_GetNumberOfCapabilities(IAMStreamConfig *iface, int *piCount, int *piSize)
> +{
> - TRACE("%p: %p %p - stub! Should be harmless\n", iface, piCount, piSize);

This should be a FIXME (or if that produces too much output then a WARN) 
instead.
> - *piCount = 1;
> - *piSize = sizeof(VIDEO_STREAM_CONFIG_CAPS);
> - return S_OK; /* Not implemented for this interface, but who cares =) */
>
> +}
> -
>
> +static HRESULT WINAPI AMStreamConfig_GetStreamCaps(IAMStreamConfig *iface,
> +int iIndex, AM_MEDIA_TYPE **pmt, BYTE *pSCC)
> +{
> - TRACE("%p: %d %p %p\n", iface, iIndex, pmt, pSCC);

FIXME?
> -
> - VIDEOINFOHEADER *pvi;
> - VIDEO_STREAM_CONFIG_CAPS *vsc;
> -
> - pmt[0] = CoTaskMemAlloc(sizeof (AM_MEDIA_TYPE));
> - memcpy (&pmt[0]->formattype, &FORMAT_VideoInfo, sizeof (GUID));
> - memcpy (&pmt[0]->majortype, &MEDIATYPE_Video, sizeof (GUID));
> - memcpy (&pmt[0]->subtype, &MEDIASUBTYPE_RGB24, sizeof (GUID));
> - pmt[0]->bTemporalCompression = 0;
> - pmt[0]->bFixedSizeSamples = 1;
> - pmt[0]->lSampleSize = 1;
> -
> - // FIXME: The stuff below is just there so it contains something
> -
> - pmt[0]->cbFormat = sizeof(VIDEOINFOHEADER);
> - pmt[0]->pbFormat = CoTaskMemAlloc(pmt[0]->cbFormat);
> - ZeroMemory(pmt[0]->pbFormat, pmt[0]->cbFormat);
> - pvi = (VIDEOINFOHEADER *)pmt[0]->pbFormat;
> - pvi->rcSource.right = 640; pvi->rcSource.bottom = 480;
> - pvi->rcTarget.right = 640; pvi->rcTarget.bottom = 480;
> - pvi->dwBitRate = 10;
> - pvi->dwBitErrorRate = 0;
> - pvi->AvgTimePerFrame = (LONGLONG)(10000000.0 / 25); // 25 is hardcoded
> -
> - // FIXME: The stuff below is just there so it contains something
> -
> - pvi->bmiHeader.biWidth = 640;
> - pvi->bmiHeader.biHeight = 480;
> - pvi->bmiHeader.biBitCount = 24;
> - pvi->bmiHeader.biCompression = BI_RGB;
> - pvi->bmiHeader.biSizeImage = 0;
> - pvi->bmiHeader.biXPelsPerMeter = 640;
> - pvi->bmiHeader.biYPelsPerMeter = 480;
> - pvi->bmiHeader.biClrUsed = 0;
> - pvi->bmiHeader.biClrImportant = 0;
> -
> - // FIXME: Blahblahblah
> -
> - vsc = (VIDEO_STREAM_CONFIG_CAPS *)pSCC;
> - memcpy (&vsc->guid, &FORMAT_VideoInfo, sizeof (GUID));
> - vsc->VideoStandard = 0;
> - vsc->InputSize.cx = 640; vsc->InputSize.cy = 480;
> - vsc->MinCroppingSize.cx = 320; vsc->MinCroppingSize.cy = 240;
> - vsc->MaxCroppingSize.cx = 640; vsc->MaxCroppingSize.cy = 480;
> - vsc->CropGranularityX = 10;
> - vsc->CropGranularityY = 10;
> - vsc->CropAlignX = 0;
> - vsc->CropAlignY = 0;
> - vsc->MinOutputSize.cx = 320; vsc->MinOutputSize.cy = 240;
> - vsc->MaxOutputSize.cx = 640; vsc->MaxOutputSize.cy = 480;
> - vsc->OutputGranularityX = 10;
> - vsc->OutputGranularityY = 10;
> - vsc->StretchTapsX = 1;
> - vsc->StretchTapsY = 1;
> - vsc->ShrinkTapsX = 1;
> - vsc->ShrinkTapsY = 1;
> - vsc->MinFrameInterval = (LONGLONG)(10000000.0 / 25);
> - vsc->MaxFrameInterval = (LONGLONG)(10000000.0 / 25);
> - vsc->MinBitsPerSecond = 24;
> - vsc->MaxBitsPerSecond = 24;
> -
> - return S_OK; /* Not implemented for this interface */
>
> +}
> -
>
> +static const IAMStreamConfigVtbl IAMStreamConfig_VTable =
> +{
> - AMStreamConfig_QueryInterface,
> - AMStreamConfig_AddRef,
> - AMStreamConfig_Release,
> - AMStreamConfig_SetFormat,
> - AMStreamConfig_GetFormat,
> - AMStreamConfig_GetNumberOfCapabilities,
> - AMStreamConfig_GetStreamCaps
>
> +};
> -
>
> +/* IKsPropertySet interface */
> +static HRESULT WINAPI KSP_QueryInterface(IKsPropertySet * iface, REFIID riid, LPVOID * ppv)
> +{
> - TRACE("%p: stub, not worth the effort\n", iface);
> - return S_OK;
>
> +}
> -
>
> +static ULONG WINAPI KSP_AddRef(IKsPropertySet * iface)
> +{
> - TRACE("%p: stub, not worth the effort\n", iface);
> - return 1;
>
> +}
> -
>
> +static ULONG WINAPI KSP_Release(IKsPropertySet * iface)
> +{
> - TRACE("%p: stub, not worth the effort\n", iface);
> - return 1;
>
> +}

Please implement the above. It is not hard and should take 5/10 minutes 
at the most.
> -
>
> +static HRESULT WINAPI KSP_Set(IKsPropertySet * iface, REFGUID guidPropSet, DWORD dwPropID, LPVOID pInstanceData, DWORD cbInstanceData, LPVOID pPropData, DWORD cbPropData)
> +{
> - return E_NOTIMPL;
>
> +}

Add a FIXME here? If it is not implemented in native, then a comment 
here instead stating this would be appropriate.
> -
>
> +static HRESULT WINAPI KSP_Get(IKsPropertySet * iface, REFGUID guidPropSet, DWORD dwPropID, LPVOID pInstanceData, DWORD cbInstanceData, LPVOID pPropData, DWORD cbPropData, DWORD *pcbReturned)
> +{
> - TRACE("()\n");
> - if (!IsEqualIID(guidPropSet, &AMPROPSETID_Pin))
> -
>  return E_PROP_SET_UNSUPPORTED;
>
> +/* if (dwPropID != AMPROPERTY_PIN_CATEGORY)
> -
>  return E_PROP_ID_UNSUPPORTED;

Any reason why this is commented out? Leaving things like this in the 
source tends to introduce an element of doubt when someone next looks at 
the code.
> - */if (pPropData == NULL && pcbReturned == NULL)
> -
>  return E_POINTER;
> - if (pcbReturned)
> -
>  *pcbReturned = sizeof(GUID);
> - if (pPropData == NULL)
> -
>  return S_OK;
> - if (cbPropData < sizeof(GUID))
> -
>  return E_UNEXPECTED;
> - *(GUID *)pPropData = PIN_CATEGORY_PREVIEW;
> - TRACE("() Returning CaP\n");
> - return S_OK;
>
> +}
> -
>
> +static HRESULT WINAPI KSP_QuerySupported(IKsPropertySet * iface, REFGUID guidPropSet, DWORD dwPropID, DWORD *pTypeSupport)
> +{
> - TRACE("%p: stub\n", iface);

FIXME.

Rob