Allow ANA to receive RPLR (recoverable packet loss rate) indications

Allow ANA to receive RPLR (recoverable packet loss rate) indications

This is part of a series of CLs. Next CLs:
1. CL for RPLR-based FecController
2. CL for allowing experiment-driven configuration of the above (through both field-trials and protobuf)

BUG=webrtc:7058
Review-Url: https://codereview.webrtc.org/2661043003
Cr-Commit-Position: refs/heads/master@{#17368}
Committed: https://chromium.googlesource.com/external/webrtc/+/dadb4dc3c9ee94300f3175a345a8d17ea562e193

[elad.alon_webrtc.org, 2017-01-31 19:05:07]

Description was changed from

==========
Allow ANA to receive RPLR (recoverable packet loss rate) indications

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

to

==========
Allow ANA to receive RPLR (recoverable packet loss rate) indications

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

[elad.alon_webrtc.org, 2017-01-31 19:05:07]

elad.alon@webrtc.org changed reviewers:
+ michaelt@webrtc.org, minyue@webrtc.org, sprang@webrtc.org, stefan@webrtc.org

[elad.alon_webrtc.org, 2017-01-31 19:08:47]

Description was changed from

==========
Allow ANA to receive RPLR (recoverable packet loss rate) indications

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

to

==========
Allow ANA to receive RPLR (recoverable packet loss rate) indications

This is part of a series of CLs. Next CLs:
1. CL for RPLR-based FecController
2. CL for allowing experiment-driven configuration of the above (through both
field-trials and protobuf)

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

[elad.alon_webrtc.org, 2017-02-01 11:20:52]

Ready for review.

[the sun, 2017-02-07 15:47:08]

solenberg@webrtc.org changed reviewers:
+ ossu@webrtc.org

[the sun, 2017-02-07 15:47:37]

On 2017/02/01 11:20:52, elad.alon wrote:
> Ready for review.

Adding ossu@ for his views on changes to the AudioEncoder interface.

[ossu, 2017-02-08 13:32:11]

On 2017/02/07 15:47:37, the sun wrote:
> On 2017/02/01 11:20:52, elad.alon wrote:
> > Ready for review.
> 
> Adding ossu@ for his views on changes to the AudioEncoder interface.

I recall minyue@ giving me the heads-up on this and some background. IIRC, the
recoverable packet-loss rate is calculated with the assumption that each
received packet could be used to recover the previous packet. I'm a bit
concerned that we're assuming too much and putting the calculation of this
information in the wrong place.

How will the recoverable packet-loss rate be used? It seems to me more powerful
if the encoder could be informed of what packages were lost, instead of just
being given percentages. Then it could figure out how much could be recovered
where it to add FEC, for example. Perhaps it could be tuned to fix more than one
packet of loss in some cases?

I also realize this is still under active development, and there's an element of
trying things out that shouldn't necessarily be hampered by finding the perfect
APIs at each step.

[elad.alon_webrtc.org, 2017-03-16 19:00:45]

On 2017/02/08 13:32:11, ossu wrote:
> On 2017/02/07 15:47:37, the sun wrote:
> > On 2017/02/01 11:20:52, elad.alon wrote:
> > > Ready for review.
> > 
> > Adding ossu@ for his views on changes to the AudioEncoder interface.
> 
> I recall minyue@ giving me the heads-up on this and some background. IIRC, the
> recoverable packet-loss rate is calculated with the assumption that each
> received packet could be used to recover the previous packet. I'm a bit
> concerned that we're assuming too much and putting the calculation of this
> information in the wrong place.
> 
> How will the recoverable packet-loss rate be used? It seems to me more
powerful
> if the encoder could be informed of what packages were lost, instead of just
> being given percentages. Then it could figure out how much could be recovered
> where it to add FEC, for example. Perhaps it could be tuned to fix more than
one
> packet of loss in some cases?
> 
> I also realize this is still under active development, and there's an element
of
> trying things out that shouldn't necessarily be hampered by finding the
perfect
> APIs at each step.

ossu@, first - heads up: when you looked at this CL, the ownership of the
tracker was in Channel (VoE). It's since moved to AudioSendStream, from where
the value is passed down to the encoder. (Just to prevent confusion; it doesn't
really make a big difference for your question.)

For now, only Opus uses the RPLR, and it's only concerned with first-order
recoverability. You're right that it would be better to let the encoder get the
feedback message rather than information derived of it. We're currently looking
into moving some observers, including TransportFeedbackAdapterObserver (and the
PL tracker), to the AudioEncoder. If/when this is done, your suggestion will
have been implemented.

[elad.alon_webrtc.org, 2017-03-16 19:01:37]

https://codereview.webrtc.org/2661043003/diff/80001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream.cc (left):

https://codereview.webrtc.org/2661043003/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream.cc:274:
channel_proxy_->OnTwccBasedUplinkPacketLossRate(
Or we could have one function to pass both. I prefer this, but I'm open to
suggestions.

https://codereview.webrtc.org/2661043003/diff/80001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h
(right):

https://codereview.webrtc.org/2661043003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:54:
override;
Minyue, what do you think of s/fraction/rate here? And what about the above
function, to match?

https://codereview.webrtc.org/2661043003/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc
(right):

https://codereview.webrtc.org/2661043003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc:97: //
TODO(elad.alon): Before landing - is there a dump-reading tool in the
michaelt@, that's parse_ana_dump.py, right? Has it made it into the repository
in some form? Maybe the visualizer needs to be updated for this?

[michaelt, 2017-03-20 09:17:23]

https://codereview.webrtc.org/2661043003/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc
(right):

https://codereview.webrtc.org/2661043003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc:97: //
TODO(elad.alon): Before landing - is there a dump-reading tool in the
The Cl for parse_ana_dump.py is uploaded and wait for reviews. 
There should no change to be necessary for your change.

[elad.alon_webrtc.org, 2017-03-20 18:54:05]

https://codereview.webrtc.org/2661043003/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc
(right):

https://codereview.webrtc.org/2661043003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc:97: //
TODO(elad.alon): Before landing - is there a dump-reading tool in the
On 2017/03/20 09:17:23, michaelt wrote:
> The Cl for parse_ana_dump.py is uploaded and wait for reviews. 
> There should no change to be necessary for your change. 
> 

Cool, thanks.

[minyue-webrtc, 2017-03-22 09:20:05]

clean and neat. only small things.

https://codereview.webrtc.org/2661043003/diff/120001/webrtc/modules/audio_cod...
File
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h
(right):

https://codereview.webrtc.org/2661043003/diff/120001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:29:
rtc::Optional<float> uplink_packet_loss_fraction;
Add the comment from the debug_dump.proto here.

https://codereview.webrtc.org/2661043003/diff/120001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:33:
// Some encoders can encode fewer channels than the actual input to make
Copy this comment to debug_dump.proto (I did not do this since I feel it adds
code redundancy, but let's be a bit redundant this time.)

[elad.alon_webrtc.org, 2017-03-22 10:10:07]

Anything else?

https://codereview.webrtc.org/2661043003/diff/120001/webrtc/modules/audio_cod...
File
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h
(right):

https://codereview.webrtc.org/2661043003/diff/120001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:29:
rtc::Optional<float> uplink_packet_loss_fraction;
On 2017/03/22 09:20:05, minyue-webrtc wrote:
> Add the comment from the debug_dump.proto here.

Done.

https://codereview.webrtc.org/2661043003/diff/120001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:33:
// Some encoders can encode fewer channels than the actual input to make
On 2017/03/22 09:20:05, minyue-webrtc wrote:
> Copy this comment to debug_dump.proto (I did not do this since I feel it adds
> code redundancy, but let's be a bit redundant this time.)

Done.

[minyue-webrtc, 2017-03-22 10:11:00]

lgtm

[stefan-webrtc, 2017-03-22 12:15:46]

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.cc:286: if (rplr) {
Can plr and rplr be active simultaneously? Should we comment on why and how that
works? Maybe it's handled somewhere else

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:1319: if (*encoder)
{}

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:376:
channel()->OnRecoverableUplinkPacketLossRate(recoverable_packet_loss_rate);
thread checker?

[minyue-webrtc, 2017-03-22 12:17:36]

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.cc:286: if (rplr) {
On 2017/03/22 12:15:46, stefan-webrtc wrote:
> Can plr and rplr be active simultaneously? Should we comment on why and how
that
> works? Maybe it's handled somewhere else

Yes, plr will be used for frame length adaptation etc, rplr mainly for FEC
tuning.

[stefan-webrtc, 2017-03-22 12:22:11]

ok

lgtm

[stefan-webrtc, 2017-03-22 12:22:44]

lgtm % nits, of course

[ossu, 2017-03-22 13:41:18]

lgtm

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.cc:279: // TODO(elad.alon): Resolve the following
known issue - if PLR goes back
I guess you've considered this: it looks like at least the RPLR is gotten as an
Optional, then stripped and sent along as a float only to eventually be put back
into an Optional. Sending an Optional all the way would give you a way to
invalidate the measurement.

Not going to insist on that here, though. I believe hooking up directly to the
encoder is still the way to go.

[elad.alon_webrtc.org, 2017-03-22 15:15:56]

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.cc:279: // TODO(elad.alon): Resolve the following
known issue - if PLR goes back
On 2017/03/22 13:41:18, ossu wrote:
> I guess you've considered this: it looks like at least the RPLR is gotten as
an
> Optional, then stripped and sent along as a float only to eventually be put
back
> into an Optional. Sending an Optional all the way would give you a way to
> invalidate the measurement.
> 
> Not going to insist on that here, though. I believe hooking up directly to the
> encoder is still the way to go.

I did consider it - even had a CL for it
(https://codereview.webrtc.org/2746333009/) - but after both online and offline
discussion, had to abandon it and place this TODO here instead, for future
handling.

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.cc:286: if (rplr) {
On 2017/03/22 12:17:36, minyue-webrtc wrote:
> On 2017/03/22 12:15:46, stefan-webrtc wrote:
> > Can plr and rplr be active simultaneously? Should we comment on why and how
> that
> > works? Maybe it's handled somewhere else
> 
> Yes, plr will be used for frame length adaptation etc, rplr mainly for FEC
> tuning.

I'm not sure what comment would make things clearer. I mean, they are two
separate metrics, after all. Minyue has given a good example with the
controllers. I'll add that the values might even theoretically be used by
something other than ANA, once it's passed to Channel. Is there a specific
comment you'd recommend?

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:1319: if (*encoder)
On 2017/03/22 12:15:46, stefan-webrtc wrote:
> {}

Done.

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2661043003/diff/160001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:376:
channel()->OnRecoverableUplinkPacketLossRate(recoverable_packet_loss_rate);
On 2017/03/22 12:15:46, stefan-webrtc wrote:
> thread checker?

Done.

[michaelt, 2017-03-23 10:05:01]

lgtm

[elad.alon_webrtc.org, 2017-03-23 18:08:21]

The CQ bit was checked by elad.alon@webrtc.org

[elad.alon_webrtc.org, 2017-03-23 18:08:22]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org,
ossu@webrtc.org, stefan@webrtc.org, michaelt@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2661043003/#ps220001 (title:
"Rebased")

[commit-bot: I haz the power, 2017-03-23 18:08:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-23 18:24:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 18:25:01]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)

[elad.alon_webrtc.org, 2017-03-23 21:53:55]

The CQ bit was checked by elad.alon@webrtc.org

[commit-bot: I haz the power, 2017-03-23 21:54:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[elad.alon_webrtc.org, 2017-03-23 22:04:07]

The CQ bit was checked by elad.alon@webrtc.org

[elad.alon_webrtc.org, 2017-03-23 22:04:07]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org,
ossu@webrtc.org, stefan@webrtc.org, michaelt@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2661043003/#ps240001 (title:
"Uncomment thread-checker to fix UT")

[commit-bot: I haz the power, 2017-03-23 22:04:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-23 22:29:50]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1490306647450570,
"parent_rev": "d1c44356c5d6d572ed660b91af683ed0b136c0cd", "commit_rev":
"dadb4dc3c9ee94300f3175a345a8d17ea562e193"}

[commit-bot: I haz the power, 2017-03-23 22:29:54]

Description was changed from

==========
Allow ANA to receive RPLR (recoverable packet loss rate) indications

This is part of a series of CLs. Next CLs:
1. CL for RPLR-based FecController
2. CL for allowing experiment-driven configuration of the above (through both
field-trials and protobuf)

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

to

==========
Allow ANA to receive RPLR (recoverable packet loss rate) indications

This is part of a series of CLs. Next CLs:
1. CL for RPLR-based FecController
2. CL for allowing experiment-driven configuration of the above (through both
field-trials and protobuf)

BUG=webrtc:7058

Review-Url: https://codereview.webrtc.org/2661043003
Cr-Commit-Position: refs/heads/master@{#17368}
Committed:
https://chromium.googlesource.com/external/webrtc/+/dadb4dc3c9ee94300f3175a34...
==========

[commit-bot: I haz the power, 2017-03-23 22:29:56]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/external/webrtc/+/dadb4dc3c9ee94300f3175a34...