OnReceivedUplinkPacketLossFraction() receives [const rtc::Optional<float>&]

OnReceivedUplinkPacketLossFraction() receives [const rtc::Optional<float>&]

TWCC-based R/RPLR can go back to unknown, so it needs an rtc::Optional<float> instead of a simple float.

BUG=webrtc:7058

[elad.alon_webrtc.org, 2017-03-16 18:24:09]

Description was changed from

==========
OnReceivedUplinkPacketLossFraction() receives [const rtc::Optional<float>&]

BUG=BUG=webrtc:7058
==========

to

==========
OnReceivedUplinkPacketLossFraction() receives [const rtc::Optional<float>&]

TWCC-based R/RPLR can go back to unknown, so it needs an rtc::Optional<float>
instead of a simple float.

BUG=webrtc:7058
==========

[elad.alon_webrtc.org, 2017-03-16 18:24:09]

elad.alon@webrtc.org changed reviewers:
+ minyue@webrtc.org

[elad.alon_webrtc.org, 2017-03-16 18:24:23]

Please have a look.

[kwiberg-webrtc, 2017-03-16 19:45:53]

kwiberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[kwiberg-webrtc, 2017-03-16 19:45:54]

As a general principle, you almost never want references to Optional. Either
pass the Optional<float> by value, or pass a const float*.

[minyue-webrtc, 2017-03-17 06:55:01]

I think the correct way to do it is simply not calling
OnReceivedUplinkPacketLossFraction if the tracker gives empty. WDYT?

[minyue-webrtc, 2017-03-17 06:59:44]

minyue@webrtc.org changed reviewers:
+ michaelt@webrtc.org

[elad.alon_webrtc.org, 2017-03-17 09:45:34]

On 2017/03/17 06:55:01, minyue-webrtc(OOO_until_Mar19) wrote:
> I think the correct way to do it is simply not calling
> OnReceivedUplinkPacketLossFraction if the tracker gives empty. WDYT?

* About Optional<float> by value or by reference - I assumed it's too small to
be worth the reference, but saw that other places in the code passed by
reference, and opted for being standard. I'll wait for one more reviewer to +1
passing by value instead, then change, okay?
* An unknown value is not possible from RTCP-XR (current source), but it is
possible from TWCC (transport-wide congestion control), due to long periods of
stream inactivity. Then, it's up to the user of the TWCC-packet-loss-tracker to
decide what to do. If I don't pass the unknown value, I'll be effectively making
the decision for him ("no change to existing decisions").

[minyue-webrtc, 2017-03-17 11:48:21]

On 2017/03/17 09:45:34, elad.alon wrote:
> On 2017/03/17 06:55:01, minyue-webrtc(OOO_until_Mar19) wrote:
> > I think the correct way to do it is simply not calling
> > OnReceivedUplinkPacketLossFraction if the tracker gives empty. WDYT?
> 
> * About Optional<float> by value or by reference - I assumed it's too small to
> be worth the reference, but saw that other places in the code passed by
> reference, and opted for being standard. I'll wait for one more reviewer to +1
> passing by value instead, then change, okay?

> * An unknown value is not possible from RTCP-XR (current source), but it is
> possible from TWCC (transport-wide congestion control), due to long periods of
> stream inactivity. Then, it's up to the user of the TWCC-packet-loss-tracker
to
> decide what to do. If I don't pass the unknown value, I'll be effectively
making
> the decision for him ("no change to existing decisions").

In principle, yes, but have you already decided how to use unknown? if not, wait
until you do need it to make this API change. My comments are
1. when ANA (the only use of the output of the tracker for the moment) receives
it, it will be merged into NetworkMetrics, which automatically assign empty to
things that are unknown. So it will be hard for you to distinguish between
"output unknown" and "never get called".
2. API changes requires additional announcement procedure.
3. If we later move the observers into AudioEncoder, and problem is solved
anyway.

[elad.alon_webrtc.org, 2017-03-17 12:28:12]

On 2017/03/17 11:48:21, minyue-webrtc(OOO_until_Mar19) wrote:
> On 2017/03/17 09:45:34, elad.alon wrote:
> > On 2017/03/17 06:55:01, minyue-webrtc(OOO_until_Mar19) wrote:
> > > I think the correct way to do it is simply not calling
> > > OnReceivedUplinkPacketLossFraction if the tracker gives empty. WDYT?
> > 
> > * About Optional<float> by value or by reference - I assumed it's too small
to
> > be worth the reference, but saw that other places in the code passed by
> > reference, and opted for being standard. I'll wait for one more reviewer to
+1
> > passing by value instead, then change, okay?
> 
> > * An unknown value is not possible from RTCP-XR (current source), but it is
> > possible from TWCC (transport-wide congestion control), due to long periods
of
> > stream inactivity. Then, it's up to the user of the TWCC-packet-loss-tracker
> to
> > decide what to do. If I don't pass the unknown value, I'll be effectively
> making
> > the decision for him ("no change to existing decisions").
> 
> In principle, yes, but have you already decided how to use unknown? if not,
wait
> until you do need it to make this API change. My comments are
> 1. when ANA (the only use of the output of the tracker for the moment)
receives
> it, it will be merged into NetworkMetrics, which automatically assign empty to
> things that are unknown. So it will be hard for you to distinguish between
> "output unknown" and "never get called".
> 2. API changes requires additional announcement procedure.
> 3. If we later move the observers into AudioEncoder, and problem is solved
> anyway.

Here's a case I am worried about, which this CL would solve (despite the
output-unknown vs. never-got-called ambiguity):
t0: Suppose a call starts, and FEC is off.
t1: After a while, PLR is high enough that FEC would have been turned on if we
had more bandwidth (which we at that point don't).
t2: After a long period of inactivity, PLR becomes unknown.
t3: PLR is still unknown, but bandwidth had gone up a bit.
Without this CL, because at t3 we think we still have the PLR from t1, we would
turn FEC on (artifact of old state which is no longer relevant). With this CL,
we wouldn't (designed behavior).

[minyue-webrtc, 2017-03-17 12:42:08]

On 2017/03/17 12:28:12, elad.alon wrote:
> On 2017/03/17 11:48:21, minyue-webrtc(OOO_until_Mar19) wrote:
> > On 2017/03/17 09:45:34, elad.alon wrote:
> > > On 2017/03/17 06:55:01, minyue-webrtc(OOO_until_Mar19) wrote:
> > > > I think the correct way to do it is simply not calling
> > > > OnReceivedUplinkPacketLossFraction if the tracker gives empty. WDYT?
> > > 
> > > * About Optional<float> by value or by reference - I assumed it's too
small
> to
> > > be worth the reference, but saw that other places in the code passed by
> > > reference, and opted for being standard. I'll wait for one more reviewer
to
> +1
> > > passing by value instead, then change, okay?
> > 
> > > * An unknown value is not possible from RTCP-XR (current source), but it
is
> > > possible from TWCC (transport-wide congestion control), due to long
periods
> of
> > > stream inactivity. Then, it's up to the user of the
TWCC-packet-loss-tracker
> > to
> > > decide what to do. If I don't pass the unknown value, I'll be effectively
> > making
> > > the decision for him ("no change to existing decisions").
> > 
> > In principle, yes, but have you already decided how to use unknown? if not,
> wait
> > until you do need it to make this API change. My comments are
> > 1. when ANA (the only use of the output of the tracker for the moment)
> receives
> > it, it will be merged into NetworkMetrics, which automatically assign empty
to
> > things that are unknown. So it will be hard for you to distinguish between
> > "output unknown" and "never get called".
> > 2. API changes requires additional announcement procedure.
> > 3. If we later move the observers into AudioEncoder, and problem is solved
> > anyway.
> 
> Here's a case I am worried about, which this CL would solve (despite the
> output-unknown vs. never-got-called ambiguity):
> t0: Suppose a call starts, and FEC is off.
> t1: After a while, PLR is high enough that FEC would have been turned on if we
> had more bandwidth (which we at that point don't).
> t2: After a long period of inactivity, PLR becomes unknown.
> t3: PLR is still unknown, but bandwidth had gone up a bit.
> Without this CL, because at t3 we think we still have the PLR from t1, we
would
> turn FEC on (artifact of old state which is no longer relevant). With this CL,
> we wouldn't (designed behavior).

I don't think this CL alone can solve the problem, you need to change some
design in ANA.

I also don't think without this CL, this problem cannot be solved. ANA is
designed to receive network conditions with arbitrary time gaps. If ANA found
that the last sample was too long ago, it can decide to not rely on it by
itself. (the bandwidth smoother is another example of dealing with arbitrary
sampling interval, see
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_coding/...)

[elad.alon_webrtc.org, 2017-03-17 12:47:40]

On 2017/03/17 12:42:08, minyue-webrtc(OOO_until_Mar19) wrote:
> On 2017/03/17 12:28:12, elad.alon wrote:
> > On 2017/03/17 11:48:21, minyue-webrtc(OOO_until_Mar19) wrote:
> > > On 2017/03/17 09:45:34, elad.alon wrote:
> > > > On 2017/03/17 06:55:01, minyue-webrtc(OOO_until_Mar19) wrote:
> > > > > I think the correct way to do it is simply not calling
> > > > > OnReceivedUplinkPacketLossFraction if the tracker gives empty. WDYT?
> > > > 
> > > > * About Optional<float> by value or by reference - I assumed it's too
> small
> > to
> > > > be worth the reference, but saw that other places in the code passed by
> > > > reference, and opted for being standard. I'll wait for one more reviewer
> to
> > +1
> > > > passing by value instead, then change, okay?
> > > 
> > > > * An unknown value is not possible from RTCP-XR (current source), but it
> is
> > > > possible from TWCC (transport-wide congestion control), due to long
> periods
> > of
> > > > stream inactivity. Then, it's up to the user of the
> TWCC-packet-loss-tracker
> > > to
> > > > decide what to do. If I don't pass the unknown value, I'll be
effectively
> > > making
> > > > the decision for him ("no change to existing decisions").
> > > 
> > > In principle, yes, but have you already decided how to use unknown? if
not,
> > wait
> > > until you do need it to make this API change. My comments are
> > > 1. when ANA (the only use of the output of the tracker for the moment)
> > receives
> > > it, it will be merged into NetworkMetrics, which automatically assign
empty
> to
> > > things that are unknown. So it will be hard for you to distinguish between
> > > "output unknown" and "never get called".
> > > 2. API changes requires additional announcement procedure.
> > > 3. If we later move the observers into AudioEncoder, and problem is solved
> > > anyway.
> > 
> > Here's a case I am worried about, which this CL would solve (despite the
> > output-unknown vs. never-got-called ambiguity):
> > t0: Suppose a call starts, and FEC is off.
> > t1: After a while, PLR is high enough that FEC would have been turned on if
we
> > had more bandwidth (which we at that point don't).
> > t2: After a long period of inactivity, PLR becomes unknown.
> > t3: PLR is still unknown, but bandwidth had gone up a bit.
> > Without this CL, because at t3 we think we still have the PLR from t1, we
> would
> > turn FEC on (artifact of old state which is no longer relevant). With this
CL,
> > we wouldn't (designed behavior).
> 
> I don't think this CL alone can solve the problem, you need to change some
> design in ANA.
> 
> I also don't think without this CL, this problem cannot be solved. ANA is
> designed to receive network conditions with arbitrary time gaps. If ANA found
> that the last sample was too long ago, it can decide to not rely on it by
> itself. (the bandwidth smoother is another example of dealing with arbitrary
> sampling interval, see
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_coding/...)

I think this CL solves the problem I have presented already. I could send you a
patch that combines this CL with the next ones, that might make it more
apparent.

[elad.alon_webrtc.org, 2017-03-17 18:42:24]

On 2017/03/17 12:47:40, elad.alon wrote:
> On 2017/03/17 12:42:08, minyue-webrtc(OOO_until_Mar19) wrote:
> > On 2017/03/17 12:28:12, elad.alon wrote:
> > > On 2017/03/17 11:48:21, minyue-webrtc(OOO_until_Mar19) wrote:
> > > > On 2017/03/17 09:45:34, elad.alon wrote:
> > > > > On 2017/03/17 06:55:01, minyue-webrtc(OOO_until_Mar19) wrote:
> > > > > > I think the correct way to do it is simply not calling
> > > > > > OnReceivedUplinkPacketLossFraction if the tracker gives empty. WDYT?
> > > > > 
> > > > > * About Optional<float> by value or by reference - I assumed it's too
> > small
> > > to
> > > > > be worth the reference, but saw that other places in the code passed
by
> > > > > reference, and opted for being standard. I'll wait for one more
reviewer
> > to
> > > +1
> > > > > passing by value instead, then change, okay?
> > > > 
> > > > > * An unknown value is not possible from RTCP-XR (current source), but
it
> > is
> > > > > possible from TWCC (transport-wide congestion control), due to long
> > periods
> > > of
> > > > > stream inactivity. Then, it's up to the user of the
> > TWCC-packet-loss-tracker
> > > > to
> > > > > decide what to do. If I don't pass the unknown value, I'll be
> effectively
> > > > making
> > > > > the decision for him ("no change to existing decisions").
> > > > 
> > > > In principle, yes, but have you already decided how to use unknown? if
> not,
> > > wait
> > > > until you do need it to make this API change. My comments are
> > > > 1. when ANA (the only use of the output of the tracker for the moment)
> > > receives
> > > > it, it will be merged into NetworkMetrics, which automatically assign
> empty
> > to
> > > > things that are unknown. So it will be hard for you to distinguish
between
> > > > "output unknown" and "never get called".
> > > > 2. API changes requires additional announcement procedure.
> > > > 3. If we later move the observers into AudioEncoder, and problem is
solved
> > > > anyway.
> > > 
> > > Here's a case I am worried about, which this CL would solve (despite the
> > > output-unknown vs. never-got-called ambiguity):
> > > t0: Suppose a call starts, and FEC is off.
> > > t1: After a while, PLR is high enough that FEC would have been turned on
if
> we
> > > had more bandwidth (which we at that point don't).
> > > t2: After a long period of inactivity, PLR becomes unknown.
> > > t3: PLR is still unknown, but bandwidth had gone up a bit.
> > > Without this CL, because at t3 we think we still have the PLR from t1, we
> > would
> > > turn FEC on (artifact of old state which is no longer relevant). With this
> CL,
> > > we wouldn't (designed behavior).
> > 
> > I don't think this CL alone can solve the problem, you need to change some
> > design in ANA.
> > 
> > I also don't think without this CL, this problem cannot be solved. ANA is
> > designed to receive network conditions with arbitrary time gaps. If ANA
found
> > that the last sample was too long ago, it can decide to not rely on it by
> > itself. (the bandwidth smoother is another example of dealing with arbitrary
> > sampling interval, see
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_coding/...)
> 
> I think this CL solves the problem I have presented already. I could send you
a
> patch that combines this CL with the next ones, that might make it more
> apparent.

Correction: As discussed offline, a small additional change would indeed be
needed to make sure the controller uses the unknown to override its already set
value. IMHO, this is the way to go, though - I think that the decision to
forget/retain the value from before the window emptied and the R/PLR went back
to unknown, should be in ANA, and not in the metrics provider of ANA.

[minyue-webrtc, 2017-03-17 19:29:33]

On 2017/03/17 18:42:24, elad.alon wrote:
> On 2017/03/17 12:47:40, elad.alon wrote:
> > On 2017/03/17 12:42:08, minyue-webrtc(OOO_until_Mar19) wrote:
> > > On 2017/03/17 12:28:12, elad.alon wrote:
> > > > On 2017/03/17 11:48:21, minyue-webrtc(OOO_until_Mar19) wrote:
> > > > > On 2017/03/17 09:45:34, elad.alon wrote:
> > > > > > On 2017/03/17 06:55:01, minyue-webrtc(OOO_until_Mar19) wrote:
> > > > > > > I think the correct way to do it is simply not calling
> > > > > > > OnReceivedUplinkPacketLossFraction if the tracker gives empty.
WDYT?
> > > > > > 
> > > > > > * About Optional<float> by value or by reference - I assumed it's
too
> > > small
> > > > to
> > > > > > be worth the reference, but saw that other places in the code passed
> by
> > > > > > reference, and opted for being standard. I'll wait for one more
> reviewer
> > > to
> > > > +1
> > > > > > passing by value instead, then change, okay?
> > > > > 
> > > > > > * An unknown value is not possible from RTCP-XR (current source),
but
> it
> > > is
> > > > > > possible from TWCC (transport-wide congestion control), due to long
> > > periods
> > > > of
> > > > > > stream inactivity. Then, it's up to the user of the
> > > TWCC-packet-loss-tracker
> > > > > to
> > > > > > decide what to do. If I don't pass the unknown value, I'll be
> > effectively
> > > > > making
> > > > > > the decision for him ("no change to existing decisions").
> > > > > 
> > > > > In principle, yes, but have you already decided how to use unknown? if
> > not,
> > > > wait
> > > > > until you do need it to make this API change. My comments are
> > > > > 1. when ANA (the only use of the output of the tracker for the moment)
> > > > receives
> > > > > it, it will be merged into NetworkMetrics, which automatically assign
> > empty
> > > to
> > > > > things that are unknown. So it will be hard for you to distinguish
> between
> > > > > "output unknown" and "never get called".
> > > > > 2. API changes requires additional announcement procedure.
> > > > > 3. If we later move the observers into AudioEncoder, and problem is
> solved
> > > > > anyway.
> > > > 
> > > > Here's a case I am worried about, which this CL would solve (despite the
> > > > output-unknown vs. never-got-called ambiguity):
> > > > t0: Suppose a call starts, and FEC is off.
> > > > t1: After a while, PLR is high enough that FEC would have been turned on
> if
> > we
> > > > had more bandwidth (which we at that point don't).
> > > > t2: After a long period of inactivity, PLR becomes unknown.
> > > > t3: PLR is still unknown, but bandwidth had gone up a bit.
> > > > Without this CL, because at t3 we think we still have the PLR from t1,
we
> > > would
> > > > turn FEC on (artifact of old state which is no longer relevant). With
this
> > CL,
> > > > we wouldn't (designed behavior).
> > > 
> > > I don't think this CL alone can solve the problem, you need to change some
> > > design in ANA.
> > > 
> > > I also don't think without this CL, this problem cannot be solved. ANA is
> > > designed to receive network conditions with arbitrary time gaps. If ANA
> found
> > > that the last sample was too long ago, it can decide to not rely on it by
> > > itself. (the bandwidth smoother is another example of dealing with
arbitrary
> > > sampling interval, see
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_coding/...)
> > 
> > I think this CL solves the problem I have presented already. I could send
you
> a
> > patch that combines this CL with the next ones, that might make it more
> > apparent.
> 
> Correction: As discussed offline, a small additional change would indeed be
> needed to make sure the controller uses the unknown to override its already
set
> value. IMHO, this is the way to go, though - I think that the decision to
> forget/retain the value from before the window emptied and the R/PLR went back
> to unknown, should be in ANA, and not in the metrics provider of ANA.

As said in #11, ANA is designed to receive network conditions with arbitrary
time gaps. This also makes sense in the case of TWCC packet loss tracker. The
empty output of TWCC packet loss tracker is "I don't have evidence to estimate
packet loss rate", this is not limited to happen when there is a long pause, but
can also happen any time. If there is no packet loss estimate, why to inform
ANA. Adding meaning to empty field will cause some conceptual problem to ANA.

Also as said in #12, if long pause is a critical thing, it can be handled in ANA
level. It can be achieved with a thought of "I have not been receiving TWCC
packet loss tracker for so long time that I cannot rely on its last output"

[elad.alon_webrtc.org, 2017-03-20 17:51:17]

As per offline discussion, a TODO will be added in the following CL (which would
otherwise have used this), and the problem mentioned would be addressed later
(probably after the tracker has been moved to the encoder).