Reverse |rtx_payload_types| map, and rename.

webrtc/video_receive_stream.h

 /*
  *  Copyright (c) 2013 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 143 matching lines @@
 
       // See UlpfecConfig for description.
       UlpfecConfig ulpfec;
 
       // SSRC for retransmissions.
       uint32_t rtx_ssrc = 0;
 
       // Set if the stream is protected using FlexFEC.
       bool protected_by_flexfec = false;
 
-      // Map from video payload type (apt) -> RTX payload type (pt).
+      // Map from rtx payload type -> media payload type.
       // For RTX to be enabled, both an SSRC and this mapping are needed.
-      std::map<int, int> rtx_payload_types;
+      std::map<int, int> media_pt_by_rtx_pt;

[danilchap, 2017/08/21 14:11:26]

non-common abbreviations in names are strongly discouraged
https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

May be media_by_rtx_payload_type

[nisse-webrtc, 2017/08/22 07:11:01]

My intention was to make the name so explicit that we don't need any comment to
say what the keys and values represent, and then |media_by...| isn't quite good
enough. (And the comment should perhaps be improved, delete the part about keys
and values, and instead explain what's the purpose of the mapping).

I could change to |media_payload_type_by_rtx_payload_type|, but if that is a bit
too unwieldy, I think abbreviating payload_type in the name is clearer than
deleting it. 

And I'd consider "pt" is a reasonably common abbreviation in webrtc, with 178
matches for grep -iw pt...

Suggestions?

[danilchap, 2017/08/22 08:09:55]

I still find 'pt' both 'Only your group knows what this stands for' and
'Ambiguous abbreviation': If you try to search what pt means, you'll find 5
different meanings, none of them is 'payload type'
It might be fine to use it in very small scope, but it is a bad word for large
scope.

Another option is to unuse map to have more descriptive names instead of
first/second instead of putting both of them into variable name, e.g.:

struct RtxBinding {
  int retransmission_payload_type;
  int associated_payload_type;
  // may be later add int rtx_time_ms;
};
std::vector<RtxBinding> rtx_bindings_;

// using names from section 8.1 of the
https://tools.ietf.org/html/rfc4588#page-20

 
       // RTP header extensions used for the received stream.
       std::vector<RtpExtension> extensions;
     } rtp;
 
     // Transport for outgoing packets (RTCP).
     Transport* rtcp_send_transport = nullptr;
 
     // Must not be 'nullptr' when the stream is started.
     rtc::VideoSinkInterface<VideoFrame>* renderer = nullptr;
@@ skipping 52 matching lines @@
   virtual void AddSecondarySink(RtpPacketSinkInterface* sink) = 0;
   virtual void RemoveSecondarySink(const RtpPacketSinkInterface* sink) = 0;
 
  protected:
   virtual ~VideoReceiveStream() {}
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_VIDEO_RECEIVE_STREAM_H_