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:
> - HROOT\CLSID{"clsid of categories"]\Instance
> - 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:
> > - HROOT\CLSID{"clsid of categories"]\Instance
> > - 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