[rtp_rtcp] rtcp::Sdes cleaned and got Parse function

webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc

Index: webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc
index 38c4ef341f78b156f8e1862070fd62d7ec383376..dad54c23f20ec640dd355b3394ff619e6a993959 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc
@@ -10,22 +10,14 @@
 
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.h"
 
+#include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/modules/rtp_rtcp/source/byte_io.h"
 
-using webrtc::RTCPUtility::PT_SDES;
+using webrtc::RTCPUtility::RtcpCommonHeader;
 
 namespace webrtc {
 namespace rtcp {
-namespace {
-void AssignUWord8(uint8_t* buffer, size_t* offset, uint8_t value) {
-  buffer[(*offset)++] = value;
-}
-
-void AssignUWord32(uint8_t* buffer, size_t* offset, uint32_t value) {
-  ByteWriter<uint32_t>::WriteBigEndian(buffer + *offset, value);
-  *offset += 4;
-}
 // Source Description (SDES) (RFC 3550).
 //
 //         0                   1                   2                   3
@@ -51,64 +43,134 @@ void AssignUWord32(uint8_t* buffer, size_t* offset, uint32_t value) {
 //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 //   |    CNAME=1    |     length    | user and domain name        ...
 //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-void CreateSdes(const std::vector<Sdes::Chunk>& chunks,
-                uint8_t* buffer,
-                size_t* pos) {
-  const uint8_t kSdesItemType = 1;
-  for (std::vector<Sdes::Chunk>::const_iterator it = chunks.begin();
-       it != chunks.end(); ++it) {
-    AssignUWord32(buffer, pos, (*it).ssrc);
-    AssignUWord8(buffer, pos, kSdesItemType);
-    AssignUWord8(buffer, pos, (*it).name.length());
-    memcpy(buffer + *pos, (*it).name.data(), (*it).name.length());
-    *pos += (*it).name.length();
-    memset(buffer + *pos, 0, (*it).null_octets);
-    *pos += (*it).null_octets;
-  }
+namespace {
+size_t ChunkSize(const Sdes::Chunk& chunk) {
+  // Chunk:
+  // SSRC/CSRC (4 bytes) | CNAME=1 (1 byte) | length (1 byte) | cname | padding.
+  size_t chunk_payload_size = 4 + 1 + 1 + chunk.cname.size();
+  size_t padding_size = 4 - (chunk_payload_size % 4);  // Minimum 1.
+  return chunk_payload_size + padding_size;
 }
 }  // namespace
 
-bool Sdes::Create(uint8_t* packet,
-                  size_t* index,
-                  size_t max_length,
-                  RtcpPacket::PacketReadyCallback* callback) const {
-  assert(!chunks_.empty());
-  while (*index + BlockLength() > max_length) {
-    if (!OnBufferFull(packet, index, callback))
+Sdes::Sdes() : block_length_(RtcpPacket::kHeaderLength) {}
+
+Sdes::~Sdes() {}
+
+bool Sdes::Parse(const RtcpCommonHeader& header, const uint8_t* payload) {
+  RTC_CHECK(header.packet_type == kPacketType);
+
+  const uint8_t number_of_chunks = header.count_or_format;
+  std::vector<Chunk> chunks;  // Read chunk into temporary array, so that in
+                              // case of an error original array would stay
+                              // unchanged.
+  size_t block_length = kHeaderLength;
+
+  if (header.payload_size_bytes % 4 != 0) {
+    LOG(LS_WARNING) << "Invalid payload size " << header.payload_size_bytes
+                    << " bytes for a valid Sdes packet. Size should be"
+                       " multiple of 4 bytes";
+  }
+  const uint8_t* const payload_end = payload + header.payload_size_bytes;
+  const uint8_t* looking_at = payload;
+  chunks.resize(number_of_chunks);
+  for (size_t i = 0; i < number_of_chunks; ++i) {
+    // Each chunk consumes at least 8 bytes.
+    if (payload_end - looking_at < 8) {
+      LOG(LS_WARNING) << "Not enough space left for chunk #" << (i + 1);
       return false;
+    }
+    chunks[i].ssrc = ByteReader<uint32_t>::ReadBigEndian(looking_at);
+    looking_at += sizeof(uint32_t);
+    bool cname_found = false;
+
+    const uint8_t kTerminateItemType = 0;
+    const uint8_t kCnameItemType = 1;

[åsapersson, 2016/02/11 10:20:32]

use same name here and in Create (kCnameId)

[danilchap, 2016/02/11 11:13:37]

Using now same name here, in Create and in unittests (kCnameTag)

+    uint8_t item_type;
+    while ((item_type = *(looking_at++)) != kTerminateItemType) {
+      if (looking_at >= payload_end) {
+        LOG(LS_WARNING) << "Unexpected end of packet while reading chunk #"
+                        << (i + 1) << ". Expected to find size of the text.";
+        return false;
+      }
+      uint8_t size = *(looking_at++);

[åsapersson, 2016/02/11 10:20:32]

maybe call length as in figure

[danilchap, 2016/02/11 11:13:37]

Done.

+      if (looking_at + size + 1 > payload_end) {

[åsapersson, 2016/02/11 10:20:32]

maybe looking_at + size >= payload_end

[danilchap, 2016/02/11 11:13:37]

+1 has a different meaning here. Explained with a constant.

+        LOG(LS_WARNING) << "Unexpected end of packet while reading chunk #"
+                        << (i + 1) << ". Expected to find text of size "
+                        << size;
+        return false;
+      }
+      if (item_type == kCnameItemType) {
+        if (cname_found) {
+          LOG(LS_WARNING) << "Found extra CNAME for same ssrc in chunk #"
+                          << (i + 1);
+          return false;
+        }
+        cname_found = true;
+        chunks[i].cname.assign(reinterpret_cast<const char*>(looking_at), size);
+      }
+      looking_at += size;
+    }

[åsapersson, 2016/02/11 10:20:32]

is a chuck with zero items valid?

[danilchap, 2016/02/11 11:13:37]

Chunk need to have a cname. chunk with zero items can't.

+    if (!cname_found) {
+      LOG(LS_WARNING) << "CNAME not found for chunk #" << (i + 1);
+      return false;
+    }
+    block_length += ChunkSize(chunks[i]);

[åsapersson, 2016/02/11 10:20:33]

is block_length supposed to not include non-cname items?

[danilchap, 2016/02/11 11:13:37]

block_length is for packing Sdes back to byte array. Because non-cname items are
not stored and will not be included in rebuild packet, their size implicitly
ignored. If chunk would include another field that we would send, ChunkSize
would count its size.

[åsapersson, 2016/02/11 12:28:45]

Maybe add a comment.

[danilchap, 2016/02/11 12:48:47]

Done.

+    // Adjust to 32bit boundary.
+    looking_at += (payload_end - looking_at) % 4;
   }
-  CreateHeader(chunks_.size(), PT_SDES, HeaderLength(), packet, index);
-  CreateSdes(chunks_, packet, index);
+
+  chunks_ = std::move(chunks);
+  block_length_ = block_length;
   return true;
 }
 
 bool Sdes::WithCName(uint32_t ssrc, const std::string& cname) {
-  assert(cname.length() <= 0xff);
+  RTC_DCHECK_LE(cname.length(), 0xffu);
   if (chunks_.size() >= kMaxNumberOfChunks) {
     LOG(LS_WARNING) << "Max SDES chunks reached.";
     return false;
   }
-  // In each chunk, the list of items must be terminated by one or more null
-  // octets. The next chunk must start on a 32-bit boundary.
-  // CNAME (1 byte) | length (1 byte) | name | padding.
-  int null_octets = 4 - ((2 + cname.length()) % 4);
   Chunk chunk;
   chunk.ssrc = ssrc;
-  chunk.name = cname;
-  chunk.null_octets = null_octets;
+  chunk.cname = cname;
   chunks_.push_back(chunk);
+  block_length_ += ChunkSize(chunk);
   return true;
 }
 
-size_t Sdes::BlockLength() const {
-  // Header (4 bytes).
-  // Chunk:
-  // SSRC/CSRC (4 bytes) | CNAME (1 byte) | length (1 byte) | name | padding.
-  size_t length = kHeaderLength;
-  for (const Chunk& chunk : chunks_)
-    length += 6 + chunk.name.length() + chunk.null_octets;
-  assert(length % 4 == 0);
-  return length;
+bool Sdes::Create(uint8_t* packet,
+                  size_t* index,
+                  size_t max_length,
+                  RtcpPacket::PacketReadyCallback* callback) const {
+  RTC_DCHECK(!chunks_.empty());

[åsapersson, 2016/02/11 10:20:32]

zero is valid right? maybe remove

[danilchap, 2016/02/11 11:13:37]

Done.

+  while (*index + BlockLength() > max_length) {
+    if (!OnBufferFull(packet, index, callback))
+      return false;
+  }
+  const size_t index_end = *index + BlockLength();
+  CreateHeader(chunks_.size(), kPacketType, HeaderLength(), packet, index);
+
+  const uint8_t kCnameId = 1;
+  for (const Sdes::Chunk& chunk : chunks_) {
+    ByteWriter<uint32_t>::WriteBigEndian(&packet[*index + 0], chunk.ssrc);
+    ByteWriter<uint8_t>::WriteBigEndian(&packet[*index + 4], kCnameId);
+    ByteWriter<uint8_t>::WriteBigEndian(&packet[*index + 5],
+                                        chunk.cname.size());
+    memcpy(&packet[*index + 6], chunk.cname.data(), chunk.cname.size());
+    *index += (6 + chunk.cname.size());
+
+    // In each chunk, the list of items must be terminated by one or more null
+    // octets. The next chunk must start on a 32-bit boundary.
+    // CNAME (1 byte) | length (1 byte) | name | padding.
+    size_t padding_size = 4 - ((6 + chunk.cname.size()) % 4);
+    const int kPadding = 0;
+    memset(packet + *index, kPadding, padding_size);
+    *index += padding_size;
+  }
+
+  RTC_CHECK_EQ(*index, index_end);
+  return true;
 }
 }  // namespace rtcp
 }  // namespace webrtc