Fix video_replay tool to respect RTX stream and fix default parameters.

webrtc/test/call_test.cc

 /*
  *  Copyright (c) 2014 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 429 matching lines @@
 const std::map<uint8_t, MediaType> CallTest::payload_type_map_ = {
     {CallTest::kVideoSendPayloadType, MediaType::VIDEO},
     {CallTest::kFakeVideoSendPayloadType, MediaType::VIDEO},
     {CallTest::kSendRtxPayloadType, MediaType::VIDEO},
     {CallTest::kRedPayloadType, MediaType::VIDEO},
     {CallTest::kRtxRedPayloadType, MediaType::VIDEO},
     {CallTest::kUlpfecPayloadType, MediaType::VIDEO},
     {CallTest::kFlexfecPayloadType, MediaType::VIDEO},
     {CallTest::kAudioSendPayloadType, MediaType::AUDIO}};
 
+const int CallTest::kPayloadTypeH264 = 122;

[philipel, 2017/07/10 17:00:50]

Codec payload types are dynamically negotiated, right?

[ilnik, 2017/07/10 17:06:19]

Not in the Call test. I think. I am certain, that full stack tests use these
hardcoded values.

[sprang_webrtc, 2017/07/10 17:29:39]

Exactly, these are the constants used in tests, for ease of use. In prod they
are dynamically allocated (so in practice probably just hardcoded on some higher
abstraction layer).

Anyway, these are only defaults in the replay tool, rather than being like
before set to 0 (which is disallowed).

+const int CallTest::kPayloadTypeVP8 = 123;
+const int CallTest::kPayloadTypeVP9 = 124;

[tommi, 2017/07/11 08:34:47]

out of curiosity - should these be enum values? alternatively, should they be
int32 if a particular size is needed?

[ilnik, 2017/07/11 08:43:43]

Actually, type really should be uint8_t. As for enum, I feel it's better like
that, similarly to all the constants at lines 422-429.

[tommi, 2017/07/11 10:14:45]

Assuming they're all mutually exclusive, it would make sense to me to group them
all together in a single enum like so:

enum class PayloadType : uint8_t {
  ...
  H264 = 122,
  VP8 = 123,
  VP9 = 124,
  ...
};

[ilnik, 2017/07/11 10:20:59]

In fact, most of them are not mutually exclusive: E.g. kFlexfecPayloadType,
kSendRtxPayloadType and kVideoSendPayloadType may be used in the same send
config at the same time. These 3 moved are exclusive, but it will look worse if
half of these constants would be in enums, and half would be plain constants.

[tommi, 2017/07/11 11:41:36]

Oh I was thinking mutually exclusive values. Not usage. Seems to me that
grouping payload types into an enum would be a good thing. Also making sure
their value is limited to the underlying primitive type (8 bits instead of 32 or
64), would be good.

[ilnik, 2017/07/11 12:20:30]

Makes sense. I am making a tracking bug for that refactoring. Will do it when I
have time.

+
 BaseTest::BaseTest() : event_log_(RtcEventLog::CreateNull()) {}
 
 BaseTest::BaseTest(unsigned int timeout_ms)
     : RtpRtcpObserver(timeout_ms), event_log_(RtcEventLog::CreateNull()) {}
 
 BaseTest::~BaseTest() {
 }
 
 std::unique_ptr<FakeAudioDevice::Capturer> BaseTest::CreateCapturer() {
   return FakeAudioDevice::CreatePulsedNoiseCapturer(256, 48000);
@@ skipping 87 matching lines @@
 
 EndToEndTest::EndToEndTest(unsigned int timeout_ms) : BaseTest(timeout_ms) {
 }
 
 bool EndToEndTest::ShouldCreateReceivers() const {
   return true;
 }
 
 }  // namespace test
 }  // namespace webrtc