[PATCH v5 0/5] MR302: winegstreamer: Some wg_transform H264 fixes for Mortal Kombat 11 and Yakuza 4.

[Zebediah Figura]

On 7/5/22 01:04, Rémi Bernon (@rbernon) wrote:
> On Mon Jul  4 21:34:22 2022 +0000, **** wrote:
>> Zebediah Figura replied on the mailing list:
>> ```
>> On 6/30/22 07:33, Rémi Bernon wrote:
>>> From: Rémi Bernon <rbernon at codeweavers.com>
>>>
>>> Using the allocator lock and replacing the transform_request_sample
>>> callback with a default wg_allocator callback.
>>>
>>> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
>>> ---
>>>    dlls/winegstreamer/unix_private.h |  2 ++
>>>    dlls/winegstreamer/wg_allocator.c | 42 +++++++++++++++++++++++++++++--
>>>    dlls/winegstreamer/wg_transform.c | 24 +++---------------
>>>    3 files changed, 46 insertions(+), 22 deletions(-)
>>>
>> This is making things even more complicated; now we have two entirely
>> different code paths to set samples.
>> I understand that adding more locks increases the mental burden, but I
>> think this is worse—sure, we're "reusing" a lock that was going to be
>> created anyway, but it's still effectively introducing another lock,
>> plus now this requires us to understand when GStreamer is going to
>> implicitly take that lock.
>> Not to mention that this is similar to what I was trying to propose,
>> enough that I'd like to revisit it. I really don't want to argue about
>> the way forward—there's been too much of that already—but I'm still
>> finding it hard to justify the complexity of this whole callback
>> infrastructure, when we could more simply set up a fixed-size pool
>> beforehand. Yes, we might run out of space in the pool and need to
>> allocate more sysmem buffers, but it should be easy enough to find a
>> pool size large enough that that doesn't happen in practice.
>> [Ideally it would take an application leaking an entire stream of a
>> muxed file—which has been known to happen—but in that case, it strikes
>> me as better to eat up the Unix address space instead of the win32
>> address space, so the limited size is actually a good thing.]
>> Or, failing that (or in addition to it?), maybe we could just not use
>> the same wg_allocator object for both transforms and parsers. Yes, it's
>> more gobject boilerplate, which is annoying, but wg_allocator is a
>> complex beast and I really don't want to make it worse. And when it
>> comes to output samples, the allocator and the transform really do seem
>> to have entirely different considerations.
>> ```
> I don't think a fixed pool can work. We cannot expect the output buffers passed downstream to the application to ever be released, or, even if they probably will, I don't think we can easily and safely put them in back in the pool when they are.

Wait, we can't?

I assume you mean for Media Foundation specifically, in particular 
because DirectShow does have pooling built into the API (and with a 
maximum buffer count as well). As far as I understand neither Media 
Foundation nor the ASF reader explicitly pool, but I'd still be 
surprised if a well-behaved application would never release samples. 
That would result in a significant memory leak.

> 
> 1) We'd need a custom buffer implementation for each API.

Wait, why?

> 
> 2) Some API provide allocation callback which conflicts with 1) needs.
> 
> 3) The allocation callbacks may block.

They may block waiting for what?

> 
> 4) The buffers may be released after the pipeline has been shutdown, we'd need to keep it alive until all the buffers are released, which is likely going to cause problems.

Why? We shouldn't need to provide anything to the Unix side except for 
the buffer pointer and size. I would expect that the PE buffers should 
outlive the unix device under normal operation.

> 
> Another way would be to keep a list of all the allocated buffers, monitoring it for released buffers, but that doesn't sound great either. Especially with the allocation callbacks there's no way of telling whether the pipeline is the only owner.
> 
> I think the `wg_allocator` callback is overall simpler, even though there's considerations to have too, especially with the blocking API callbacks, which need to be solved without blocking the reading thread or other streams allocations.
> 

I can grudgingly accept the allocator callback, although I'm not 
convinced yet it's the best solution, but if it's necessary, I'd rather 
be consistent about using it, and I'd rather keep the sample in the 
wg_transform object (and ideally introduce a new lock to protect it) 
than what 2/5 and 3/5 do.