On Tue Nov 15 20:31:01 2022 +0000, Zebediah Figura wrote:
> Linux cuts off thread names at 15 characters (not counting the null
> terminator), which somewhat unfortunately makes all of these identical
> to each other and the deliver threads. Thus far I think Brendan had been
> trying to make the thread names distinct, if not per se trying to fit
> the entire name within 15 characters.
Unfortunate...
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1311#note_15906
On Tue Nov 15 20:31:00 2022 +0000, Zebediah Figura wrote:
> I guess, but it depends on the fact that "read_result" is
> zero-initialized and never written, which seems non-obvious and a little
> fragile. In fact, I think it can already break if a stream was selected
> and then later becomes deselected, even if the filter was stopped in
> between. Checking "stream->read_requested" would be more obvious but I
> think still broken for the same reasons (but simple enough to fix.)
> Note that if you do that then read_result becomes superfluous and
> doesn't really need to be stored anywhere.
`read_result` is written to E_PENDING whenever a read is requested. Every requested read will eventually be completed, writing the result to `read_result`, either `S_OK` or an error.
If a stream was selected and a read requested to it, then later becomes deselected, we will consider it a valid thread wrt sample delivery until it completes its read and writes to `read_result`. It may complete it with a sample if the deselection happened late, or with an error if its read was interrupted.
`read_requested` is a one way flag to tell reader to start reading. It needs to be reset before the `GetNextSample` call, and checked again on return to correctly synchronize flush and read requests from the callback thread, for instance when seeking.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1311#note_15905
On Tue Nov 15 20:30:59 2022 +0000, Zebediah Figura wrote:
> What's the point of this? We never check the return code of
> async_reader_get_next_sample(), only whether it FAILED().
No, it's checked outside of the loop to see if all streams have received EOS (in which case `hr == NS_E_NO_MORE_SAMPLES`), or if there's still some streams with pending requests or pending samples to deliver (`hr == E_PENDING`).
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1311#note_15904
On Tue Nov 15 20:30:59 2022 +0000, Zebediah Figura wrote:
> Missed this the first time, but: I'd make this an ERR, personally. (An
> application can trigger it, but it's one of those cases where it's a
> sign that something went catastrophically wrong, and also we're pretty
> much guessing how to handle it if not.)
The most common case where this happens is on EOS, I don't think we want to print error. When streams are deselected this would happen as well if it interrupts the wait on buffer.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1311#note_15903
On Tue Nov 15 20:30:58 2022 +0000, Zebediah Figura wrote:
> Similarly to the earlier question about IWMSyncReader locking, do we
> need to drop the CS here?
Because this is a blocking call, which can call application callbacks, or otherwise also blocking until there's a sample ready.
We want to be able to deliver samples from the callback thread, on other streams, without being blocked and without blocking the reader. There's no point in making all this asynchronous otherwise.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1311#note_15902
On Tue Nov 15 20:31:01 2022 +0000, Zebediah Figura wrote:
> > We need the read threads to always be running and returning their next
> sample because we then deliver them in PTS order.
> Somewhat out of curiosity, did you check whether this still applies when
> DedicatedDeliveryThread is set?
I believe I did and it still applies (with the same offset as without threads, when samples are decoded), and there's another setting named `DeliverOnReceive` that is the one which changes that behavior (though I don't think we need it).
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1311#note_15901