On 8/24/22 12:50, Rémi Bernon (@rbernon) wrote:
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.
Well, I didn't think that these fixes and cleanups, although motivated in part by the threading issues, were really dependent.
Not dependent in terms of code, no, but in reasoning some of these patches don't make sense without that context.
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.
The async reader has access to any state kept in the wm_reader, it's hard to tell across three different sources what is used and where.
Yes, there's actually not so much async reader specifics on wm_reader side, and that's probably a good reason to split it as it means the private interface isn't really necessary.
Sure, that's a good reason to switch.
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.
I pushed a branch with the changes I intend to do there: https://gitlab.winehq.org/rbernon/wine/-/commits/wip/wmvcore/v1
I had a brief look and I think I'm convinced enough that this is a good idea.
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.
I guess we can keep the async reader side logging as well, I don't mind personally and I don't think it's so much clutter.
Note that you're pointing the issue with having too few logging, and at the same time worried that using the sync reader would cause clutter. I think that in general there's never enough logging, and the logs are huge anyway. So I don't think implementing things on top of each other is an issue, we do that all the time.
Also currently, none of the wm_reader_* entry points are really tracing anything. It's probably okay-ish as it's almost single threaded, but adding more threads will just make debugging the async reader very difficult. Implementing it on top of the sync reader will solve that as every call to the sync reader will be automatically traced.
To be clear I think that if we want to implement the async reader directly on top of the sync reader, I'd rather trace both entry points (and have that extra clutter) than just one of them. My preference in general, though, is that multiple entry points which do the same thing be backed by a single helper which is not traced.
It's not a strong preference, though. At this point my hesitation to refactor wmvcore is motivated more by inertia than anything.