Reducing calling to SmoothingFilter in Fec Controller.

webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h

 /*
  *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 69 matching lines @@
   struct ThresholdInfo {
     explicit ThresholdInfo(const Config::Threshold& threshold);
     float slope;
     float offset;
   };
 
   float GetPacketLossThreshold(int bandwidth_bps,
                                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_;
   std::unique_ptr<SmoothingFilter> packet_loss_smoothed_;
 
   const ThresholdInfo fec_enabling_threshold_info_;
   const ThresholdInfo fec_disabling_threshold_info_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(FecController);
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_AUDIO_CODING_AUDIO_NETWORK_ADAPTOR_FEC_CONTROLLER_H_