[haiku-bugs] Re: [Haiku] #7285: BSoundPlayer sends buffers late, with ever-increasing latency

#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.

Other related posts: