Re: [PATCH 0/5] MR116: qasf: Better implementation of ASF reader filter.

[Rémi Bernon (＠rbernon)]

On Mon May 23 23:15:35 2022 +0000, **** wrote:
> Zebediah Figura replied on the mailing list:
> On 5/23/22 07:07, Rémi Bernon wrote:
> > +static HRESULT source_query_interface(struct strmbase_pin *iface,
> REFIID iid, void **out)
> > +{
> > +    struct asf_stream *stream = impl_from_strmbase_pin(iface);
> > +
> > +    TRACE("iface %p, iid %s, out %p.\n", iface, debugstr_guid(iid), out);
> > +
> > +    if (IsEqualGUID(iid, &IID_IUnknown)
> > +            || IsEqualGUID(iid, &IID_IPin))
> > +        *out = &stream->source.pin.IPin_iface;
> > +    else
> > +    {
> > +        *out = NULL;
> > +        WARN("%s not implemented, returning E_NOINTERFACE.\n", debugstr_guid(iid));
> > +        return E_NOINTERFACE;
> > +    }
> > +
> > +    IUnknown_AddRef((IUnknown *)*out);
> > +    return S_OK;
> > +}
> > +
> No need to handle IUnknown or IPin here; strmbase will do that for us if 
> we return E_NOINTERFACE. (And if we don't need to handle any other 
> interfaces, we can just leave the query_interface callback NULL.)
> > @@ -46,12 +80,29 @@ static inline struct asf_reader
> *impl_from_strmbase_filter(struct strmbase_filte
> >   
> >   static struct strmbase_pin *asf_reader_get_pin(struct
> strmbase_filter *iface, unsigned int index)
> >   {
> > -    return NULL;
> > +    struct asf_reader *filter = impl_from_strmbase_filter(iface);
> > +    struct strmbase_pin *pin;
> > +
> > +    TRACE("iface %p, index %u.\n", iface, index);
> > +
> > +    if (index >= InterlockedOr(&filter->stream_count, 0)) pin = NULL;
> > +    else pin = &filter->streams[index].source.pin;
> InterlockedOr() looks suspicious. If we're going to be thread safe (and 
> we should) we should just take the filter lock around 
> IFileSourceFilter::Load(). [It's already implicitly taken in the 
> get_pin() callback.]
> Please keep if and else bodies on a separate line.
> Early return would obviate the need for a local variable here.
>
> InterlockedOr() looks suspicious. If we're going to be thread safe (and 
> we should) we should just take the filter lock around 
> IFileSourceFilter::Load(). [It's already implicitly taken in the 
> get_pin() callback.]

I was wary of locking because WM reader callbacks are done while holding WM reader lock, and prevent any concurrent call to other WM reader methods. I had some patches to leave the lock when calling the callbacks but I'm not sure it's a good way to go.