Re: [PATCH 2/5] qedit: Add pins, IMemInputPin implementation and grabbing to SampleGrabber (try 4)

[Nikolay Sivov]

On 2/5/2010 17:54, Paul Chitescu wrote:
> Changelog:
>   qedit: Add pins, IMemInputPin implementation and grabbing to SampleGrabber
>
> This time I checked it applies. Sorry.

---
+/* Sample Grabber pin implementation */

+typedef struct _SG_Pin {
+    IPin pin;

This should be const IPinVtbl.

+    PIN_DIRECTION dir;
+    WCHAR const *name;
+    struct _SG_Impl *sg;
+    IPin *pair;

If it's a list why not SG_Pin* here?

+} SG_Pin;

---
Also I don't see why such helpers are needed:
---
static ULONG SampleGrabber_addref(SG_Impl *This)
---
Why not use common Ixxxx_AddRef() on grabber anywhere you need it?

[PATCH 2/5] qedit: Add pins, IMemInputPin implementation and grabbing to SampleGrabber (try 4)

[Paul Chitescu]

See my answers inline just under your questions.

Regards,

Paul

On Sunday 07 February 2010 04:26:59 pm Nikolay Sivov wrote:
> On 2/5/2010 17:54, Paul Chitescu wrote:
> > Changelog:
> >   qedit: Add pins, IMemInputPin implementation and grabbing to 
> > SampleGrabber
> >
> > This time I checked it applies. Sorry.
>
> ---
> +/* Sample Grabber pin implementation */
>
> +typedef struct _SG_Pin {
> +    IPin pin;
>
> This should be const IPinVtbl.

I'm extending IPin which contains the vtable itself. The resulting binary code 
should be the same but with less casting so less chances of errors.
> +    PIN_DIRECTION dir;
> +    WCHAR const *name;
> +    struct _SG_Impl *sg;
> +    IPin *pair;
>
> If it's a list why not SG_Pin* here?

Because it's not a list. There are exactly 2 fixed pins and they are members 
of the SampleGrabber structure.
> +} SG_Pin;
>
> ---
> Also I don't see why such helpers are needed:
> ---
> static ULONG SampleGrabber_addref(SG_Impl *This)
> ---
> Why not use common Ixxxx_AddRef() on grabber anywhere you need it?

It's the same object. Why have different code paths? Why reimplement AddRef 
(or worse, the more complex Release or QueryInterface) with the potential of 
introducing errors whenever one modifies just one of the versions?

Implementing just one of the intefaces (say, IBaseFilter) is equally possible 
but the traces are more confusing. At least SampleGrabber_addref() makes no 
false assumptions about which interface was used to call it.