Re: [QUARTZ] Add implementation for IFilterMapper interface

[Robert Shearman]

Christian Costa wrote:
> Hi,
>
> This patch adds implementation for the IFilterMapper interface.

Nice. I have few comments (see below).
> However filters registered with this interface require some work in
> devenum to
> be seen from application. This will be done when time permits.

Ok. I had assumed devenum was complete, but obviously not. Does it spit out
any fixme's?

...
> @@ -879,7 +893,7 @@
>              if (SUCCEEDED(hrSub))
>                  hrSub =
> ICreateDevEnum_CreateClassEnumerator(pCreateDevEnum, &clsidCat,
> &pEnum, 0);
>
> -            if (SUCCEEDED(hrSub))
> +            if (SUCCEEDED(hrSub) && (hrSub == S_OK))

You can remove the SUCCEEDED(hrSub) completely here.

...
> +        if (SUCCEEDED(hrSub))
> +	{
> +	    len = (strlenW((WCHAR*)&V_UNION(&var, bstrVal))+1) *
> sizeof(WCHAR);
> +	    if (!(regfilters[idx].Name = CoTaskMemAlloc(len*2)))
> +		hr = E_OUTOFMEMORY;
> +	}
> +
> +        if (SUCCEEDED(hrSub))
> +	{
> +	    memcpy(regfilters[idx].Name, &V_UNION(&var, bstrVal), len);
> +	    regfilters[idx].Clsid = clsid;
> +	    idx++;
> +	}

Indentation messed up a bit here and many other places.

...
> +    if (cFetched > 0)
> +    {
> +	for(i = 0; i < cFetched; i++) {
> +	    /* The string in the REGFILTER structure must be
> allocated in the same block as the REGFILTER structure itself */
> +            ppRegFilter[i] =
> (REGFILTER*)CoTaskMemAlloc(sizeof(REGFILTER)+(strlenW(This->RegFil
> ters[i].Name)+1)*sizeof(WCHAR));
> +	    if (!ppRegFilter[i]) {
> +		while(i) {
> +		    CoTaskMemFree(ppRegFilter[--i]);
> +		    ppRegFilter[i] = NULL;
> +		}
> +		return E_OUTOFMEMORY;
> +	    }
> +	    ppRegFilter[i]->Clsid = This->RegFilters[i].Clsid;
> +	    ppRegFilter[i]->Name =
> (WCHAR*)((char*)ppRegFilter[i]+sizeof(REGFILTER));
> +	    CopyMemory(ppRegFilter[i]->Name,
> This->RegFilters[i].Name,
> (strlenW(This->RegFilters[i].Name)+1)*sizeof(WCHAR));
> +	}

Please choose one style of bracket placement and stick to it (this occurs in
a few places too).

Other than these things, it looks fine.

Rob

[QUARTZ] Add implementation for IFilterMapper interface

[Christian Costa]

Robert Shearman wrote:
> Christian Costa wrote:
> > Hi,
> >
> > This patch adds implementation for the IFilterMapper interface.
>
> Nice. I have few comments (see below).
> > However filters registered with this interface require some work in
> > devenum to
> > be seen from application. This will be done when time permits.
>
> Ok. I had assumed devenum was complete, but obviously not. Does it spit out
> any fixme's?

No, it doesn't.

In fact, Devenum enumerates entries in:
1) HROOT\CLSID\{"clsid of categories"]\Instance
2) HCURRENTUSER\Software\Microsoft\ActiveMovie\devenum

For Direct Show filters, Devenum retreives filters registered with 
IFilterMapper and recreates entries in ....\devenum
so when enumerating filters  we get the IFilterMapper2 filters followed 
by IFilterMapper ones.
I think this is the same for ICM and ACM filters.

The current code only do 1 or 2 but not both at the same time.
> ...
> > @@ -879,7 +893,7 @@
> >             if (SUCCEEDED(hrSub))
> >                 hrSub =
> > ICreateDevEnum_CreateClassEnumerator(pCreateDevEnum, &clsidCat,
> > &pEnum, 0);
> >
> > -            if (SUCCEEDED(hrSub))
> > +            if (SUCCEEDED(hrSub) && (hrSub == S_OK))
>
> You can remove the SUCCEEDED(hrSub) completely here.
>
> ...

He! Right! :-)
> > +        if (SUCCEEDED(hrSub))
> > +	{
> > +	    len = (strlenW((WCHAR*)&V_UNION(&var, bstrVal))+1) *
> > sizeof(WCHAR);
> > +	    if (!(regfilters[idx].Name = CoTaskMemAlloc(len*2)))
> > +		hr = E_OUTOFMEMORY;
> > +	}
> > +
> > +        if (SUCCEEDED(hrSub))
> > +	{
> > +	    memcpy(regfilters[idx].Name, &V_UNION(&var, bstrVal), len);
> > +	    regfilters[idx].Clsid = clsid;
> > +	    idx++;
> > +	}
>
> Indentation messed up a bit here and many other places.

Ok! I will fix that.
> ...
> > +    if (cFetched > 0)
> > +    {
> > +	for(i = 0; i < cFetched; i++) {
> > +	    /* The string in the REGFILTER structure must be
> > allocated in the same block as the REGFILTER structure itself */
> > +            ppRegFilter[i] =
> > (REGFILTER*)CoTaskMemAlloc(sizeof(REGFILTER)+(strlenW(This->RegFil
> > ters[i].Name)+1)*sizeof(WCHAR));
> > +	    if (!ppRegFilter[i]) {
> > +		while(i) {
> > +		    CoTaskMemFree(ppRegFilter[--i]);
> > +		    ppRegFilter[i] = NULL;
> > +		}
> > +		return E_OUTOFMEMORY;
> > +	    }
> > +	    ppRegFilter[i]->Clsid = This->RegFilters[i].Clsid;
> > +	    ppRegFilter[i]->Name =
> > (WCHAR*)((char*)ppRegFilter[i]+sizeof(REGFILTER));
> > +	    CopyMemory(ppRegFilter[i]->Name,
> > This->RegFilters[i].Name,
> > (strlenW(This->RegFilters[i].Name)+1)*sizeof(WCHAR));
> > +	}
>
> Please choose one style of bracket placement and stick to it (this occurs in
> a few places too).

I tried to stick to your style. It seems I forgot some brackets. :-)
> Other than these things, it looks fine.
>
> Rob

Fine.
Thanks Rob,
Christian

[QUARTZ] Add implementation for IFilterMapper interface

[Robert Shearman]

Christian Costa wrote:
> > Ok. I had assumed devenum was complete, but obviously not. Does 
> > it spit out
> > any fixme's?
>
> No, it doesn't.
>
> In fact, Devenum enumerates entries in:
> 1) HROOT\CLSID\{"clsid of categories"]\Instance
> 2) HCURRENTUSER\Software\Microsoft\ActiveMovie\devenum
>
> For Direct Show filters, Devenum retreives filters registered with 
> IFilterMapper and recreates entries in ....\devenum
> so when enumerating filters  we get the IFilterMapper2 filters followed 
> by IFilterMapper ones.
> I think this is the same for ICM and ACM filters.
>
> The current code only do 1 or 2 but not both at the same time.

That makes a lot of sense. If I get some time I will implement this.

Thanks for the new patch, it looks good.

Rob

[QUARTZ] Add implementation for IFilterMapper interface

[Christian Costa]

Robert Shearman wrote:
> Christian Costa wrote:
> > > Ok. I had assumed devenum was complete, but obviously not. Does
> >
> > it spit out
> > > any fixme's?
> >
> > No, it doesn't.
> >
> > In fact, Devenum enumerates entries in:
> > 1) HROOT\CLSID\{"clsid of categories"]\Instance
> > 2) HCURRENTUSER\Software\Microsoft\ActiveMovie\devenum
> >
> > For Direct Show filters, Devenum retreives filters registered with 
> > IFilterMapper and recreates entries in ....\devenum
> > so when enumerating filters  we get the IFilterMapper2 filters followed 
> > by IFilterMapper ones.
> > I think this is the same for ICM and ACM filters.
> >
> > The current code only do 1 or 2 but not both at the same time.
>
> That makes a lot of sense. If I get some time I will implement this.

Fine. :-)
BTW, Windows seems to only update the devenum entries, if any, of the 
category eumerated.
I don't think this is significant or not though...

Bye,
Christian

[QUARTZ] Add implementation for IFilterMapper interface

[Boaz Harrosh]

Christian Costa wrote:
> Fine. :-)
> BTW, Windows seems to only update the devenum entries, if any, of the 
> category eumerated.
> I don't think this is significant or not though...

devenum entries are Just a cache that quartz is keeping. It is different 
between DirectShow versions. I have never figured out how/when it 
decides to refresh the cache but he does a good Job. So I suspect it is 
only an in-session cache that is refreshed every App load (first use).

The main REG, that is standard, and that gets written by 
DllRegisterServer is the HKEY_CLASS_ROOT one.

[QUARTZ] Add implementation for IFilterMapper interface

[Christian Costa]

Boaz Harrosh wrote:
> Christian Costa wrote:
> > Fine. :-)
> > BTW, Windows seems to only update the devenum entries, if any, of the 
> > category eumerated.
> > I don't think this is significant or not though...
>
> devenum entries are Just a cache that quartz is keeping. It is 
> different between DirectShow versions. I have never figured out 
> how/when it decides to refresh the cache but he does a good Job. So I 
> suspect it is only an in-session cache that is refreshed every App 
> load (first use).
>
> The main REG, that is standard, and that gets written by 
> DllRegisterServer is the HKEY_CLASS_ROOT one.

So it doesn't matter. Unless we want to mix different versions (native 
and builtin) of devenum.dll and quartz.dll...
Thanks for these details. :-)

Bye,
Christian