Add insertedSamplesForDeceleration and removedSamplesForAcceleration

[henbos]

Fixes #407.

[vr000m]

Why would the JB not playout packets slowly instead of inserting fake samples? what about video?

[ivocreusen]

Adding samples is how a JB plays audio slower. Playing audio slowly means that more samples will come out of the jitter buffer than went into it. This stat counts the number of samples that were added for the purpose of playing the audio slower than normal.

…

On Mon, Mar 18, 2019 at 1:10 PM Varun Singh ***@***.***> wrote: Why would the JB not playout packets slowly instead of inserting samples? what about video? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#408 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARGCovB7ITueu3pUVJoYFlwnwMPwBT5hks5vX4JFgaJpZM4b5WP7> .

[vr000m]

I thought most Jitter Buffer (JB) were timestamp oriented, i.e., the JB would push out to the decoder at the indicated timestamp. My understanding was as follows -- If a JB is going to underflow (assume there is a low water mark threshold which triggers before underflow actually occurs), the JB would add a constant delay to each audio packet/ video frame, thus slowing down the time to underflow because the JB is not triggering at the intended timeout but delaying it.

The way I read the proposal here is suggesting (AFAICT) that the JB sends the audio packets out for decoding at the intended and in the Playout Buffer (PB) where data is played out at the sampling rate, silent samples are added to slow down playback?

Is my understanding of your proposal correct?

[vr000m]

Why do we need acceleratedSamples? would the jitter buffer (JB) not discard the packet earlier before sending it to playout? Is there a reason to send the packet from the JB even though it was late? simply put, why wouldn't packetsDiscarded not work for this case?

[vr000m]

How is deceleratedSamples different from concealedSamples?

[ivocreusen]

There are different ways of achieving faster/slower audio. In the description of the stat we have not specified what the content is of the samples that are added/removed. In Chrome's WebRTC implementation we try to find a repeating pattern in the audio and then we add one or more extra repetition to slow down the audio (or remove on or more repetitions to speed it up). There are various alternative methods that could be used to achieve the same effect. However, the one thing that any method of increasing or reducing the speed of audio has in common is that samples must be added/removed, which is what the stats are supposed to reflect. In normal operation, the task of the JB is to maintain an adequate buffer level to be able to handle network jitter. If network conditions change during a call, the JB needs to respond to this change by increasing or reducing the buffer level. This can be achieved by increasing/decreasing the playout speed of the audio. These operations don't have to operate on a per-packet basis, it is entirely possible to add/remove any number of samples after the packets have been decoded (and this is exactly what we do in Chrome's WebRTC). There is no reason for the audio in the playout buffer to be aligned with the incoming audio packets. deceleratedSamples is similar to concealedSamples, but they are not the same. Packet loss concealment (concealedSamples) happens when the packet containing the necessary audio hasn't been received, and can last for a long time (network freeze) and result in significant audio degradation (e.g. robot voice). This should be rare if the user is on a good connection. On the other hand, deceleratedSamples is part of the normal operation for a jitter buffer and happens when the JB wants to increase its buffer level to adapt to changing network conditions. A good implementation is almost inaudible for users.

…

On Mon, Mar 18, 2019 at 3:30 PM Varun Singh ***@***.***> wrote: How is deceleratedSamples different from concealedSamples? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#408 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARGCoreZhV_MNWx7ef_8Osl5fPeIH1W5ks5vX6MKgaJpZM4b5WP7> .

[henbos]

Please take another look @vr000m, see Ivo's comment.

Repeating some key points of the above:

• This is not implementation-specific: "The one thing that any method of increasing or reducing the speed of audio has in common is that samples must be added/removed, which is what the stats are supposed to reflect."
• Packet loss concealment happen when we have no samples for playout, whereas deceleratedSamples are samples added to prevent us from reaching such a state. There is an audible difference, and they are not the same thing.
• It is useful to know all of these different metrics, which is one of the reasons people still use legacy goog-stats.

[alvestrand]

Alternate text: "When playout is slowed down, this counter is increased by the difference between the number of samples received and the number of samples played out in the interval. If playout is slowed down by inserting samples, this will be the number of inserted samples."

[henbos]

Name: insertedSamples
Rephrase: Decelerating is a consequence, not a definition.

[henbos]

I talked with Chen and things to note:

• Samples added or removed for acceleration/deceleration may be based on true samples. When "removing samples" we are not simply discarding samples, how we end up with less samples is up to the implementation and may involve analysing all of the samples that we did receive.
• If the name is as vague as "insert/add" it sounds like this would include concealedSamples, which is not the case.
• If the name is as vague as "remove/discard" this sounds like it would include buffer flushes, which is not the case.
• We are talking about samples for playout, not adding or removing to the jitter buffer per-se.

Conclusion:

• Harald's definition is good.
• The name should include "accelerate/decelerate" or similar, to avoid confusion with other reasons for adding or removing.

[henbos]

Suggestion:

Name: insertedDeceleratedSamples
Description: Harald's definition

[alvestrand]

When playout is speeded up, this counter is increased by the difference between the number of samples received and the number of samples played out. If speedup is achieved by discarding samples, this will be the count of samples discarded."

[henbos]

Name: removedSamples
Rephrase: Accelerate is a consequence, not a definition.

[henbos]

Relating to my comment above, saying "removed" is better than saying "discarded", because discarded sounds like that data was not examined at all.

[henbos]

Suggestion:

Name: removedAcceleratedSamples
Description: Harald's definition but replace "discarded" with "removed"

[henbos]

@vr000m to please make suggestions

[henbos]

Based on input from comments and Chen Xing the PR is updated. Please take another look. @alvestrand

@vr000m note that the case discussed of "what if samples are inserted only to be removed?" is not applicable, as this happens after the jitter buffer. You might add or remove before playout, but not both. The new definition is also more flexible about implementation strategy.

The new names still make it clear that these counters have to do with acceleration and deceleration, which is necessary as to not include concealedSamples or jitter buffer flushes as "inserted" or "removed" samples.

[vr000m]

@henbos thanks for the update, these definitions make more sense to me and allay my concerns. Do we really need For* in the metric names?

[vr000m]

I have another question, are the insertions and removals happening at playback clockrate or at packet arrival, the reason for asking is -- "would make sense to count the number of times the decision to accelerate/decelerate is made?"

to calculate something like average number of insertions or removals?

[vr000m]

Do we need For* in the names? otherwise LGTM

[henbos]

Do we need For* in the names? otherwise LGTM

I think so. For example, if the acceleration name is "removedSamples" instead of "removedSamplesForAcceleration" it sounds by the name like this applies to any removed samples, but we don't want this to include jitter buffer flushes. This specifically measures sample differences related to acceleration and deceleration, so this should be called out in the name.

@vr000m Requesting that you merge this. I fixed the nit, but did not change the names.

[henbos]

I have another question, are the insertions and removals happening at playback clockrate or at packet arrival, the reason for asking is -- "would make sense to count the number of times the decision to accelerate/decelerate is made?"

to calculate something like average number of insertions or removals?

I would discuss this separately, but I don't think so. I think the number of times a decision is made to accelerate or decelerate would be implementation specific and if acceleration/deceleration is tiny I think the number of times it happens doesn't tell you anything, versus it happening not very often but happening at a great magnitude.

I think what is interesting to calculate is the average number of accelerated/decelerated samples compared to the number of samples received, and that does not require any new counters, we already have samplesReceived.

[alvestrand]

I think this looks Good Enough now.

[vr000m]

if the behaviour is unknown for how often it happens, we should indeed wait for deployment experience for this.