webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc - Issue 2524923002: Remove RTPPayloadStrategy and simplify RTPPayloadRegistry

Unified Diff: webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc

Issue 2524923002: Remove RTPPayloadStrategy and simplify RTPPayloadRegistry (Closed)

Patch Set: Rebase Created 4 years, 1 month ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc ('K') | « webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc ('k') | webrtc/modules/rtp_rtcp/source/rtp_utility.h » ('j') | webrtc/modules/rtp_rtcp/source/rtp_utility.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc

diff --git a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc

index 6a7215fd924d9f463e6110014f4441171bbfdc01..d5dcd9b8940f7935d0cd4a0d05aa9ccb0598189f 100644

--- a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc

+++ b/webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc

@@ -14,7 +14,6 @@

#include "webrtc/modules/rtp_rtcp/include/rtp_header_parser.h"

#include "webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h"

#include "webrtc/modules/rtp_rtcp/source/byte_io.h"

-#include "webrtc/modules/rtp_rtcp/source/mock/mock_rtp_payload_strategy.h"

#include "webrtc/modules/rtp_rtcp/source/rtp_utility.h"

#include "webrtc/test/gmock.h"

#include "webrtc/test/gtest.h"

@@ -28,8 +27,8 @@ using ::testing::_;

static const char* kTypicalPayloadName = "name";

static const size_t kTypicalChannels = 1;

-static const int kTypicalFrequency = 44000;

-static const int kTypicalRate = 32 * 1024;

+static const uint32_t kTypicalFrequency = 44000;

danilchap 2016/11/23 19:06:18 what is the reason for using fixed size integer ty

magjed_webrtc 2016/11/24 12:20:39 I had to change them in order to avoid compiler wa

+static const uint32_t kTypicalRate = 32 * 1024;

static const CodecInst kTypicalAudioCodec = {-1 /* pltype */, "name",

kTypicalFrequency, 0 /* pacsize */,

kTypicalChannels, kTypicalRate};

@@ -38,50 +37,16 @@ class RtpPayloadRegistryTest : public ::testing::Test {

public:

void SetUp() {

// Note: the payload registry takes ownership of the strategy.

- mock_payload_strategy_ = new testing::NiceMock<MockRTPPayloadStrategy>();

- rtp_payload_registry_.reset(new RTPPayloadRegistry(mock_payload_strategy_));

+ rtp_payload_registry_.reset(new RTPPayloadRegistry());

}

protected:

- RtpUtility::Payload* ExpectReturnOfTypicalAudioPayload(uint8_t payload_type,

- uint32_t rate) {

- bool audio = true;

- RtpUtility::Payload returned_payload = {

- "name",

- audio,

- {// Initialize the audio struct in this case.

- {kTypicalFrequency, kTypicalChannels, rate}}};

- // Note: we return a new payload since the payload registry takes ownership

- // of the created object.

- RtpUtility::Payload* returned_payload_on_heap =

- new RtpUtility::Payload(returned_payload);

- EXPECT_CALL(*mock_payload_strategy_,

- CreatePayloadType(StrEq(kTypicalPayloadName), payload_type,

- kTypicalFrequency, kTypicalChannels, rate))

- .WillOnce(Return(returned_payload_on_heap));

- return returned_payload_on_heap;

- }

std::unique_ptr<RTPPayloadRegistry> rtp_payload_registry_;

- testing::NiceMock<MockRTPPayloadStrategy>* mock_payload_strategy_;

};

TEST_F(RtpPayloadRegistryTest,

RegistersAndRemembersVideoPayloadsUntilDeregistered) {

const uint8_t payload_type = 97;

- RtpUtility::Payload returned_video_payload = {

- "VP8", false /* audio */, {{kRtpVideoVp8}}};

- // Note: The payload registry takes ownership of this object in

- // RegisterReceivePayload.

- RtpUtility::Payload* returned_video_payload_on_heap =

- new RtpUtility::Payload(returned_video_payload);

- EXPECT_CALL(

- *mock_payload_strategy_,

- CreatePayloadType(StrEq("VP8"), payload_type, kVideoPayloadTypeFrequency,

- 0 /* channels */, 0 /* rate */))

- .WillOnce(Return(returned_video_payload_on_heap));

VideoCodec video_codec;

video_codec.codecType = kVideoCodecVP8;

strcpy(video_codec.plName, "VP8");

@@ -93,9 +58,10 @@ TEST_F(RtpPayloadRegistryTest,

rtp_payload_registry_->PayloadTypeToPayload(payload_type);

EXPECT_TRUE(retrieved_payload);

- // We should get back the exact pointer to the payload returned by the

- // payload strategy.

- EXPECT_EQ(returned_video_payload_on_heap, retrieved_payload);

+ // We should get back the corresponding payload that we registered.

+ EXPECT_STREQ("VP8", retrieved_payload->name);

+ EXPECT_EQ(false, retrieved_payload->audio);

danilchap 2016/11/23 19:06:18 EXPECT_FALSE(retrieved_payload->audio);

magjed_webrtc 2016/11/24 12:20:39 Done.

+ EXPECT_EQ(kRtpVideoVp8, retrieved_payload->typeSpecific.Video.videoCodecType);

// Now forget about it and verify it's gone.

EXPECT_EQ(0, rtp_payload_registry_->DeRegisterReceivePayload(payload_type));

@@ -105,9 +71,6 @@ TEST_F(RtpPayloadRegistryTest,

TEST_F(RtpPayloadRegistryTest,

RegistersAndRemembersAudioPayloadsUntilDeregistered) {

uint8_t payload_type = 97;

- RtpUtility::Payload* returned_payload_on_heap =

- ExpectReturnOfTypicalAudioPayload(payload_type, kTypicalRate);

bool new_payload_created = false;

CodecInst audio_codec = kTypicalAudioCodec;

audio_codec.pltype = payload_type;

@@ -120,9 +83,12 @@ TEST_F(RtpPayloadRegistryTest,

rtp_payload_registry_->PayloadTypeToPayload(payload_type);

EXPECT_TRUE(retrieved_payload);

- // We should get back the exact pointer to the payload returned by the

- // payload strategy.

- EXPECT_EQ(returned_payload_on_heap, retrieved_payload);

+ // We should get back the corresponding payload that we registered.

+ EXPECT_STREQ(kTypicalPayloadName, retrieved_payload->name);

+ EXPECT_EQ(true, retrieved_payload->audio);

danilchap 2016/11/23 19:06:18 EXPECT_TRUE(retrieved_payload->audio)

magjed_webrtc 2016/11/24 12:20:39 Done.

+ EXPECT_EQ(kTypicalFrequency, retrieved_payload->typeSpecific.Audio.frequency);

+ EXPECT_EQ(kTypicalChannels, retrieved_payload->typeSpecific.Audio.channels);

+ EXPECT_EQ(kTypicalRate, retrieved_payload->typeSpecific.Audio.rate);

// Now forget about it and verify it's gone.

EXPECT_EQ(0, rtp_payload_registry_->DeRegisterReceivePayload(payload_type));

@@ -136,8 +102,7 @@ TEST_F(RtpPayloadRegistryTest, AudioRedWorkProperly) {

const int kRedBitRate = 0;

// This creates an audio RTP payload strategy.

- rtp_payload_registry_.reset(

- new RTPPayloadRegistry(RTPPayloadStrategy::CreateStrategy(true)));

+ rtp_payload_registry_.reset(new RTPPayloadRegistry());

danilchap 2016/11/23 19:06:18 without strategies look like the line can be remov

magjed_webrtc 2016/11/24 12:20:39 Done.

bool new_payload_created = false;

CodecInst red_audio_codec;

@@ -168,20 +133,21 @@ TEST_F(RtpPayloadRegistryTest,

uint8_t payload_type = 97;

bool ignored = false;

- RtpUtility::Payload* first_payload_on_heap =

- ExpectReturnOfTypicalAudioPayload(payload_type, kTypicalRate);

CodecInst audio_codec = kTypicalAudioCodec;

audio_codec.pltype = payload_type;

EXPECT_EQ(

0, rtp_payload_registry_->RegisterReceivePayload(audio_codec, &ignored));

- EXPECT_EQ(

- -1, rtp_payload_registry_->RegisterReceivePayload(audio_codec, &ignored))

- << "Adding same codec twice = bad.";

- RtpUtility::Payload* second_payload_on_heap =

- ExpectReturnOfTypicalAudioPayload(payload_type - 1, kTypicalRate);

CodecInst audio_codec_2 = kTypicalAudioCodec;

+ audio_codec_2.pltype = payload_type;

+ // Make |audio_codec_2| incompatible with |audio_codec| by changing

+ // the frequency.

+ audio_codec_2.plfreq = kTypicalFrequency + 1;

+ EXPECT_EQ(-1, rtp_payload_registry_->RegisterReceivePayload(audio_codec_2,

+ &ignored))

+ << "Adding incompatible codec with same payload type = bad.";

+ // Change payload type.

audio_codec_2.pltype = payload_type - 1;

EXPECT_EQ(

0, rtp_payload_registry_->RegisterReceivePayload(audio_codec_2, &ignored))

@@ -191,39 +157,38 @@ TEST_F(RtpPayloadRegistryTest,

const RtpUtility::Payload* retrieved_payload =

rtp_payload_registry_->PayloadTypeToPayload(payload_type);

EXPECT_TRUE(retrieved_payload);

- EXPECT_EQ(first_payload_on_heap, retrieved_payload);

+ EXPECT_STREQ(kTypicalPayloadName, retrieved_payload->name);

+ EXPECT_EQ(true, retrieved_payload->audio);

+ EXPECT_EQ(kTypicalFrequency, retrieved_payload->typeSpecific.Audio.frequency);

+ EXPECT_EQ(kTypicalChannels, retrieved_payload->typeSpecific.Audio.channels);

+ EXPECT_EQ(kTypicalRate, retrieved_payload->typeSpecific.Audio.rate);

retrieved_payload =

rtp_payload_registry_->PayloadTypeToPayload(payload_type - 1);

EXPECT_TRUE(retrieved_payload);

- EXPECT_EQ(second_payload_on_heap, retrieved_payload);

+ EXPECT_STREQ(kTypicalPayloadName, retrieved_payload->name);

+ EXPECT_EQ(true, retrieved_payload->audio);

+ EXPECT_EQ(kTypicalFrequency + 1,

+ retrieved_payload->typeSpecific.Audio.frequency);

+ EXPECT_EQ(kTypicalChannels, retrieved_payload->typeSpecific.Audio.channels);

+ EXPECT_EQ(kTypicalRate, retrieved_payload->typeSpecific.Audio.rate);

// Ok, update the rate for one of the codecs. If either the incoming rate or

// the stored rate is zero it's not really an error to register the same

// codec twice, and in that case roughly the following happens.

- ON_CALL(*mock_payload_strategy_, PayloadIsCompatible(_, _, _, _))

- .WillByDefault(Return(true));

- EXPECT_CALL(*mock_payload_strategy_,

- UpdatePayloadRate(first_payload_on_heap, kTypicalRate));

EXPECT_EQ(

0, rtp_payload_registry_->RegisterReceivePayload(audio_codec, &ignored));

}

TEST_F(RtpPayloadRegistryTest,

RemovesCompatibleCodecsOnRegistryIfCodecsMustBeUnique) {

- ON_CALL(*mock_payload_strategy_, PayloadIsCompatible(_, _, _, _))

- .WillByDefault(Return(true));

- ON_CALL(*mock_payload_strategy_, CodecsMustBeUnique())

- .WillByDefault(Return(true));

uint8_t payload_type = 97;

bool ignored = false;

- ExpectReturnOfTypicalAudioPayload(payload_type, kTypicalRate);

CodecInst audio_codec = kTypicalAudioCodec;

audio_codec.pltype = payload_type;

EXPECT_EQ(

0, rtp_payload_registry_->RegisterReceivePayload(audio_codec, &ignored));

- ExpectReturnOfTypicalAudioPayload(payload_type - 1, kTypicalRate);

CodecInst audio_codec_2 = kTypicalAudioCodec;

audio_codec_2.pltype = payload_type - 1;

EXPECT_EQ(0, rtp_payload_registry_->RegisterReceivePayload(audio_codec_2,

@@ -235,12 +200,11 @@ TEST_F(RtpPayloadRegistryTest,

EXPECT_TRUE(rtp_payload_registry_->PayloadTypeToPayload(payload_type - 1))

<< "The second payload should still be registered though.";

- // Now ensure non-compatible codecs aren't removed.

- ON_CALL(*mock_payload_strategy_, PayloadIsCompatible(_, _, _, _))

- .WillByDefault(Return(false));

- ExpectReturnOfTypicalAudioPayload(payload_type + 1, kTypicalRate);

+ // Now ensure non-compatible codecs aren't removed. Make |audio_codec_3|

+ // incompatible by changing the frequency.

CodecInst audio_codec_3 = kTypicalAudioCodec;

audio_codec_3.pltype = payload_type + 1;

+ audio_codec_3.plfreq = kTypicalFrequency + 1;

EXPECT_EQ(0, rtp_payload_registry_->RegisterReceivePayload(audio_codec_3,

&ignored));

@@ -261,7 +225,6 @@ TEST_F(RtpPayloadRegistryTest,

EXPECT_TRUE(media_type_unchanged);

bool ignored;

- ExpectReturnOfTypicalAudioPayload(34, kTypicalRate);

CodecInst audio_codec = kTypicalAudioCodec;

audio_codec.pltype = 34;

EXPECT_EQ(

@@ -284,7 +247,7 @@ TEST_P(ParameterizedRtpPayloadRegistryTest,

CodecInst audio_codec;

strcpy(audio_codec.plname, "whatever");

audio_codec.pltype = static_cast<uint8_t>(payload_type);

- audio_codec.plfreq = 19;

+ audio_codec.plfreq = 1900;

audio_codec.channels = 1;

audio_codec.rate = 17;

EXPECT_EQ(

@@ -306,7 +269,7 @@ TEST_P(RtpPayloadRegistryGenericTest, RegisterGenericReceivePayloadType) {

// Dummy values, except for payload_type.

strcpy(audio_codec.plname, "generic-codec");

audio_codec.pltype = payload_type;

- audio_codec.plfreq = 19;

+ audio_codec.plfreq = 1900;

audio_codec.channels = 1;

audio_codec.rate = 17;

EXPECT_EQ(