Re: Some fixes for filtergraph.c

[Christian Costa]

Hi Maarten,

Maarten Lankhorst wrote:
> This doesn't fix everything, just theroutines i called.. some other 
> similar functions aren't fixed because filtergraph.c is really THAT 
> leaky..b mostly with reference counts..

Yes, Indeed. :-)

Here are some comments :
> ------------------------------------------------------------------------
>
> --- wine-20050310/dlls/quartz/filtergraph.c	2005-03-02 11:12:12.000000000 +0100
> +++ wine/dlls/quartz/filtergraph.c	2005-04-24 00:50:30.000000000 +0200
> @@ -556,8 +556,7 @@
>             IPin_QueryDirection(ppin, &pindir);
>             if (pindir == PINDIR_OUTPUT)
>                 i++;
> -            else
> -                IPin_Release(ppin);
> +            IPin_Release(ppin);
>         }
>         *pppins = CoTaskMemAlloc(sizeof(IPin*)*i);
>         /* Retrieve output pins */
> @@ -650,6 +649,7 @@
>         IPin** ppins;
>         IPin* ppinfilter;
>         IBaseFilter* pfilter = NULL;
> +        BOOL HaveEnumPin = FALSE;

Why don't you just set ppinfilter to NULL
and do "if (ppinfilter) IPin_Release(ppinfilter);" at the end ?
> hr = GetFilterInfo(pMoniker, &clsid, &var);
>         IMoniker_Release(pMoniker);
> @@ -677,6 +677,7 @@
>             ERR("Enumpins (%lx)\n", hr);
>             goto error;
>         }
> +
>         hr = IEnumPins_Next(penumpins, 1, &ppinfilter, &pin);
>         if (FAILED(hr)) {
>             ERR("Next (%lx)\n", hr);
> @@ -687,6 +688,7 @@
>             goto error;
>         }
>         IEnumPins_Release(penumpins);
> +        HaveEnumPin = TRUE;

See above.
> hr = IPin_Connect(ppinOut, ppinfilter, NULL);
>         if (FAILED(hr)) {
> @@ -703,17 +705,21 @@
>             TRACE("pins to consider: %ld\n", nb);
>             for(i = 0; i < nb; i++) {
>                 TRACE("Processing pin %d\n", i);
> -                hr = IGraphBuilder_Connect(iface, ppins[0], ppinIn);
> +                hr = IGraphBuilder_Connect(iface, ppins[i], ppinIn);
>                 if (FAILED(hr)) {
> -                    TRACE("Cannot render pin %p (%lx)\n", ppinfilter, hr);
> -                    return hr;
> +                   TRACE("Cannot render pin %p (%lx)\n", ppinfilter, hr);
>                 }
> +                IPin_Release(ppins[i]);
> +                if (SUCCEEDED(hr)) break;

This is not correct, you must connect all pins, not just the first that 
succeeds.
> }
>             CoTaskMemFree(ppins);
> +            IBaseFilter_Release(pfilter);
> +            IPin_Release(ppinfilter);
> +            break;
>         }
> -        break;
>
> error:
> +        if (HaveEnumPin) IPin_Release(ppinfilter);

See above.
> if (pfilter) {
>             IGraphBuilder_RemoveFilter(iface, pfilter);
>             IBaseFilter_Release(pfilter);
> @@ -1175,13 +1181,16 @@
>     ULONG nb;
>     ULONG i;
>     PIN_INFO PinInfo;
> +    BOOL HavePinInfo = FALSE;
>
> TRACE("%p %p %lld\n", pGraph, pOutputPin, tStart);
>
> hr = IPin_ConnectedTo(pOutputPin, &pInputPin);
>
> -    if (SUCCEEDED(hr))
> +    if (SUCCEEDED(hr)) {
> +        HavePinInfo = TRUE;
>         hr = IPin_QueryPinInfo(pInputPin, &PinInfo);
> +    }

Personally, I would set PinIno.pFilter to NULL on failure and do if 
(PinIno.pFilter) release... at the end.
But it's just a matter of taste.
> if (SUCCEEDED(hr))
>         hr = GetInternalConnections(PinInfo.pFilter, pInputPin, &ppPins, &nb);
> @@ -1202,6 +1211,7 @@
>        * FIXME: We should prevent exploring from a pin more than once. This can happens when
>        * several input pins are connected to the same output (a MUX for instance). */
>                 ExploreGraph(pGraph, ppPins[i], tStart);
> +                IPin_Release(ppPins[i]);
>             }
>
> CoTaskMemFree(ppPins);
> @@ -1210,6 +1220,7 @@
>         IBaseFilter_Run(PinInfo.pFilter, tStart);
>     }
>
> +    if (HavePinInfo) IBaseFilter_Release(PinInfo.pFilter);
>     return hr;
> }
>
> @@ -1255,9 +1266,11 @@
>             IPin_QueryDirection(pPin, &dir);
>             if (dir == PINDIR_INPUT)
>             {
> +                IPin_Release(pPin);
>                 source = FALSE;
>                 break;
>             }
> +            IPin_Release(pPin);

That would be simplier to do a single call to IPinRelease after 
IPin_QueryDirection.
> }
>         if (source == TRUE)
>         {
> @@ -1267,6 +1280,7 @@
>             {
>                 /* Explore the graph downstream from this pin */
>                 ExploreGraph(This, pPin, 0);
> +                IPin_Release(pPin);
>             }
>             IBaseFilter_Run(pfilter, 0);
>         }

Nice patch, BTW! :-)

Bye,
Christian