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

[Nikolay Sivov]

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.
>  if (IsEqualIID(riid, &IID_IUnknown) || IsEqualIID(riid, &IID_IPin))
> -
>    *ppv = This;
> -
>    *ppv = &This->pin.pin.IPin_iface;

This could as well say 'iface'.

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

[Sebastian Lackner]

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 ---
> >  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.

[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.

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

[Sebastian Lackner]

On 29.06.2015 12:50, Nikolay Sivov wrote:
> On 29.06.2015 13:13, Sebastian Lackner wrote:
> > 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.

Okay, I'll resend an updated version later then.
> 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.

Yes, I also noticed it. Changes to this part should probably be in a separate patch though.