Reducing calling to SmoothingFilter in Fec Controller.

webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h

Index: webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h
diff --git a/webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h b/webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h
index 62a3533a7591a15fc6c98047dbdfb04fbdff918f..ac4b1e236a0fc45bff517061613dd9d77b0d8ada 100644
--- a/webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h
+++ b/webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h
@@ -87,8 +87,10 @@ class FecController final : public Controller {
                                const Config::Threshold& threshold,
                                const ThresholdInfo& threshold_info) const;
 
-  bool FecEnablingDecision(const NetworkMetrics& metrics) const;
-  bool FecDisablingDecision(const NetworkMetrics& metrics) const;
+  bool FecEnablingDecision(const NetworkMetrics& metrics,
+                           const rtc::Optional<float>& packet_loss) const;

[hlundin-webrtc, 2016/12/22 10:42:39]

I think you can pass an Optional<float> by value.

[minyue-webrtc, 2016/12/27 13:43:33]

I am not sure about the efficiency between ref and copy when comes to Optional.
I'd like to learn about it.

I can commit it as it is now but would like to discuss with you about it and
possibly do an update in a following patch.

[hlundin-webrtc, 2017/01/09 14:46:16]

I think the argument from ArrayView applies here as well:
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/base/array_....

That is, the Optional type is "tiny", making it trivial to copy, and thus not
worth to bother with pointers or references. But I usually call on kwiberg@ to
keep me honest in these cases...

[kwiberg-webrtc, 2017/01/09 14:55:19]

I'm pretty sure Henrik is right (and if I were writing this code, I would've
just passed it by value). But I'll see if I can whip up a small test to verify
this...

(By the way, for non-tiny types, passing a const T* to the function is usually
the optimal choice. It's just like a const reference, except that it can be
null...)

[kwiberg-webrtc, 2017/01/10 02:19:28]

So, I did the experiment, and it seems like even for something as simple as
Optional<float>, the compiler won't use registers for argument passing.
(Presumably, this is because it's a union and not a struct under the hood. Or
something.) So for any type T, large or small,

  1. Argument type const Optional<T>& is slightly better than const T* if the
caller already has an Optional<T>. (It wins by a few instructions, including one
conditional branch.)

  2. Argument type const T* is much better than const Optional<T>& if the caller
doesn't already have an Optional<T>. (It wins by the cost of constructing an
Optional<T>, which includes a conditional branch and making a copy of a T.)

  3. Argument type Optional<T> is always at least as bad as the other two
options, since it has to make an extra copy of the Optional, which costs a
conditional branch and making a copy of a T.

So, recommendation: Never use an Optional<T> argument. Almost always use a const
T* argument, except when you want to optimize hard to save a few instructions
for the case where the caller already has an Optional<T> (in which case you
should use const Optional<T>&).

Moral of the story: Note how my original intuition was to go with the
alternative that turned out to be the worst of the three. https://goo.gl/JWVCxC

[hlundin-webrtc, 2017/01/10 08:34:07]

Wow! Interesting findings. I stand corrected.

+  bool FecDisablingDecision(const NetworkMetrics& metrics,
+                            const rtc::Optional<float>& packet_loss) const;

[hlundin-webrtc, 2016/12/22 10:42:39]

And here.

 
   const Config config_;
   bool fec_enabled_;