On 8/22/22 18:03, Rémi Bernon (@rbernon) wrote:
This includes some fixes for obvious mistakes as well as initial cleanup of the WM async reader, in preparation for implementing proper threading support.
The cleanup is certainly appreciated, but what's broken about our current threading?
The qasf (https://gitlab.winehq.org/wine/wine/-/merge_requests/686) and wmvcore (https://gitlab.winehq.org/wine/wine/-/merge_requests/685) tests show that there's actually at least one thread per stream, and likely two (one waiting / and eventually delivering samples, and one allocating samples), in addition to the callback thread.
Okay, thanks for the clarification. In the future, if submitting patch sets as if they are independent, it would help to explain all of the reasoning and context in each independent patch series. I had not even looked at patch set 685 yet, and 686 did not by itself imply anything about wmvcore behaviour.
(It may be worth pointing out that 15 patches is a lot to review at once, even if split across multiple series, and that splitting them like this may not be as productive as intended.)
I'm planning on doing some more refactoring first, ultimately implementing the async reader on top of the sync reader, and removing the need for a private interface. I've checked that it can indeed work on Windows too, and it'll make the code simpler overall.
What about the code is actually made simpler?
It will move all the async reader specific behavior to the async reader side, making it possible to move it out of winegstreamer and look at it alone.
Current wm_reader is a mix of different use cases, depending on whether it's used from the async or sync side, and it complicates the code.
I'm not sure I understand. As far as I can see the only currently async-specific code in wm_reader.c is the allocate-for-output support, and that will have to be changed anyway if we implement IWMSyncReader2::SetAllocateFor*().
Whether the async reader lives in wmvcore or winegstreamer is not something I'm inclined to see as important.
Refactoring will also remove the internal struct wm_reader object, merging it with the sync reader, as well as the internal helpers which are just there to factor the implementation between the sync and async reader equivalents. Most async reader functions can be implemented as a direct forward to the sync reader functions and currently some sync reader methods areimplemented and not the async ones, and reversely, depending on what was needed.
Sure. I'm not sure I see it as worthwhile yet, but maybe seeing the code will change my mind. I don't hate the idea at least.
One of the reasons I tend to avoid implementing one externally visible API on top of another is that it tends to make logs harder to read (mostly due to the extra clutter). That's not to say I'm sure it's not worth it in this case, but it's not immediately clear to me that the code would get a lot simpler.
I get that, but for most functions I don't think there's any need to log the async reader side as they are just proxying the call to the sync reader.
I really don't like that idea. I find it extremely unhelpful not to trace the outermost API entry point for a given call, and I've been annoyed at the lack of such traces in logs many times.