[1/2] qcap: Distinguish interface and implementation pointer for VfwCapture output pin.

[Nikolay Sivov]

On 29.06.2015 13:13, Sebastian Lackner wrote:
> On 29.06.2015 09:09, Nikolay Sivov wrote:
>> On 29.06.2015 9:08, Sebastian Lackner wrote:
>>> -        BasePin *pin;
>>> +        VfwPinImpl *pin;
>>>
>>>            TRACE("destroying everything\n");
>>>            if (This->init)
>>> @@ -226,10 +236,10 @@ static ULONG WINAPI VfwCapture_Release(IBaseFilter * iface)
>>>                    qcap_driver_stop(This->driver_info, &This->filter.state);
>>>                qcap_driver_destroy(This->driver_info);
>>>            }
>>> -        pin = (BasePin*) This->pOutputPin;
>>> -        if (pin->pConnectedTo != NULL)
>>> +        pin = impl_from_IPin(This->pOutputPin);
>>> +        if (pin->pin.pin.pConnectedTo != NULL)
>>>            {
>>> -            IPin_Disconnect(pin->pConnectedTo);
>>> +            IPin_Disconnect(pin->pin.pin.pConnectedTo);
>>
>> This is not better than it is now, 'pOutputPin' is IPin* so you should be using public methods.
>
> Thanks for the feedback. In this case pOutputPin is basically part of the same object,
> not an arbitrary external interface. It is constructed with VfwPin_Construct ->
> BaseOutputPin_Construct, and also holds a reference to the critical section of the parent
> object for example, so there is no clear distinction between public/private interfaces
> anyway.
>
> Most other code parts which use BaseOutputPin_Construct store the result in something like:
> (IPin **)&This->store_impl_pointer_here where the variable already has the corresponding
> implementation type. Should we change it here too? Using only the public interface is not possible,
> take a look at for example the following code part, where a private member is set from a
> different interface:
>
> --- snip ---
>          This->driver_info = qcap_driver_init( This->pOutputPin,
>                 var.__VARIANT_NAME_1.__VARIANT_NAME_2.__VARIANT_NAME_3.ulVal );
>          if (This->driver_info)
>          {
>              pin = impl_from_IPin(This->pOutputPin);
>              pin->driver_info = This->driver_info; // <---
> --- snip ---

I personally prefer to use public interface, usually it makes things 
more clear, and code doesn't depend on implementation changes.

If in this particular case it's not possible then yes, we should store 
impl pointer instead of interface pointer.

By the way, variant thing looks a little insane, assuming intention was 
to get V_UI4(&var), which is not necessary right on its own as 
qcap_driver_init() takes USHORT argument.

>
>
>>
>>>       if (IsEqualIID(riid, &IID_IUnknown) || IsEqualIID(riid, &IID_IPin))
>>> -        *ppv = This;
>>> +        *ppv = &This->pin.pin.IPin_iface;
>>
>> This could as well say 'iface'.
>>
>>
>
> Is there really a clear preference? I personally always preferred the long form so far,
> to make it a bit more clear when many different interfaces are returned.
>

No, I don't think there is. In this case it would be shorter, but only 
because of strmbase nesting level. Both ways are better than 'This', so 
it's up to author I suppose.