Re: [PATCH] evr: Add stubs for EnhancedVideoRenderer

[Nikolay Sivov]

On 18.08.2017 4:25, Fabian Maurer wrote:
> +static const IBaseFilterVtbl IBaseFilter_Vtbl =
> +{
> - /*** IUnknown methods ***/
> - EnhancedVideoRendererImpl_QueryInterface,
> - EnhancedVideoRendererImpl_AddRef,
> - EnhancedVideoRendererImpl_Release,
> - /*** IPersist methods ***/
> - EnhancedVideoRendererImpl_GetClassID,
> - /*** IMediaFilter methods ***/
> - EnhancedVideoRendererImpl_Stop,
> - EnhancedVideoRendererImpl_Pause,
> - EnhancedVideoRendererImpl_Run,
> - EnhancedVideoRendererImpl_GetState,
> - EnhancedVideoRendererImpl_SetSyncSource,
> - EnhancedVideoRendererImpl_GetSyncSource,
> - /*** IBaseFilter methods ***/
> - EnhancedVideoRendererImpl_EnumPins,
> - EnhancedVideoRendererImpl_FindPin,
> - EnhancedVideoRendererImpl_QueryFilterInfo,
> - EnhancedVideoRendererImpl_JoinFilterGraph,
> - EnhancedVideoRendererImpl_QueryVendorInfo
>
> +};

Shouldn't it use strmbase?
> +HRESULT EnhancedVideoRendererImpl_Create(IUnknown *pUnkOuter, void **ppObj)
> +{
> - EnhancedVideoRendererImpl *object;
> -
> - TRACE("(%p,%p)\n", pUnkOuter, ppObj);
> -
> - object = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(EnhancedVideoRendererImpl));
> - if (!object)
> -
>    return E_OUTOFMEMORY;
> -
> - object->IBaseFilter_iface.lpVtbl = &IBaseFilter_Vtbl;
> - object->ref = 1;
> -
> - *ppObj = &object->IBaseFilter_iface;
> -
> - return S_OK;
>
> +}

This needs at test for aggregation support.
> +typedef struct {
> - IBaseFilter IBaseFilter_iface;
> - LONG ref;
>
> +} EnhancedVideoRendererImpl;

Does this need to be exposed?
> - TRACE("(0x%p, %d, %p)\n", instance, reason, reserved);
> -

Format looks wrong.
> -
>    case DLL_WINE_PREATTACH:
> -
>        return FALSE;    /* prefer native version */

It still doesn't do anything.
> +static HRESULT WINAPI XFCF_QueryInterface(LPCLASSFACTORY iface,REFIID riid, void **ppobj)
> +{

What does XF prefix stand for? Also please use consistent type names and
formatting.
> +/*******************************************************************************
> -
> - Retrieves class object from a DLL object
> -
> -
> -
> - NOTES
> -
> - Docs say returns STDAPI
> -
> -
> -
> - PARAMS
> -
> - rclsid [I] CLSID for the class object
> -
> - riid   [I] Reference to identifier of interface for class object
> -
> - ppv    [O] Address of variable to receive interface pointer for riid
> -
> -
> -
> - RETURNS
> -
> - Success: S_OK
> -
> - Failure: CLASS_E_CLASSNOTAVAILABLE, E_OUTOFMEMORY, E_INVALIDARG,
> -
> -
>         E_UNEXPECTED
> - */
>
> +HRESULT WINAPI DllGetClassObject(REFCLSID rclsid, REFIID riid, void **ppv)

I personally don't think this export needs a documentation header.
> - if ( !IsEqualGUID( &IID_IClassFactory, riid )
> -
>     && ! IsEqualGUID( &IID_IUnknown, riid) )
> -
>    return E_NOINTERFACE;

I think it's better to let QueryInterface handle this.
> - for (i=0; i < sizeof(object_creation)/sizeof(object_creation[0]); i++)
> - {
> -
>    if (IsEqualGUID(object_creation[i].clsid, rclsid))
> -
>        break;
> - }
> -
> - if (i == sizeof(object_creation)/sizeof(object_creation[0]))
> - {
> -
>    FIXME("%s: no class found.\n", debugstr_guid(rclsid));
> -
>    return CLASS_E_CLASSNOTAVAILABLE;
> - }

Does it support any other CLSIDs? This seems like too much. Static
factory instance will work too I suppose.

[PATCH] evr: Add stubs for EnhancedVideoRenderer

[Fabian Maurer]

Hi, thanks for your feedback.
> Shouldn't it use strmbase?

You mean the methods that for example quartz/nullrenderer.c uses  in 
NullRenderer_Vtbl?
I'm not sure, that's why I just stubbed them.
> This needs at test for aggregation support.

Sorry I don't know what you mean. Care to elaborate?
> > +typedef struct {
> > - IBaseFilter IBaseFilter_iface;
> > - LONG ref;
> >
> > +} EnhancedVideoRendererImpl;
>
> Does this need to be exposed?

You mean I shouldn't need to put that in a header? I just did it like in 
d3dxof, and it's the same in uiribbon. But yes, I could change that.
> > - TRACE("(0x%p, %d, %p)\n", instance, reason, reserved);
> > -
>
> Format looks wrong.

Already used that in my uiribbon patch and it was fine though. What should I 
change about it?
> It still doesn't do anything.

Yes, I'll re-add that.
> What does XF prefix stand for? Also please use consistent type names and
> formatting.
>
> I personally don't think this export needs a documentation header.
>
> I think it's better to let QueryInterface handle this.

I just copied that code from d3dxof, I figured it was ok since it was accepted 
into my uiribbon patch. Should I change it?
> > - for (i=0; i < sizeof(object_creation)/sizeof(object_creation[0]);
> >
> > i++)
> > - {
> > -
> >    if (IsEqualGUID(object_creation[i].clsid, rclsid))
> > -
> >        break;
> > - }
> > -
> > - if (i == sizeof(object_creation)/sizeof(object_creation[0]))
> > - {
> > -
> >    FIXME("%s: no class found.\n", debugstr_guid(rclsid));
> > -
> >    return CLASS_E_CLASSNOTAVAILABLE;
> > - }
>
> Does it support any other CLSIDs? This seems like too much. Static
> factory instance will work too I suppose.

According to my windows7 registry, yes, there are other CLSIDs in that dll, 
like "MF Video Mixer" and "Tearless Window Presenter" for example.
Not sure what exactly static factory instance means.

Regards,
Fabian Maurer

[PATCH] evr: Add stubs for EnhancedVideoRenderer

[Nikolay Sivov]

On 18.08.2017 20:01, Fabian Maurer wrote:
> Hi, thanks for your feedback.
> > Shouldn't it use strmbase?
>
> You mean the methods that for example quartz/nullrenderer.c uses  in 
> NullRenderer_Vtbl?
> I'm not sure, that's why I just stubbed them.

I mean BaseFilter from strmbase.
> > This needs at test for aggregation support.
>
> Sorry I don't know what you mean. Care to elaborate?

I mean a test to show if it's supported or not. Currently you ignore
outer interface argument. Take a look at qcap for example.
> > > +typedef struct {
> > > - IBaseFilter IBaseFilter_iface;
> > > - LONG ref;
> > >
> > > +} EnhancedVideoRendererImpl;
> >
> > Does this need to be exposed?
>
> You mean I shouldn't need to put that in a header? I just did it like in 
> d3dxof, and it's the same in uiribbon. But yes, I could change that.

I mean if there's no reason, why should it be in a header.
> > > - TRACE("(0x%p, %d, %p)\n", instance, reason, reserved);
> > > -
> >
> > Format looks wrong.
>
> Already used that in my uiribbon patch and it was fine though. What should I 
> change about it?

Using just "%p" is enough for pointers, you don't need extra prefix.
> > What does XF prefix stand for? Also please use consistent type names and
> > formatting.
> >
> > I personally don't think this export needs a documentation header.
> >
> > I think it's better to let QueryInterface handle this.
>
> I just copied that code from d3dxof, I figured it was ok since it was accepted 
> into my uiribbon patch. Should I change it?

I personally don't think d3dxof is a good place to look for examples.
But anyway copied parts should get some cleanup and not be taken for
granted. XF prefix for example could possibly be an acronym for
directXFile, and that does not apply to anything but d3dxof.
> > > - for (i=0; i < sizeof(object_creation)/sizeof(object_creation[0]);
> > >
> > > i++)
> > > - {
> > > -
> > >    if (IsEqualGUID(object_creation[i].clsid, rclsid))
> > > -
> > >        break;
> > > - }
> > > -
> > > - if (i == sizeof(object_creation)/sizeof(object_creation[0]))
> > > - {
> > > -
> > >    FIXME("%s: no class found.\n", debugstr_guid(rclsid));
> > > -
> > >    return CLASS_E_CLASSNOTAVAILABLE;
> > > - }
> >
> > Does it support any other CLSIDs? This seems like too much. Static
> > factory instance will work too I suppose.
>
> According to my windows7 registry, yes, there are other CLSIDs in that dll, 
> like "MF Video Mixer" and "Tearless Window Presenter" for example.

Ok, then this structure is fine I guess. Thanks for checking.
> Not sure what exactly static factory instance means.

I mean a static, non-heap allocated, instance. But that's minor.
> Regards,
> Fabian Maurer

[PATCH] evr: Add stubs for EnhancedVideoRenderer

[Fabian Maurer]

> > > Shouldn't it use strmbase?
> >
> > You mean the methods that for example quartz/nullrenderer.c uses  in
> > NullRenderer_Vtbl?
> > I'm not sure, that's why I just stubbed them.
>
> I mean BaseFilter from strmbase.

Ah you mean I shouldn't implement this interface by myself, but just return a 
BaseFilter object?
I'm not sure how I would test against that to make sure it actually is a 
BaseFilter and not something different.
> > > This needs at test for aggregation support.
> >
> > Sorry I don't know what you mean. Care to elaborate?
>
> I mean a test to show if it's supported or not. Currently you ignore
> outer interface argument. Take a look at qcap for example.

I see, I never heard of COM aggregation before, so I first need to read into 
it.
> I mean if there's no reason, why should it be in a header.
> Using just "%p" is enough for pointers, you don't need extra prefix.
> I personally don't think d3dxof is a good place to look for examples.
> But anyway copied parts should get some cleanup and not be taken for
> granted. XF prefix for example could possibly be an acronym for
> directXFile, and that does not apply to anything but d3dxof.

Makes sense, I will rework that.

Regards,
Fabian Maurer