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

[Rémi Bernon (@rbernon)]

On Tue May 24 16:40:21 2022 +0000, **** wrote:
> Zebediah Figura replied on the mailing list:
> ```
> On 5/24/22 11:25, Rémi Bernon (@rbernon) wrote:
> > On Tue May 24 16:09:54 2022 +0000, **** wrote:
> >> Zebediah Figura replied on the mailing list:
> >> \`\`\`
> >> On 5/24/22 01:12, Rémi Bernon (@rbernon) wrote:
> >>> On Mon May 23 23:15:33 2022 +0000, **** wrote:
> >>>> Zebediah Figura replied on the mailing list:
> >>>> \`\`\`
> >>>> On 5/23/22 07:07, Rémi Bernon wrote:
> >>>>> +static HRESULT asf_callback_create(struct asf_reader *filter,
> >>>> IWMReaderCallback **out)
> >>>>> +{
> >>>>> +    struct asf_callback *impl;
> >>>>> +
> >>>>> +    if (!(impl = calloc(1, sizeof(*impl))))
> >>>>> +        return E_OUTOFMEMORY;
> >>>>> +
> >>>>> +    impl->IWMReaderCallback_iface.lpVtbl = &reader_callback_vtbl;
> >>>>> +    impl->filter = filter;
> >>>>> +    impl->ref = 1;
> >>>>> +
> >>>>> +    *out = &impl->IWMReaderCallback_iface;
> >>>>> +    return S_OK;
> >>>>> +}
> >>>> Can we put the IWMReaderCallback interface inside of struct asf_reader,
> >>>> instead of making this a separate object?
> >>>> \`\`\`
> >>>
> >>> No, the reader will hold a ref on it and it would prevent filter
> >> destruction otherwise.
> >>>
> >> If it's the same object, the reader shouldn't be holding a reference. In
> >> fact I think they should be able to share the same refcount.
> >> \`\`\`
> >> If it's the same object, the reader shouldn't be holding a reference. In
> >> fact I think they should be able to share the same refcount.
> > 
> > The reader has no way to know that and it adds a ref to the callback
> interface as it should. I don't think we should decref ourselves after
> calling IWMReader_Open, that sounds pretty ugly.
> > 
> Oops, you meant the wmvcore reader, not the qasf reader.
> Making it a separate allocation doesn't really help the problem, though. 
> We're still holding a pointer to the reader without holding a reference, 
> which is probably workable in practice but makes me worried.
> If we want to be fully safe, I think the right thing to do is to have a 
> separate refcount for the callback, close the WM reader when the 
> filter's refcount reaches zero, and destroy the filter when both 
> refcounts reach zero. This is kind of similar to what's done here, but 
> with the callback holding an "internal" reference count to the 
> filter—except that you don't need them to be separate objects.
> (It's also not clear that using the asynchronous reader is worth the 
> trouble, but for the sake of progress I'll set that aside...)
> ```
Regarding the refcount issue it looks like calling `IWMReader_Close` should be enough to release the callback. As there's no guarantee that it does we could also clear the filter pointer when it is destroyed and leave the callback to be released eventually later.

Using the sync reader we would just re-implement the async reader in qasf reader and add another read thread implementation which feels pretty redundant.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/116#note_1165