Re: [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