Define remote playback proto buffer

media/remoting/rpc/decoder_buffer_segment.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/remoting/rpc/decoder_buffer_segment.h"
+
+#include <algorithm>
+#include <memory>
+#include <utility>
+#include <vector>
+
+#include "base/logging.h"
+#include "base/values.h"
+#include "media/base/decrypt_config.h"
+
+namespace media {
+namespace remoting {
+
+namespace {
+
+const int kPayloadVersionFieldSize = 1;
+const int kProtoBufferHeaderSize = 2;
+const int kDataBufferHeaderSize = 4;
+
+std::unique_ptr<::media::DecryptConfig> DeserializeDecryptConfig(
+    const pb::DecryptConfig& config_message) {
+  std::vector<::media::SubsampleEntry> entries;
+
+  DCHECK(config_message.has_key_id());

[miu, 2016/09/08 01:07:34]

These DCHECKs suggest the fields should not have been optional in the proto.

[erickung1, 2016/09/13 03:52:52]

After the discussion, the field in proto buffer will remain optional.

In this case, this is the util function to deserialize decryptdonfig from proto
back to ::media::DecryptoConfig if the incoming frame is encrypted.
Therefore, the utility function is called with such assumption, and we use
DCHECK to verify the implementation sticks on the theory.

[miu, 2016/09/13 05:40:57]

Should we keep these checks? If they are needed, and triggered, we should return
nullptr instead of crashing the process; since we are parsing "external
untrusted data."

[erickung1, 2016/09/15 02:13:32]

Done.

+  DCHECK(config_message.has_iv());
+
+  for (int i = 0; i < config_message.sub_samples_size(); ++i) {
+    entries.push_back(
+        ::media::SubsampleEntry(config_message.sub_samples(i).clear_bytes(),
+                                config_message.sub_samples(i).cypher_bytes()));
+  }
+
+  std::unique_ptr<::media::DecryptConfig> decrypt_config(
+      new ::media::DecryptConfig(config_message.key_id(), config_message.iv(),
+                                 entries));
+  return decrypt_config;
+}
+
+scoped_refptr<::media::DecoderBuffer> DeserializeDecoderBuffer(
+    const pb::DecoderBuffer& buffer_message,
+    scoped_refptr<::media::DecoderBuffer> buffer) {
+  std::unique_ptr<::media::DecryptConfig> decrypt_config;
+  base::TimeDelta front_discard;
+  base::TimeDelta back_discard;
+  bool has_discard = true;
+
+  if (buffer_message.is_eos()) {
+    VLOG(1) << "EOS data";
+    return ::media::DecoderBuffer::CreateEOSBuffer();
+  }
+
+  if (buffer_message.has_timestamp_usec()) {
+    buffer->set_timestamp(
+        base::TimeDelta::FromMicroseconds(buffer_message.timestamp_usec()));
+  }
+
+  if (buffer_message.has_duration_usec()) {
+    buffer->set_duration(
+        base::TimeDelta::FromMicroseconds(buffer_message.duration_usec()));
+  }
+  VLOG(2) << "timestamp:" << buffer_message.timestamp_usec()
+          << " duration:" << buffer_message.duration_usec();
+
+  if (buffer_message.has_is_key_frame())
+    buffer->set_is_key_frame(buffer_message.is_key_frame());
+
+  if (buffer_message.has_decrypt_config()) {
+    const pb::DecryptConfig config_message = buffer_message.decrypt_config();
+    decrypt_config = DeserializeDecryptConfig(config_message);
+    buffer->set_decrypt_config(std::move(decrypt_config));

[miu, 2016/09/08 01:07:34]

You can simplify here and eliminate the local variables:

  buffer->set_decrypt_config(
      DeserializeDecryptConfig(buffer_message.decrypt_config());

[erickung1, 2016/09/13 03:52:53]

Done.

+  }
+
+  if (buffer_message.has_front_discard_usec()) {
+    has_discard = true;
+    front_discard =
+        base::TimeDelta::FromMicroseconds(buffer_message.front_discard_usec());
+  }
+  if (buffer_message.has_back_discard_usec()) {
+    has_discard = true;
+    back_discard =
+        base::TimeDelta::FromMicroseconds(buffer_message.back_discard_usec());
+  }
+
+  if (has_discard) {
+    buffer->set_discard_padding(
+        ::media::DecoderBuffer::DiscardPadding(front_discard, back_discard));
+  }
+
+  if (buffer_message.has_splice_timestamp_usec()) {
+    buffer->set_splice_timestamp(base::TimeDelta::FromMicroseconds(
+        buffer_message.splice_timestamp_usec()));
+  }
+
+  if (buffer_message.has_side_data()) {
+    buffer->CopySideDataFrom(
+        reinterpret_cast<const uint8_t*>(buffer_message.side_data().c_str()),

[miu, 2016/09/08 01:07:34]

Don't use c_str() here. Use data() instead.

[erickung1, 2016/09/13 03:52:52]

Done.

+        buffer_message.side_data().size());
+  }
+
+  return buffer;
+}
+
+void SerializeDecryptConfig(const ::media::DecryptConfig* decrypt_config,

[miu, 2016/09/08 01:07:34]

nit: Please pass by const-reference instead of const-pointer, since
|decrypt_config| must never be null.

[erickung1, 2016/09/13 03:52:53]

Done.

+                            pb::DecryptConfig* config_message) {
+  DCHECK(config_message);
+
+  config_message->set_key_id(decrypt_config->key_id());
+  config_message->set_iv(decrypt_config->iv());
+
+  for (const auto& entry : decrypt_config->subsamples()) {
+    pb::DecryptConfig::SubSample* sub_sample =
+        config_message->add_sub_samples();
+    sub_sample->set_clear_bytes(entry.clear_bytes);
+    sub_sample->set_cypher_bytes(entry.cypher_bytes);
+  }
+}
+
+void SerializeDecoderBuffer(
+    const scoped_refptr<::media::DecoderBuffer>& decoder_buffer,
+    pb::DecoderBuffer* buffer_message) {
+  if (decoder_buffer->end_of_stream()) {
+    buffer_message->set_is_eos(true);
+    return;
+  }
+
+  VLOG(2) << "timestamp:" << decoder_buffer->timestamp().InMicroseconds()
+          << " duration:" << decoder_buffer->duration().InMicroseconds();
+  buffer_message->set_timestamp_usec(
+      decoder_buffer->timestamp().InMicroseconds());
+  buffer_message->set_duration_usec(
+      decoder_buffer->duration().InMicroseconds());
+  buffer_message->set_is_key_frame(decoder_buffer->is_key_frame());
+
+  if (decoder_buffer->decrypt_config()) {
+    SerializeDecryptConfig(decoder_buffer->decrypt_config(),
+                           buffer_message->mutable_decrypt_config());
+  }
+
+  buffer_message->set_front_discard_usec(
+      decoder_buffer->discard_padding().first.InMicroseconds());
+  buffer_message->set_back_discard_usec(
+      decoder_buffer->discard_padding().second.InMicroseconds());
+  buffer_message->set_splice_timestamp_usec(
+      decoder_buffer->splice_timestamp().InMicroseconds());
+
+  if (decoder_buffer->side_data_size()) {
+    buffer_message->set_side_data(decoder_buffer->side_data(),
+                                  decoder_buffer->side_data_size());
+  }
+}
+
+}  // namespace
+
+DecoderBufferSegment::DecoderBufferSegment(
+    const scoped_refptr<::media::DecoderBuffer>& decoder_buffer)
+    : decoder_buffer_(decoder_buffer) {}
+
+DecoderBufferSegment::~DecoderBufferSegment() = default;
+
+// static
+scoped_refptr<DecoderBufferSegment> DecoderBufferSegment::FromBuffer(
+    const uint8_t* data,
+    uint32_t size) {
+  uint32_t pos = 0;
+
+  int payload_version = data[pos++];

[miu, 2016/09/08 01:07:34]

In Chrome, please use base::BigEndianReader instead of reading bytes out of the
byte array directly. It's safer, and also much nicer for code readability
(checks size boundaries, etc. for you).

  base::BigEndianReader reader(data, size);
  int payload_version;
  uint32_t proto_size;
  pb::DecoderBuffer segment;
  uint32_t buffer_size;
  if (reader.ReadU8(&payload_version) &&
      payload_version == 0 &&
      reader.ReadU32(&proto_size) &&
      static_cast<int64_t>(proto_size) < reader.remaining() &&
      segment.ParseFromArray(reader.ptr(), proto_size) &&
      reader.Skip(proto_size) &&
      reader.ReadU32(&buffer_size) &&
      static_cast<int64_t>(buffer_size) <= reader.remaining()) {
    decoder_buffer =
        DeserializeDecoderBuffer(segment,
                                 DecoderBuffer::CopyFrom(reader.ptr(),
buffer_size));
    if (decoder_buffer)
      return new DecoderBufferSegment(decoder_buffer);
  }
  return nullptr;

[erickung1, 2016/09/13 03:52:52]

Done.

+  DCHECK_EQ(payload_version, 0);
+
+  DCHECK(size > pos + kProtoBufferHeaderSize);
+  uint32_t proto_size = (data[pos] << 8) | data[pos + 1];
+  pos += kProtoBufferHeaderSize;
+
+  DCHECK(size > pos + proto_size);
+  pb::DecoderBuffer segment;
+  if (!segment.ParseFromArray(data + pos, proto_size)) {
+    LOG(ERROR) << "Bad proto buffer";
+    return nullptr;
+  }
+  pos += proto_size;
+
+  DCHECK(size >= pos + kDataBufferHeaderSize);
+  uint32_t buffer_size = (data[pos] << 24) | (data[pos + 1] << 16) |
+                         (data[pos + 2] << 8) | data[pos + 3];
+  pos += kDataBufferHeaderSize;
+
+  DCHECK(size == pos + buffer_size);
+  // Create DecoderBuffer first to allocate buffer.
+  scoped_refptr<DecoderBuffer> decoder_buffer =
+      DecoderBuffer::CopyFrom(data + pos, buffer_size);
+  // Deserialize proto buffer. It passes the pre allocated DecoderBuffer into
+  // the function because the proto buffer may overwrite DecoderBuffer since it
+  // may be EOS buffer.
+  decoder_buffer = DeserializeDecoderBuffer(segment, std::move(decoder_buffer));
+
+  return new DecoderBufferSegment(decoder_buffer);
+}
+
+uint32_t DecoderBufferSegment::ToBuffer(std::vector<uint8_t>* buffer) {
+  DCHECK(buffer);
+  pb::DecoderBuffer decoder_buffer_data;

[miu, 2016/09/08 01:07:34]

naming nit: How about decoder_buffer_message?

[erickung1, 2016/09/13 03:52:53]

Done.

+  SerializeDecoderBuffer(decoder_buffer_, &decoder_buffer_data);
+
+  size_t decoder_buffer_size =
+      decoder_buffer_->end_of_stream() ? 0 : decoder_buffer_->data_size();
+  size_t size = kPayloadVersionFieldSize + kProtoBufferHeaderSize +
+                decoder_buffer_data.ByteSize() + kDataBufferHeaderSize +
+                decoder_buffer_size;
+  buffer->resize(size);
+  int pos = 0;

[miu, 2016/09/08 01:07:34]

Like above, please use base::BigEndianWriter here:

  base::BigEndianWriter writer(buffer->data(), buffer->size());
  CHECK(writer.WriteU8(0) &&
        writer.WriteU16(decoder_buffer_data.ByteSize() &&
        decoder_buffer_data.SerializeToArray(writer.ptr(),
                                             decoder_buffer_data.ByteSize()) &&
        writer.Skip(decoder_buffer_data.ByteSize()) &&
        writer.WriteU32(decoder_buffer_size) &&
        writer.WriteBytes(decoder_buffer_->data(),
decoder_buffer_->data_size()));

[erickung1, 2016/09/13 03:52:52]

Done.

+  // payload_version, default is 0.
+  (*buffer)[pos++] = 0;
+  // Length of protobuf-encoded segment.
+  (*buffer)[pos++] = (decoder_buffer_data.ByteSize() >> 8) & 0xff;
+  (*buffer)[pos++] = decoder_buffer_data.ByteSize() & 0xff;
+  // Protobuf-encoded DecoderBuffer.
+  DCHECK(decoder_buffer_data.SerializeToArray((*buffer).data() + pos,

[miu, 2016/09/08 01:07:34]

FYI--DCHECKs() do not evaluate in release builds. So, SerializeToArray() would
not be called in a release build.

[erickung1, 2016/09/13 03:52:52]

Done.

+                                              decoder_buffer_data.ByteSize()));
+  pos += decoder_buffer_data.ByteSize();
+  // Length of data segment.
+  (*buffer)[pos++] = (decoder_buffer_size >> 24) & 0xff;
+  (*buffer)[pos++] = (decoder_buffer_size >> 16) & 0xff;
+  (*buffer)[pos++] = (decoder_buffer_size >> 8) & 0xff;
+  (*buffer)[pos++] = decoder_buffer_size & 0xff;
+  // DecoderBuffer frame data.
+  if (decoder_buffer_size) {
+    std::copy(decoder_buffer_->data(),
+              decoder_buffer_->data() + decoder_buffer_->data_size(),
+              (*buffer).data() + pos);
+  }
+  return size;
+}
+
+}  // namespace remoting
+}  // namespace media