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

[Zebediah Figura]

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