Reverse |rtx_payload_types| map, and rename.

Reverse |rtx_payload_types| map, and rename.

New name is |rtx_associated_payload_types|.

BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/3000273002
Cr-Commit-Position: refs/heads/master@{#19514}
Committed: https://chromium.googlesource.com/external/webrtc/+/26e3abbb40d5fc24642ed61ca226810713084dec

[nisse-webrtc, 2017-08-21 13:20:38]

nisse@webrtc.org changed reviewers:
+ brandtr@webrtc.org, danilchap@webrtc.org

[nisse-webrtc, 2017-08-21 13:20:39]

PTAL. This mapping direction is what's needed for the per-packet processing.

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

https://codereview.webrtc.org/3000273002/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/3000273002/diff/1/webrtc/call/call.cc#newcode90
webrtc/call/call.cc:90: template <typename K, typename V>
may be do not use template until you'll use it for different K and V

https://codereview.webrtc.org/3000273002/diff/1/webrtc/call/call.cc#newcode91
webrtc/call/call.cc:91: const K* find_by_value(const std::map<K, V>& m, V v) {
FindByValue
https://google.github.io/styleguide/cppguide.html#Function_Names
Or may be FindKeyByValue (since result is pointer to key, not iterator)

https://codereview.webrtc.org/3000273002/diff/1/webrtc/call/call.cc#newcode110
webrtc/call/call.cc:110: auto search =
find_by_value(config.rtp.media_pt_by_rtx_pt, d.payload_type);
since search is no longer a long-named iterator, use exact type: 'const int*'
(or at least auto*)

https://codereview.webrtc.org/3000273002/diff/1/webrtc/video/rtp_video_stream...
File webrtc/video/rtp_video_stream_receiver.cc (right):

https://codereview.webrtc.org/3000273002/diff/1/webrtc/video/rtp_video_stream...
webrtc/video/rtp_video_stream_receiver.cc:152: RTC_DCHECK(kv.second != 0);
change DCHECK to validate rtx payload type (media payload type 0 is common for
audio)

https://codereview.webrtc.org/3000273002/diff/1/webrtc/video_receive_stream.h
File webrtc/video_receive_stream.h (right):

https://codereview.webrtc.org/3000273002/diff/1/webrtc/video_receive_stream.h...
webrtc/video_receive_stream.h:166: std::map<int, int> media_pt_by_rtx_pt;
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:02]

https://codereview.webrtc.org/3000273002/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/3000273002/diff/1/webrtc/call/call.cc#newcode90
webrtc/call/call.cc:90: template <typename K, typename V>
On 2017/08/21 14:11:26, danilchap wrote:
> may be do not use template until you'll use it for different K and V

Done.

https://codereview.webrtc.org/3000273002/diff/1/webrtc/call/call.cc#newcode91
webrtc/call/call.cc:91: const K* find_by_value(const std::map<K, V>& m, V v) {
On 2017/08/21 14:11:26, danilchap wrote:
> FindByValue
> https://google.github.io/styleguide/cppguide.html#Function_Names
> Or may be FindKeyByValue (since result is pointer to key, not iterator)

Done.

https://codereview.webrtc.org/3000273002/diff/1/webrtc/call/call.cc#newcode110
webrtc/call/call.cc:110: auto search =
find_by_value(config.rtp.media_pt_by_rtx_pt, d.payload_type);
On 2017/08/21 14:11:26, danilchap wrote:
> since search is no longer a long-named iterator, use exact type: 'const int*'
> (or at least auto*)

Done.

https://codereview.webrtc.org/3000273002/diff/1/webrtc/video/rtp_video_stream...
File webrtc/video/rtp_video_stream_receiver.cc (right):

https://codereview.webrtc.org/3000273002/diff/1/webrtc/video/rtp_video_stream...
webrtc/video/rtp_video_stream_receiver.cc:152: RTC_DCHECK(kv.second != 0);
On 2017/08/21 14:11:26, danilchap wrote:
> change DCHECK to validate rtx payload type (media payload type 0 is common for
> audio)

Done, also changed to RTC_DCHECK_NE.

https://codereview.webrtc.org/3000273002/diff/1/webrtc/video_receive_stream.h
File webrtc/video_receive_stream.h (right):

https://codereview.webrtc.org/3000273002/diff/1/webrtc/video_receive_stream.h...
webrtc/video_receive_stream.h:166: std::map<int, int> media_pt_by_rtx_pt;
On 2017/08/21 14:11:26, danilchap wrote:
> 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

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]

https://codereview.webrtc.org/3000273002/diff/1/webrtc/video_receive_stream.h
File webrtc/video_receive_stream.h (right):

https://codereview.webrtc.org/3000273002/diff/1/webrtc/video_receive_stream.h...
webrtc/video_receive_stream.h:166: std::map<int, int> media_pt_by_rtx_pt;
On 2017/08/22 07:11:01, nisse-webrtc wrote:
> On 2017/08/21 14:11:26, danilchap wrote:
> > 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
> 
> 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?
> 

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

[nisse-webrtc, 2017-08-22 08:37:39]

On 2017/08/22 08:09:55, danilchap wrote:
> 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.

I see.
 
> 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.:

I prefer map because the primary use is lookup on the key, and since it's used
in the per-packet processing, it's nice to use a type which kind-of promises to
be the most efficient for that.
I could write a class around that which has better names and methods like
MediaPayloadTypeByRtxPayloadType (forward lookup) and
RtxPayloadTypeByMediaPayloadType (less efficient reverse lookup). But that seems
like overkill, and I had planned on not reinventing the RTPPayloadRegistry
class.

> 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

Using terminology matching the spec is nice. But as far as I see, "associated"
is without a clear direction, it's more of a symmetric relation, so it doesn't
help much with resolving confusion of the direction of the map. To me, media_foo
and rtx_foo seems clearer.

And "rtx binding" is not terminology from the spec, right?

[danilchap, 2017-08-22 10:18:35]

On 2017/08/22 08:37:39, nisse-webrtc wrote:
> On 2017/08/22 08:09:55, danilchap wrote:
> > 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.
> 
> I see.
>  
> > 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.:
> 
> I prefer map
If you prefer map, use map. it was just a suggestion how to avoid using 'pt' 
in a configuration structure where it is important to be descriptive.


> And "rtx binding" is not terminology from the spec, right?
Quote from spec that I used to come up with the name
"The MIME subtype name used in the binding is "rtx"."

[nisse-webrtc, 2017-08-22 10:57:57]

Description was changed from

==========
Reverse |rtx_payload_types| map, and rename to |media_pt_by_rtx_pt|.

BUG=webrtc:7135
==========

to

==========
Reverse |rtx_payload_types| map, and rename.

New name is |rtx_associated_payload_types|.

BUG=webrtc:7135
==========

[nisse-webrtc, 2017-08-22 10:59:21]

Renamed mapping.

[danilchap, 2017-08-22 12:05:42]

lgtm

[brandtr, 2017-08-22 13:10:22]

lgtm

[nisse-webrtc, 2017-08-22 14:14:27]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-22 14:14:35]

Dry run: 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-08-22 14:43:35]

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

[commit-bot: I haz the power, 2017-08-22 14:43:36]

Dry run: Try jobs failed on following builders:
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/23088)

[nisse-webrtc, 2017-08-22 15:09:04]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-22 15:09:11]

Dry run: 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-08-22 17:03:27]

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

[commit-bot: I haz the power, 2017-08-22 17:03:28]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-08-25 09:31:02]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-08-25 09:31:02]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org,
danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3000273002/#ps100001 (title:
"Rebased.")

[commit-bot: I haz the power, 2017-08-25 09:31:07]

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-08-25 09:35:37]

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

[commit-bot: I haz the power, 2017-08-25 09:35:38]

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

[nisse-webrtc, 2017-08-25 09:39:57]

nisse@webrtc.org changed reviewers:
+ stefan@webrtc.org

[nisse-webrtc, 2017-08-25 09:39:58]

Stefan, PTAL. OWNERs approval needed.

[stefan-webrtc, 2017-08-25 11:28:56]

lgtm

[nisse-webrtc, 2017-08-25 11:29:37]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2017-08-25 11:29:48]

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-08-25 11:44:26]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1503660577876540,
"parent_rev": "87f5c5a4d366da90ca8239b54e25d4d9ec6c1364", "commit_rev":
"26e3abbb40d5fc24642ed61ca226810713084dec"}

[commit-bot: I haz the power, 2017-08-25 11:44:37]

Description was changed from

==========
Reverse |rtx_payload_types| map, and rename.

New name is |rtx_associated_payload_types|.

BUG=webrtc:7135
==========

to

==========
Reverse |rtx_payload_types| map, and rename.

New name is |rtx_associated_payload_types|.

BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/3000273002
Cr-Commit-Position: refs/heads/master@{#19514}
Committed:
https://chromium.googlesource.com/external/webrtc/+/26e3abbb40d5fc24642ed61ca...
==========

[commit-bot: I haz the power, 2017-08-25 11:44:40]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/26e3abbb40d5fc24642ed61ca...