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/big_endian.h"
+#include "base/logging.h"
+#include "base/time/time.h"
+#include "base/values.h"
+#include "media/base/decrypt_config.h"
+#include "media/remoting/remoting_rpc_message.pb.h"
+
+namespace media {
+namespace remoting {
+
+namespace {
+
+const int kPayloadVersionFieldSize = 1;
+const int kProtoBufferHeaderSize = 2;
+const int kDataBufferHeaderSize = 4;
+
+std::unique_ptr<::media::DecryptConfig> DeserializeDecryptConfig(

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

naming nit: This isn't deserializing from a wire format, but converting from the
protobuf type to the impl type. So, how about "ConvertProtoToDecryptConfig()"?

...and same for DeserializeDecoderBuffer --> ConvertProtoToDecoderBuffer

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

Done.

+    const pb::DecryptConfig& config_message) {
+  std::vector<::media::SubsampleEntry> entries;
+
+  DCHECK(config_message.has_key_id());
+  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;

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

Looks like this variable is unused. Otherwise, please declare it where it is
first used, to aid in code readability.

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

Done. remove |decrypt_config| and also move |front_discard|, |back_discard| and
|has_discard| close to the place being used
Also change the default value of |has_discard| to false

+  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()) {
+    buffer->set_decrypt_config(
+        DeserializeDecryptConfig(buffer_message.decrypt_config()));
+  }
+
+  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) {

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

|has_discard| seems to be true no matter what. Is that what you intended?

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

Done. Yes, just noticed it and change the default value to false

+    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().data()),
+        buffer_message.side_data().size());
+  }
+
+  return buffer;
+}
+
+void SerializeDecryptConfig(const ::media::DecryptConfig& decrypt_config,

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

naming nit: This isn't serializing to a wire format, but converting from one
type to the protobuf type. So, how about "ConvertDecryptConfigToProto()"?

...and same for SerializeDecoderBuffer --> ConvertDecoderBufferToProto

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

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
+
+scoped_refptr<::media::DecoderBuffer> ByteArrayToDecoderBuffer(
+    const uint8_t* data,
+    uint32_t size) {
+  base::BigEndianReader reader(reinterpret_cast<const char*>(data), size);
+  uint8_t payload_version = 0;
+  uint32_t proto_size = 0;
+  pb::DecoderBuffer segment;
+  uint32_t buffer_size = 0;
+  if (reader.ReadU8(&payload_version) && payload_version == 0 &&
+      reader.ReadU32(&proto_size) &&

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

Looks like this should be reader.ReadU16(&proto_size).
                                     ^^^

Consider adding a quick unit test that converts using both functions and gets
back the same media::DecoderBuffer data that it started with. That would catch
things like this.

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

Done. Yes, fixed in patch set#8 and also have this in the unit test

+      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()) {
+    // 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.
+    scoped_refptr<media::DecoderBuffer> decoder_buffer =
+        DeserializeDecoderBuffer(
+            segment,
+            DecoderBuffer::CopyFrom(
+                reinterpret_cast<const uint8_t*>(reader.ptr()), buffer_size));
+    return decoder_buffer;
+  }
+  return nullptr;
+}
+
+std::vector<uint8_t> DecoderBufferToByteArray(
+    const scoped_refptr<::media::DecoderBuffer> decoder_buffer) {
+  pb::DecoderBuffer decoder_buffer_message;
+  SerializeDecoderBuffer(decoder_buffer, &decoder_buffer_message);
+
+  std::vector<uint8_t> buffer;

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

Let's move this down to just before the call to resize()? Actually, you might be
able to just do the declaration and resize() in one simple LOC:

  std::vector<uint8_t> buffer(size);

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

Done.

+  size_t decoder_buffer_size =
+      decoder_buffer->end_of_stream() ? 0 : decoder_buffer->data_size();
+  size_t size = kPayloadVersionFieldSize + kProtoBufferHeaderSize +
+                decoder_buffer_message.ByteSize() + kDataBufferHeaderSize +

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

I see you're calling decoder_buffer_message.ByteSize() several times in this
method. This will re-compute the size each time. With protos, you should call
GetCachedSize() after this point.

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

Done.

+                decoder_buffer_size;
+  buffer.resize(size);
+
+  base::BigEndianWriter writer(reinterpret_cast<char*>(buffer.data()),
+                               buffer.size());
+  if (writer.WriteU8(0) &&
+      writer.WriteU16(
+          static_cast<uint16_t>(decoder_buffer_message.ByteSize())) &&
+      decoder_buffer_message.SerializeToArray(
+          writer.ptr(), decoder_buffer_message.ByteSize()) &&
+      writer.Skip(decoder_buffer_message.ByteSize()) &&
+      writer.WriteU32(decoder_buffer_size)) {
+    if (decoder_buffer_size) {
+      // DecoderBuffer frame data.
+      writer.WriteBytes(reinterpret_cast<const void*>(decoder_buffer->data()),
+                        decoder_buffer->data_size());
+    }
+    return buffer;
+  }
+
+  // Reset buffer since data is corrupted during serialization.

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

s/data is corrupted during serialization/serialization of the data failed/

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

Done.

+  buffer.clear();
+  return buffer;
+}
+
+}  // namespace remoting
+}  // namespace media