[PATCH 0/5] MR116: qasf: Better implementation of ASF reader filter.
Rémi Bernon (@rbernon)
wine at gitlab.winehq.org
Tue May 24 01:14:02 CDT 2022
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.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/116#note_1140
More information about the wine-devel
mailing list