generic-library/webrtc
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Removes VoERTP_RTCP::InsertExtraRTPPacket.

Browse Source 
Reasons for removing:

- Feels like a complete hack IMHO.
- Not used by any client.
- Unclear functionality regarding time stamp, marker bit etc.
- Causes several issues in tests due to a bad design which mainly depends on the fact that this API "breaks" an ongoing data/packet flow and it complicates the threading model and creates risks for deadlock and memory corruption. Not worth trying to fix given the very unclear benefit of maintaining the API. Better to remove the API instead.
- We also see lots of TSan races and memcheck errors related to this API.

BUG=2296,2240
R=mflodman@webrtc.org, niklas.enbom@webrtc.org, xians@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/8819004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@5574 4adac7df-926f-26a2-2b94-8c16560cd09d
This commit is contained in:
henrika@webrtc.org 2014-02-19 08:58:08 +00:00
parent e384104166
commit b7a91fa95a
6 changed files with 7 additions and 173 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

90
webrtc/voice_engine/channel.cc
View File

@ -179,21 +179,6 @@ Channel::SendPacket(int channel, const void *data, int len)
return -1;
}
// Insert extra RTP packet using if user has called the InsertExtraRTPPacket
// API
if (_insertExtraRTPPacket)
{
uint8_t* rtpHdr = (uint8_t*)data;
uint8_t M_PT(0);
if (_extraMarkerBit)
{
M_PT = 0x80; // set the M-bit
}
M_PT += _extraPayloadType; // set the payload type
*(++rtpHdr) = M_PT; // modify the M|PT-byte within the RTP header
_insertExtraRTPPacket = false; // insert one packet only
}
uint8_t* bufferToSendPtr = (uint8_t*)data;
int32_t bufferLength = len;
@ -922,9 +907,6 @@ Channel::Channel(int32_t channelId,
_outputGain(1.0f),
_playOutbandDtmfEvent(false),
_playInbandDtmfEvent(false),
_extraPayloadType(0),
_insertExtraRTPPacket(false),
_extraMarkerBit(false),
_lastLocalTimeStamp(0),
_lastRemoteTimeStamp(0),
_lastPayloadType(0),
@ -4141,78 +4123,6 @@ Channel::RTPDumpIsActive(RTPDirections direction)
return rtpDumpPtr->IsActive();
}
int
Channel::InsertExtraRTPPacket(unsigned char payloadType,
bool markerBit,
const char* payloadData,
unsigned short payloadSize)
{
WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId, _channelId),
"Channel::InsertExtraRTPPacket()");
if (payloadType > 127)
{
_engineStatisticsPtr->SetLastError(
VE_INVALID_PLTYPE, kTraceError,
"InsertExtraRTPPacket() invalid payload type");
return -1;
}
if (payloadData == NULL)
{
_engineStatisticsPtr->SetLastError(
VE_INVALID_ARGUMENT, kTraceError,
"InsertExtraRTPPacket() invalid payload data");
return -1;
}
if (payloadSize > _rtpRtcpModule->MaxDataPayloadLength())
{
_engineStatisticsPtr->SetLastError(
VE_INVALID_ARGUMENT, kTraceError,
"InsertExtraRTPPacket() invalid payload size");
return -1;
}
if (!_sending)
{
_engineStatisticsPtr->SetLastError(
VE_NOT_SENDING, kTraceError,
"InsertExtraRTPPacket() not sending");
return -1;
}
// Create extra RTP packet by calling RtpRtcp::SendOutgoingData().
// Transport::SendPacket() will be called by the module when the RTP packet
// is created.
// The call to SendOutgoingData() does *not* modify the timestamp and
// payloadtype to ensure that the RTP module generates a valid RTP packet
// (user might utilize a non-registered payload type).
// The marker bit and payload type will be replaced just before the actual
// transmission, i.e., the actual modification is done *after* the RTP
// module has delivered its RTP packet back to the VoE.
// We will use the stored values above when the packet is modified
// (see Channel::SendPacket()).
_extraPayloadType = payloadType;
_extraMarkerBit = markerBit;
_insertExtraRTPPacket = true;
if (_rtpRtcpModule->SendOutgoingData(kAudioFrameSpeech,
_lastPayloadType,
_lastLocalTimeStamp,
// Leaving the time when this frame was
// received from the capture device as
// undefined for voice for now.
-1,
(const uint8_t*) payloadData,
payloadSize) != 0)
{
_engineStatisticsPtr->SetLastError(
VE_RTP_RTCP_MODULE_ERROR, kTraceError,
"InsertExtraRTPPacket() failed to send extra RTP packet");
return -1;
}
return 0;
}
uint32_t
Channel::Demultiplex(const AudioFrame& audioFrame)
{

6
webrtc/voice_engine/channel.h
View File

@ -279,9 +279,6 @@ public:
int StartRTPDump(const char fileNameUTF8[1024], RTPDirections direction);
int StopRTPDump(RTPDirections direction);
bool RTPDumpIsActive(RTPDirections direction);
int InsertExtraRTPPacket(unsigned char payloadType, bool markerBit,
const char* payloadData,
unsigned short payloadSize);
uint32_t LastRemoteTimeStamp() { return _lastRemoteTimeStamp; }
// From AudioPacketizationCallback in the ACM
@ -528,9 +525,6 @@ private:
bool _playOutbandDtmfEvent;
bool _playInbandDtmfEvent;
// VoeRTP_RTCP
uint8_t _extraPayloadType;
bool _insertExtraRTPPacket;
bool _extraMarkerBit;
uint32_t _lastLocalTimeStamp;
uint32_t _lastRemoteTimeStamp;
int8_t _lastPayloadType;

14
webrtc/voice_engine/include/voe_rtp_rtcp.h
View File

@ -18,7 +18,6 @@
// - Forward Error Correction (FEC).
// - Writing RTP and RTCP packets to binary files for off-line analysis of
// the call quality.
// - Inserting extra RTP packets into active audio stream.
//
// Usage example, omitting error checking:
//
@ -246,17 +245,16 @@ public:
virtual int RTPDumpIsActive(
int channel, RTPDirections direction = kRtpIncoming) = 0;
// Sends an extra RTP packet using an existing/active RTP session.
// It is possible to set the payload type, marker bit and payload
// of the extra RTP
virtual int InsertExtraRTPPacket(
int channel, unsigned char payloadType, bool markerBit,
const char* payloadData, unsigned short payloadSize) = 0;
// Gets the timestamp of the last RTP packet received by |channel|.
virtual int GetLastRemoteTimeStamp(int channel,
uint32_t* lastRemoteTimeStamp) = 0;
// Don't use. To be removed.
virtual int InsertExtraRTPPacket(
int channel, unsigned char payloadType, bool markerBit,
const char* payloadData, unsigned short payloadSize) { return -1; };
protected:
VoERTP_RTCP() {}
virtual ~VoERTP_RTCP() {}

32
webrtc/voice_engine/test/auto_test/standard/rtp_rtcp_test.cc
View File

@ -201,38 +201,6 @@ TEST_F(RtpRtcpTest, DisabledRtcpObserverDoesNotReceiveData) {
EXPECT_EQ(0u, rtcp_app_handler.sub_type_);
}
TEST_F(RtpRtcpTest, InsertExtraRTPPacketDealsWithInvalidArguments) {
const char payload_data[8] = { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H' };
EXPECT_EQ(-1, voe_rtp_rtcp_->InsertExtraRTPPacket(
-1, 0, false, payload_data, 8)) <<
"Should reject: invalid channel.";
EXPECT_EQ(-1, voe_rtp_rtcp_->InsertExtraRTPPacket(
channel_, -1, false, payload_data, 8)) <<
"Should reject: invalid payload type.";
EXPECT_EQ(-1, voe_rtp_rtcp_->InsertExtraRTPPacket(
channel_, 128, false, payload_data, 8)) <<
"Should reject: invalid payload type.";
EXPECT_EQ(-1, voe_rtp_rtcp_->InsertExtraRTPPacket(
channel_, 99, false, NULL, 8)) <<
"Should reject: bad pointer.";
EXPECT_EQ(-1, voe_rtp_rtcp_->InsertExtraRTPPacket(
channel_, 99, false, payload_data, 1500 - 28 + 1)) <<
"Should reject: invalid size.";
}
TEST_F(RtpRtcpTest, DISABLED_CanTransmitExtraRtpPacketsWithoutError) {
const char payload_data[8] = { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H' };
for (int i = 0; i < 128; ++i) {
// Try both with and without the marker bit set
EXPECT_EQ(0, voe_rtp_rtcp_->InsertExtraRTPPacket(
channel_, i, false, payload_data, 8));
EXPECT_EQ(0, voe_rtp_rtcp_->InsertExtraRTPPacket(
channel_, i, true, payload_data, 8));
}
}
// TODO(xians, phoglund): Re-enable when issue 372 is resolved.
TEST_F(RtpRtcpTest, DISABLED_CanCreateRtpDumpFilesWithoutError) {
// Create two RTP dump files (3 seconds long). You can verify these after

30
webrtc/voice_engine/voe_rtp_rtcp_impl.cc
View File

@ -655,36 +655,6 @@ int VoERTP_RTCPImpl::RTPDumpIsActive(int channel, RTPDirections direction)
return channelPtr->RTPDumpIsActive(direction);
}
int VoERTP_RTCPImpl::InsertExtraRTPPacket(int channel,
unsigned char payloadType,
bool markerBit,
const char* payloadData,
unsigned short payloadSize)
{
WEBRTC_TRACE(kTraceApiCall, kTraceVoice, VoEId(_shared->instance_id(), -1),
"InsertExtraRTPPacket(channel=%d, payloadType=%u,"
" markerBit=%u, payloadSize=%u)",
channel, payloadType, markerBit, payloadSize);
if (!_shared->statistics().Initialized())
{
_shared->SetLastError(VE_NOT_INITED, kTraceError);
return -1;
}
voe::ChannelOwner ch = _shared->channel_manager().GetChannel(channel);
voe::Channel* channelPtr = ch.channel();
if (channelPtr == NULL)
{
_shared->SetLastError(VE_CHANNEL_NOT_VALID, kTraceError,
"InsertExtraRTPPacket() failed to locate channel");
return -1;
}
return channelPtr->InsertExtraRTPPacket(payloadType,
markerBit,
payloadData,
payloadSize);
}
int VoERTP_RTCPImpl::GetLastRemoteTimeStamp(int channel,
uint32_t* timestamp) {
WEBRTC_TRACE(kTraceApiCall, kTraceVoice, VoEId(_shared->instance_id(), -1),

8
webrtc/voice_engine/voe_rtp_rtcp_impl.h
View File

@ -71,7 +71,7 @@ public:
bool& enabled,
unsigned char& ID);
// CSRC
// CSRC
virtual int GetRemoteCSRCs(int channel, unsigned int arrCSRC[15]);
// Statistics
@ -110,12 +110,6 @@ public:
virtual int RTPDumpIsActive(int channel,
RTPDirections direction = kRtpIncoming);
// Insert (and transmits) extra RTP packet into active RTP audio stream
virtual int InsertExtraRTPPacket(int channel,
unsigned char payloadType,
bool markerBit,
const char* payloadData,
unsigned short payloadSize);
virtual int GetLastRemoteTimeStamp(int channel,
uint32_t* lastRemoteTimeStamp);
protected: