On Fri Jul 4 18:32:26 2025 +0000, Charlotte Pabst wrote:
I think it is better to sequentially call then check for events in a
lock step, making things more deterministic and better exhibiting the order things are supposed to happen. Of course in this case some events are received on the source, and some on the stream, so it's not 100% deterministic and you may wait for events in one order or the other but still. To be fair, MS docs say things like "Events from the media source are not synchronized with events from the media streams." [1], so I figured it'd be more reliable to wait for source and stream events at the same time in any scenario where they could theoretically be delivered in arbitrary order - independently of if I'm able to observe a certain order on windows reliably or not (Hence also the different design from mf's test_callback - I adapted it from test_callback originally but changed it to this to support doing BeginEvent on multiple event sources in parallel). For many of these cases (like the start events), our MF media session code also accounts for arbitrary order. The issues I see with testing unspecified windows behavior more precisely especially when it comes to event order is that:
- Some things might be racy on windows and simply be likely but not
guaranteed to be delivered in a certain order, especially when the docs explicitly say that the order is not defined. 2. The windows implementation might change, and since most of this stuff go through the more robust media session, it wouldn't break applications. 3. A test that relies on less implementation details could potentially be adapted to run with a bigger matrix of different files or configurations. Which is why I've tried to mostly just rely on behavior that is either documented or something that we know games actively need. Essentially, treating the unit tests as a sort of specification. Especially some of the things like waiting for the first sample and then expecting the `MESourceRateChanged` and `MEStreamThinMode` event and updated timestamps afterwards seems incorrect - implementations may buffer a bunch of samples (I think winegstreamer does, actually? And I vaguely remember having related issues with the windows mp4 source, too), so it's not exactly guaranteed when the transition will happen - marking that is what `MEStreamThinMode` exists for. (Sidenote: if we don't rely on when the transition happens, we also can't use exact timestamp tests, which is why in this case it wouldn't matter whether we're dealing with avi or mp4. And the game I'm implementing this for also happens to just rely on the interval between timestamps being correct). And (hypothetically) what if that first sample we request takes a while to load and the `MESourceRateChanged` event goes through first? I'm also a bit wary of testing for the next event to match something. What if new, unrelated events are added and somehow get in the way? Another concern: Why make the check for `MEStreamThinMode` depend on `MESourceRateChanged` delivering `VT_R4` values? That's just an unrelated bug/oversight in wine, this MR doesn't even fix it, and at that point I'd rather check for `winetest_platform_is_wine` again. I'm not that big of a fan of wine checks anyway tho, I'd much rather just make sure to not "eat" random events in case a certain expected event doesn't get delivered. My initial approach for these tests was much more sequential and I've changed it to what it is now after considering many of these problems. Well, these are my concerns, I'm less knowledgeable/experienced in this matter so maybe I'm just overthinking things. I'm not against using your approach instead. [1] https://learn.microsoft.com/en-us/windows/win32/api/mfidl/nf-mfidl-imfrateco...
MSDN may say something, the implementation is what matters in the end because that's what applications depend on. And more often than we would like, applications also depend on some specific implementation behavior, such as event delivery order, specific timestamps etc...
Applications may even have race condition themselves that somehow only manage to succeed because Windows implementation is leaning in some specific way, that makes it less likely to trigger, and we should be trying to do the same.
- Some things might be racy on windows and simply be likely but not guaranteed to be delivered in a certain order, especially when the docs explicitly say that the order is not defined.
And (hypothetically) what if that first sample we request takes a while to load and the `MESourceRateChanged` event goes through first?
You're right about that and although some local testing suggested that it was always sent in some specific order, it seems that running the test on the testbot sometimes shows events in a different order.
As that's the case I'll suggest waiting for the first sample before changing thin mode, and make sure it's free of race conditions.
For other cases where a single request causes multiple events to be sent on both source and stream, it shouldn't matter in which order we wait and check them.
- The windows implementation might change, and since most of this stuff go through the more robust media session, it wouldn't break applications.
I'm also a bit wary of testing for the next event to match something. What if new, unrelated events are added and somehow get in the way?
I think tests are also a way of catching Windows implementation changes in advance, it is valuable to know about changes of the implementation. Also it doesn't seem the implementation has changed in all the Windows versions we test, so I'd say it's unlikely to change much in the future.
- A test that relies on less implementation details could potentially be adapted to run with a bigger matrix of different files or configurations.
In my experience sequential tests tend to stay simpler and are easier to adapt / copy to cover other cases. Generic tests which multiplex streams of events and check multiple things together quickly become complicated, hard to follow, debug or adjust, and in the end less useful.
Another concern: Why make the check for `MEStreamThinMode` depend on `MESourceRateChanged` delivering `VT_R4` values? That's just an unrelated bug/oversight in wine, this MR doesn't even fix it, and at that point I'd rather check for `winetest_platform_is_wine` again.
I'm not that big of a fan of wine checks anyway tho, I'd much rather just make sure to not "eat" random events in case a certain expected event doesn't get delivered.
Yes, it should probably be using `winetest_platform_is_wine` instead, it's supposed to be a temporary fixup anyway until the event is generated.