#7285: BSoundPlayer sends buffers late, with ever-increasing latency ------------------------------+----------------------- Reporter: Pete | Owner: axeld Type: bug | Status: new Priority: normal | Milestone: R1 Component: Kits/Media Kit | Version: R1/alpha2 Resolution: | Keywords: Blocked By: | Blocking: Has a Patch: 1 | Platform: All ------------------------------+----------------------- Comment (by bonefish): Replying to [comment:13 Pete]: > Replying to [comment:12 bonefish]: > > Replying to [comment:11 Pete]: > > > (The actual calculation of "lateness" was at one point moved to that point at the beginning of the loop, but it seems better that the true lateness when the buffer is returned from the loop be provided, so this check is still done after wakeup, as it was originally.) > > > > Due to the violation of the seamless latency computation it is still incorrect, though. If the behavior is worse with the correct implementation, there's obviously something else wrong. Since after with your changes the mixer completely ignores the value computed here, the issue is probably in a different node up- or downstream. > It was not "worse" -- just ineffective. My thesis is that there is ''no possible'' "correct implementation" of lateness calculation in ControlLoop()! [...] > Now let's suppose a blackout occurs, resulting in a late wakeup. Then using a lateness calculated after the wakeup to affect upstream latency is obviously wrong, because the delay in the loop gets reported as if it was upstream. However, the other setup is no better, because if buffer duration is short there will be other buffers in the queue; a late wakeup will mean that -- though the first buffer will not be "late" because its lateness was determined before the wait -- the next buffer is ''looked at'' late when the loop returns to the top after the wait, so computing lateness at that point has just the same result: again "lateness" will be detected and the cause will erroneously be attributed to the producer. (If buffer duration is longer than the latency, the early test does work, because the loop goes into infinite timeout after passing on a buffer, and is woken by the next one's arrival.) Agreed. > > > '''AudioMixer''':[[BR]] > > > Checking the lateness of incoming buffers -- and notifying the producer -- is now done in BufferReceived, rather than when the buffer is returned from the loop in HandleInputBuffer. > > > > Well, that is a work-around for the incorrect lateness computation in `BMediaEventLooper::ControlLoop()`. > See above... (:-)) Why do you think this is the wrong place to do the check? Upstream latency should only be changed when the producer is late (and blackouts do also happen in the producer's looper). So testing when the buffer actually arrives at the mixer seems the obvious (and correct!) place to do the check. I think any place outside libbe is suboptimal for the lateness computation. BMediaEventLooper already provides a lateness value. To let that be computed incorrectly and have the API user compute it themselves is not a solution. I have nothing against basing the lateness computation on a time stamp from the receiving thread. Unfortunately the event doesn't go through the `BMediaEventLooper`, but is added directly to the event queue. A reasonable solution would be to add a `queued_time` field to `media_timed_event`, filled in when the event is added. That means `BTimedEventQueue` would need to allow for optionally specifying a `BTimeSource`. The actual lateness computation would still happen in `BMediaEventLooper::ControlLoop()`, just based on the event's `queued_time`. > > > There is a new test in the latter for actual lateness ( wrt downstream latency -- ''not'' the total event latency) which will adjust the Mixer's internal latency. > > > > While it doesn't harm, I don't think this is necessary either. I would rather see the lateness be computed correctly in `BMediaEventLooper` and handled here instead of in `BufferReceived()`. > Again, see above... You too. :-) > In fact, I think this would properly be dealt with by LateNoticeReceived, but as I say I didn't seem to be seeing any activity there. I felt a "belt and braces" approach was safer for the moment. We don't want something that works just because we have added enough work- arounds. We want it to work because it is correct. In fact adding work- arounds usually makes it more complicated to find the actual problems. > As you say, it does no harm (and works in practice...). No-ops don't harm either. Still we don't want them in the code. > > That only leaves the wait itself, which should be covered by the scheduling latency. Though I wouldn't bother adjusting it, but rather let the internal latency compensate. The internal latency could be checked after the mixing has happened (i.e. whether the lateness after mixing is greater than the greatest lateness of the processed input buffers (using the lateness value computed by `BMediaEventLooper`)). > Not sure exactly what you're suggesting here. I'm suggesting that, for the time being, all that is needed is the lateness handling in `HandleInputBuffer()`. If you or anyone else feels motivated, one could add comparisons of the actual buffer processing times with the internal latency and increase the latter, if necessary. Due to the mixer's nature of having multiple inputs with possibly different formats and buffer durations, this might turn out to be a bit complicated. I'm also not sure whether this would actually improve things in practice. > Note, though, that using the looper's lateness to calculate internal latency is no use either, because the looper only knows about (total) event latency (and scheduling latency). If you adjust internal latency and thus event latency, all calculations come out just as before (it will compute both sleep time and lateness based on the event latency, so short buffers especially can still generate spurious lateness). So each blackout will incur another bump. I don't follow. "All calculations" certainly don't come out the same: The wakeup time will be earlier, which is the whole point of increasing the internal latency. The lateness has nothing to do with our internal latency (other than shifting the requested buffer arrival times), so, of course, it shouldn't be affected. Regarding the blackout, I thought we agreed on that: As long as the latency/lateness computation is done seamlessly without attributing wait times (be it actual wait times or times where the thread is busy with another event) to the upstream latency the blackout time will increase the right latencies until they completely compensate for blackouts. And computing the lateness as suggested (based on a time stamp in `BTimedEventQueue::AddEvent()`), that will be the case: - A blackout anytime before getting the time stamp in `BTimedEventQueue::AddEvent()` will increase the lateness and will cause the upstream node's latency to be increased until buffers arrive up to a full blackout time early. Another blackout will simply result in 0 lateness. - A blackout anytime after getting the time stamp -- be it even before actually queuing the event or anywhere in the looper thread -- will not affect the computed lateness and will thus not increase the upstream node's latency. The buffer will be processed and sent downstream where it will possibly arrive late, which will cause a late notice and an increase of the internal latency. Eventually the internal latency will have been increased by a full blackout time. Hence the looper will schedule the wakeup time a full blackout time earlier. Thereafter: - A blackout before starting to wait will simply cause the event to be pulled out of the queue without wait at wakeup time + blackout time. Since the internal latency includes a full blackout time, there's still enough time to process and send the buffer so that it arrives in time. - A blackout while or immediately after waiting will cause the event to be pulled out of the queue at the latest at wakeup time + blackout time, so, again, in time for the downstream node. - In case of a later blackout the event will be pulled out of the queue at wakeup time, so there's a full additional blackout time left for processing and sending the buffer, which compensates for the blackout. So in neither case latencies will be increased unboundedly. > > > LateNoticeReceived will also adjust internal latency, but as far as I can tell is never invoked! > > That's bad and should be fixed. > I'm not exactly sure of the flow sequence here. I think MixerInput is the next in line? No, that's just part of the mixer. The whole mixer is a single node. Next upstream you usually already have the audio producer. > I presume MultiAudio gets the actual mixer output, but that's probably too far down the line. Nope, that's usually already the next one downstream. Just check with Cortex. > > > In all cases, lateness is only acted upon if it is an ''initial'' report, as multiple buffers following in the queue will also be flagged late. The fLastNotification variable has been redirected for this purpose, as its original use in LateNoticeReceived seemed to be pointless. > > > > The purpose of `fLastLateNotification` is to prevent a reaction to a late notice before the latency adjustment caused by the previous late notice could have taken effect. Looking at the code I'd say the value it is currently set to (`TimeSource()->Now()`) should be increased by the downstream and internal latency, since any buffer that already has been or is currently being processed won't benefit from the latency change. So in that your change is correct, but the respective change in `HandleInputBuffer()` seems superfluous at best. > ("Now"+downstream+internal) should be approximately the same as the buffer start time by my calculation, so I chose that for convenience. > The `HandleInputBuffer()` code section is intended to compensate for queued buffers all being affected by a single blackout, To clarify what I mean: This code is insufficient, because only the late notice receiver can really decide whether an internal latency change due to a previous late notice happened before or after processing the buffer causing the current late notice. And since the late notice receiver has to do that anyway (haven't checked whether it actually does in this case), the code in `HandleInputBuffer()` is superfluous. Apropos, latency change: I noticed that `BMediaEventLooper::SetEventLatency()` does not wake up the looper thread, which means that the event currently waited on will not be affected by the latency change. -- Ticket URL: <http://dev.haiku-os.org/ticket/7285#comment:15> Haiku <http://dev.haiku-os.org> Haiku - the operating system.