Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2261503002:
Define remote playback proto buffer (Closed)
DescriptionDefine remote playback proto buffer
This allows us to send RPC message to remote device for content playback.
Committed: https://crrev.com/1fc58a4b0bd5d0a58ce8a353a0c7840cd6561f75
Cr-Commit-Position: refs/heads/master@{#420821}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Update RPC to in sync with CDM #Patch Set 3 : Add OWNERS file #
Total comments: 7
Patch Set 4 : Update RPC message matching latest code base #Patch Set 5 : DecoderBufferSegment more description and change interface name #
Total comments: 71
Patch Set 6 : Remove Rpc wrapper class #Patch Set 7 : Add OWNERS file #
Total comments: 44
Patch Set 8 : fix bug, update unit test #Patch Set 9 : Refine Rpc untility function #Patch Set 10 : Add proto enum utils, merge utils function to single file, same as unit test #Patch Set 11 : Fix typo in BUILD.gn #Patch Set 12 : Change some field to unit32 and add missing enum type in RPC #
Total comments: 26
Patch Set 13 : Address comments from patch set #12 #Patch Set 14 : Add README and update build option #
Total comments: 2
Patch Set 15 : media shouldn't have dependency on media/remoting #Patch Set 16 : Remove all change in src/BUILD.gn and media_options.gni #
Messages
Total messages: 43 (9 generated)
Description was changed from ========== Define remote playback proto buffer This allows us to send RPC message to remote device for content playback. Please refer full WIP CL (https://codereview.chromium.org/2089133003) BUG= ========== to ========== Define remote playback proto buffer This allows us to send RPC message to remote device for content playback. Please refer full WIP CL (https://codereview.chromium.org/2089133003) BUG= ==========
erickung@chromium.org changed reviewers: + dalecurtis@chromium.org, ddorwin@chromium.org, xhwang@chromium.org
I only looked at media/ :) https://chromiumcodereview.appspot.com/2261503002/diff/1/media/BUILD.gn File media/BUILD.gn (right): https://chromiumcodereview.appspot.com/2261503002/diff/1/media/BUILD.gn#newco... media/BUILD.gn:476: deps += [ "//media/remoting:rpc" ] OOC, why media needs to depend on this? https://chromiumcodereview.appspot.com/2261503002/diff/1/media/media_options.gni File media/media_options.gni (right): https://chromiumcodereview.appspot.com/2261503002/diff/1/media/media_options.... media/media_options.gni:128: # flag to enable media remoting feature. Could you please provide more context about what "media remoting feature" is?
Eric, Please add a media/remoting/OWNERS file too. Probably you and me should be in it for now. I'll try to take a first look at this tonight and post some comments.
erickung@chromium.org changed reviewers: + miu@chromium.org
Add OWNERS file per miu@ request https://codereview.chromium.org/2261503002/diff/1/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/2261503002/diff/1/media/BUILD.gn#newcode476 media/BUILD.gn:476: deps += [ "//media/remoting:rpc" ] On 2016/08/25 20:00:15, xhwang (ooo_until_sept_8) wrote: > OOC, why media needs to depend on this? once we checked in the remote renderer and remote media keys implementation, we'll make change to have media depending on that target, and have that target depending on rpc. Currently it is to make sure the code is built without any issue https://codereview.chromium.org/2261503002/diff/1/media/media_options.gni File media/media_options.gni (right): https://codereview.chromium.org/2261503002/diff/1/media/media_options.gni#new... media/media_options.gni:128: # flag to enable media remoting feature. On 2016/08/25 20:00:15, xhwang (ooo_until_sept_8) wrote: > Could you please provide more context about what "media remoting feature" is? Sure, the media remoting is the original project we discussed. Instead of using mojo::renderer and mojo:cdm, we decide to create its own mojo service, its own renderer and its own media keys implementation, to covert message to RPC using proto buffer in the render process, and send the byte array to browser process. With such solution, we can eliminate security concern on parsing data in browser process.
xjz@chromium.org changed reviewers: + xjz@chromium.org
https://codereview.chromium.org/2261503002/diff/40001/media/remoting/rpc/deco... File media/remoting/rpc/decoder_buffer_segment.h (right): https://codereview.chromium.org/2261503002/diff/40001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.h:20: class DecoderBufferSegment Can you add some comments briefly describe this class? https://codereview.chromium.org/2261503002/diff/40001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.h:27: uint32_t ToBuffer(std::vector<uint8_t>* buffer); Suggest use a more meaningful name, and use std::string as the parameter. So maybe void SerializeDecoderBuffer(std::string& data) or void FromDecoderBuffer(std::string& data)? https://codereview.chromium.org/2261503002/diff/40001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.h:30: static scoped_refptr<DecoderBufferSegment> FromBuffer(const uint8_t* data, This method creates a new object. Maybe name it Create...? Or can we instead add another constructor with the string as input parameter?
https://codereview.chromium.org/2261503002/diff/40001/media/remoting/rpc/deco... File media/remoting/rpc/decoder_buffer_segment.h (right): https://codereview.chromium.org/2261503002/diff/40001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.h:20: class DecoderBufferSegment On 2016/09/02 18:29:38, xjz wrote: > Can you add some comments briefly describe this class? Done. https://codereview.chromium.org/2261503002/diff/40001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.h:27: uint32_t ToBuffer(std::vector<uint8_t>* buffer); On 2016/09/02 18:29:38, xjz wrote: > Suggest use a more meaningful name, and use std::string as the parameter. So > maybe void SerializeDecoderBuffer(std::string& data) or void > FromDecoderBuffer(std::string& data)? change it to std::string Serialzie(); https://codereview.chromium.org/2261503002/diff/40001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.h:30: static scoped_refptr<DecoderBufferSegment> FromBuffer(const uint8_t* data, On 2016/09/02 18:29:38, xjz wrote: > This method creates a new object. Maybe name it Create...? Or can we instead add > another constructor with the string as input parameter? Done.
Finally sat down and did the much more detail review (of Patch Set 5): https://codereview.chromium.org/2261503002/diff/40001/media/remoting/rpc/deco... File media/remoting/rpc/decoder_buffer_segment.h (right): https://codereview.chromium.org/2261503002/diff/40001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.h:27: uint32_t ToBuffer(std::vector<uint8_t>* buffer); On 2016/09/06 09:37:24, erickung1 wrote: > On 2016/09/02 18:29:38, xjz wrote: > > Suggest use a more meaningful name, and use std::string as the parameter. So > > maybe void SerializeDecoderBuffer(std::string& data) or void > > FromDecoderBuffer(std::string& data)? > > change it to std::string Serialzie(); If this string is going to be passed to the Mojo interface directly, or pushed into a Mojo data pipe, then std::vector<uint8_t> is more appropriate here. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... File media/remoting/proto/remoting_rpc_message.proto (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:13: enum RpcProc { Perhaps this enum should be nested inside of RpcMessage (since it is only used there)? https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:74: optional int64 timestamp_usec = 1; Some of these feel like they shouldn't be optional; or, if they should, there should be a default value (e.g., in the case of |is_eos|). https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:88: optional int32 width = 1; Why are these fields optional? Seems like the Size object can be optional, but once you have a Size, a Size should always consist of a known width and height. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:100: optional CipherMode mode = 1; Mode is optional? Should there be a default value here, then? (e.g., CIPHER_MODE_UNENCRYPTED) https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:187: optional int32 x = 1; ditto: Same concern as Size (fields should not be optional). https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:447: optional int32 handle = 1; Is this a session handle, or does it change per-message somehow? Please document a little more. :) https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:450: optional RpcProc proc = 2; I'm probably repeating myself a lot here, so I'll just say generally (for this whole file): Some fields should not be optional. This would allow the protobuf deserialization code to reject bad messages missing required fields, and greatly reduce the amount of code you have to write to validate the parsed messages downstream. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:480: string string_value = 7; Should this be of type |string| or |bytes|? I'm thinking probably the latter, to avoid char encoding interpretation? https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... File media/remoting/rpc/decoder_buffer_segment.cc (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:29: DCHECK(config_message.has_key_id()); These DCHECKs suggest the fields should not have been optional in the proto. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:75: buffer->set_decrypt_config(std::move(decrypt_config)); You can simplify here and eliminate the local variables: buffer->set_decrypt_config( DeserializeDecryptConfig(buffer_message.decrypt_config()); https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:101: reinterpret_cast<const uint8_t*>(buffer_message.side_data().c_str()), Don't use c_str() here. Use data() instead. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:108: void SerializeDecryptConfig(const ::media::DecryptConfig* decrypt_config, nit: Please pass by const-reference instead of const-pointer, since |decrypt_config| must never be null. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:171: int payload_version = data[pos++]; 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; https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:205: pb::DecoderBuffer decoder_buffer_data; naming nit: How about decoder_buffer_message? https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:214: int pos = 0; 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())); https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:221: DCHECK(decoder_buffer_data.SerializeToArray((*buffer).data() + pos, FYI--DCHECKs() do not evaluate in release builds. So, SerializeToArray() would not be called in a release build. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... File media/remoting/rpc/decoder_buffer_segment.h (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.h:23: explicit DecoderBufferSegment( This class should never be instantiated. It contains only one data member, |decoder_buffer_| which is really just a temporary object and could be provided by the callers of FromBuffer() and ToBuffer(). (Meaning, FromBuffer() and ToBuffer() could be static methods.) Maybe, instead of a class, you just need two utility functions: void ToByteVector(const media::DecoderBuffer& buffer, std::vector<uint8_t>* out); void FromArray(const uint8_t* data, size_t size, media::DecoderBuffer* out); IMHO, a lot less code that does everything you need. ;) https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.cc File media/remoting/rpc/rpc.cc (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:27: #define RPC(x) pb::x IMHO, you shouldn't do this. I was not able to immediately understand what this macro was doing in the code later in the file. It would have been much more obvious to understand pb::RPC_FOO_BAR_BAZ instead of RPC(RPC_FOO_BAR_BAZ). https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:41: if (message.has_encrypt_blocks()) Throughout this file: The default value for integer-typed optional fields is zero, and optional string-typed fields are empty strings. So, a lot of these has_xxx() checks are just extra code that does nothing. Also, IIRC, the "getters" for protobuf messages are simple and in-lined, so there's no need to copy them into local variables first. Putting it all together, for example, the code here could be more simply: REQUIRED(message.encrypt_blocks() >= 0); REQUIRED(message.skip_blocks() >= 0); pattern = EncryptionScheme::Pattern(message.encrypt_blocks(), message.skip_blocks()); https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:71: ::media::AudioCodec codec = ::media::kUnknownAudioCodec; General comment: Seems like a lot of these "converter" functions could be simplified by eliminating all the local variables and just passing the mapped/type-casted values directly to the media::XXXConfig::Initialize() method. For example, this whole function could be: ... { DCHECK(audio_config); audio_config->Initialize( static_cast<::media::AudioCodec>(audio_message.codec()), static_cast<::media::SampleFormat>(audio_message.sample_format()), static_cast<::media::ChannelLayout>(audio_message.channel_layout()), audio_message.samples_per_second(), audio_message.extra_data(), DeserializeEncryptionScheme(audio_message.encryption_scheme()), base::TimeDelta::FromMicroseconds(audio_message.seek_preroll_usec()), audio_message.codec_delay()); return audio_config->IsValidConfig(); } https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:102: return true; Suggest returning audio_config->IsValidConfig() here instead, and in DeserializeVideoConfig(). https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:146: bool DeserializeCdmPromiseResult(const pb::CdmPromise& promise_message, General comment: Please make sure you validate the values that have been parsed, and return false when they are illegal in these converter functions. For example, this function always returns true (or if all possible field values are legal, maybe it should have a void return type instead?). https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:170: std::vector<::media::CdmKeyInformation> info; nit: When populating vectors where you know their final size beforehand, please call std::vector::reserve(). (Please check that you are doing this throughout your change.) https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:193: pb::EncryptionScheme* message, style: Input arguments, then output arguments. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:207: void SerializeRect(pb::Rect* message, const gfx::Rect& rect) { here too: Input arguments, then output arguments. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:220: decoder_config_message->set_extra_data(&config.extra_data()[0], nit: In C++11 code, you can now use config.extra_data().data() instead of &config.extra_data()[0]. This seems to appear in a lot of places in this change, so please search-and-replace throughout all the files. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:287: std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue()); Looks like this DictionaryValue is unused. Can you remove it? https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.h File media/remoting/rpc/rpc.h (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.h:118: class Rpc : public base::RefCountedThreadSafe<Rpc> { Is there a special reason to ref-count this object? Chromium C++ guide says to avoid use of ref-counting if at all possible: https://www.chromium.org/developers/smart-pointer-guidelines tl;dr: Ownership and lifetime can more easily be reasoned when not using ref-counting; and ref-counting can often be eliminated by making better design choices. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.h:123: static scoped_refptr<Rpc> FromMessage(const std::string& proto); Note: Since these will come from the Mojo interface, the type here should be const std::vector<uint8_t>&. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.h:127: std::string ToMessage() const; ditto: If these are to be sent via the Mojo interface, the return type should be std::vector<uint8_t>. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.h:198: class RendererInitializeRpc : public Rpc { General comments for rest of file: Do we really need all these wrapper classes? For instance, how is a pb::RendererInitialize any different from a RendererInitializeRpc? Unless I'm missing something important, these wrappers are a ton of code that don't add any value. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc_... File media/remoting/rpc/rpc_unittest.cc (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc_... media/remoting/rpc/rpc_unittest.cc:42: DCHECK(!to_rpc); I think you meant to use EXPECT_TRUE/FALSE() instead of DCHECK() throughout this file. Guideline: Use EXPECT_XXX() when it is safe for your test code to continue even if the expected result is wrong. Use ASSERT_XXX() when your test will crash if it continues, or if the test output will be super-spammy if the test continues running. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc_... media/remoting/rpc/rpc_unittest.cc:46: TEST_F(RemotingRpcTest, AcquireRendererRpc) { General comments for rest of file: If you ditch the Rpc wrapper classes, none of these are needed, since you assume the protobuf compiler/library are working. However, it would still be good to write tests to confirm your converters work, and also to confirm that invalid inputs are recognized as invalid in the various parsing/conversion functions.
the comment is to reply if we wrapper class for every RPC. If we don't need it, definitely we can reduce lots of line of code in this CL :D Please let me know. Thanks. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.h File media/remoting/rpc/rpc.h (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.h:198: class RendererInitializeRpc : public Rpc { On 2016/09/08 01:07:35, miu wrote: > General comments for rest of file: Do we really need all these wrapper classes? > For instance, how is a pb::RendererInitialize any different from a > RendererInitializeRpc? Unless I'm missing something important, these wrappers > are a ton of code that don't add any value. We had similar discussion before sending out this CL. I don't have strong opinion if we need all the wrapper. The reason I kept the wrapper here is to 1. isolate tons of pb related conversion code in this wrapper/helper/utility class, so that the real code in remote_render_impl.cc and remote_media_keys.cc in the CL (https://codereview.chromium.org/2089133003/) is simple and less code. 2. I replace the message from JSON to proto, having a wrapper class seems creating the flexibility if we want to go with other solution. (to decouple proto code out of render and mediakeys implementation. Take CdmCreateSessionAndGenerateRequestRpc for example. the caller in remote_media_keys.cc is like the following. The code is to focus on how to correlate promise into RPC, and register the proper handle in order to get the right rpc message response back. Once |rpc_broker| dispatches the request, it will call ToMessage to get the proto buff as a string. void RemoteMediaKeys::CreateSessionAndGenerateRequest( SessionType session_type, media::EmeInitDataType init_data_type, const std::vector<uint8_t>& init_data, std::unique_ptr<media::NewSessionCdmPromise> promise) { LOG(INFO) << __FUNCTION__; int promise_id = cdm_promise_adapter_.SavePromise(std::move(promise)); int callback_handle = rpc_broker_->GenerateHandle(); scoped_refptr<remoting::Rpc> rpc( new remoting::CdmCreateSessionAndGenerateRequestRpc( remote_cdm_handle_, session_type, init_data_type, &init_data[0], init_data.size(), callback_handle)); promise_handle_map_[promise_id] = callback_handle; rpc_broker_->RegisterHandle(callback_handle, this); rpc_broker_->DispatchRemote(rpc); } If we eliminate the wrapper, the code will like the following void RemoteMediaKeys::CreateSessionAndGenerateRequest( SessionType session_type, media::EmeInitDataType init_data_type, const std::vector<uint8_t>& init_data, std::unique_ptr<media::NewSessionCdmPromise> promise) { LOG(INFO) << __FUNCTION__; int promise_id = cdm_promise_adapter_.SavePromise(std::move(promise)); int callback_handle = rpc_broker_->GenerateHandle(); promise_handle_map_[promise_id] = callback_handle; rpc_broker_->RegisterHandle(callback_handle, this); pb::RpcMessage rpc; rpc.set_handle(remote_cdm_handle); rpc.set_proc(pb::RPC_CDM_CREATESESSIONANDGENERATEREQUEST); pb::CdmCreateSessionAndGenerateRequest* message = rpc->mutable_cdm_createsessionandgeneraterequest_rpc(); message->set_session_type(session_type); message->set_init_data_type(static_cast<int>(init_data_type)); message->set_callback_handle(callback_handle); message->set_init_data(&init_data[0], init_data.size()); std::string proto_msg; bool valid = rpc.ByteSize() > 0 && rpc.SerializeToString(&proto_msg); rpc_broker_->DispatchRemote(proto_msg); // Change the data format to string } and it will be similar at the receiver side. So, it would be OK if you think we don't need extra code here for such purposes. But regardless, we probably need the base class Rpc at least.
https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.h File media/remoting/rpc/rpc.h (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.h:198: class RendererInitializeRpc : public Rpc { On 2016/09/08 18:18:45, erickung1 wrote: > On 2016/09/08 01:07:35, miu wrote: > > General comments for rest of file: Do we really need all these wrapper > classes? > > For instance, how is a pb::RendererInitialize any different from a > > RendererInitializeRpc? Unless I'm missing something important, these wrappers > > are a ton of code that don't add any value. I should also admit: I myself am often tempted to add layers in code to provide more-generic interfaces to client code. For example, I recently did this in one of my iterations of the browser-side code, where I wrote a bunch of generic Media Router infrastructure to support "media remoting" on any platform (i.e., not just Cast). However, we should resist doing things like this at this point for a number of very good reasons: 1. This is a first implementation. By the time we finish this project, I'm certain we're all going to be thinking about how we could have done things better (code structure, etc.); and likely to have technical debt to pay off that will require us to refactor large chunks of code. 2. By adding extra layers, we're solving a problem we don't have yet. In the case of this change, you mentioned supporting alternate formats for wire formatting (such as JSON). Because of reason #1, adding extra layers now will make it harder for us to change our code structure later. 3. I'm concerned that adding too much new code too fast will trigger certain Chromium waterfall alerts, such as "sizes" (which measures bloat in the chrome binaries), and memory usage (since we're creating lots of extra small objects and copying things around a bit more). 4. We should try to minimize the amount of code review effort from stakeholders outside our project. Along these lines, less code that does more always wins (and is usually the better technical choice anyway). > 1. isolate tons of pb related conversion code in this wrapper/helper/utility > class, so that the real code in remote_render_impl.cc and remote_media_keys.cc > in the CL (https://codereview.chromium.org/2089133003/) is simple and less code. > Take CdmCreateSessionAndGenerateRequestRpc for example. I looked at the example you provided in your last comment, and I'm not seeing there's much of a difference. The code is identical except, when using the wrapper class, this code: scoped_refptr<remoting::Rpc> rpc( new remoting::CdmCreateSessionAndGenerateRequestRpc( remote_cdm_handle_, session_type, init_data_type, &init_data[0], init_data.size(), callback_handle)); rpc_broker_->DispatchRemote(rpc); Becomes: pb::RpcMessage rpc; rpc.set_handle(remote_cdm_handle); rpc.set_proc(pb::RPC_CDM_CREATESESSIONANDGENERATEREQUEST); pb::CdmCreateSessionAndGenerateRequest* message = rpc->mutable_cdm_createsessionandgeneraterequest_rpc(); message->set_session_type(session_type); message->set_init_data_type(static_cast<int>(init_data_type)); message->set_callback_handle(callback_handle); message->set_init_data(&init_data[0], init_data.size()); rpc_broker_->DispatchRemote(rpc); So, we've gained 5 LOC here, but saved ~100 LOC (the wrapper code); and we've removed an additional object allocation/copying for each of these RPCs. And then there's the 4 other considerations mentioned above. > 2. I replace the message from JSON to proto, having a wrapper class seems > creating the flexibility if we want to go with other solution. (to decouple > proto code out of render and mediakeys implementation. The cool thing about protobufs is that they provide custom "serializers." There is already an ability to serialize as JSON, for example. Also, protobufs have a lot of other useful features built-in, like basic input validation, a text formatter for logging as human-readable strings for when you're debugging, etc. > So, it would be OK if you think we don't need extra code here for such purposes. It's important you agree too. :) This whole issue is about weighing the benefits versus the costs of going in either direction. Now that you have more information, does it sound right to you to eliminate the wrappers? > But regardless, we probably need the base class Rpc at least. FWICT, probably not. It seems like client code provides the Rpc class with all the data it needs. Therefore, if the client code is working with the protobuf-generated classes directly, the Rpc class isn't needed. Your RpcBroker methods become simply: void RpcBroker::Dispatch(const std::vector<uint8_t>& serialized_message) { ::media::remoting::pb::RpcMessage message; if (!message.ParseFromArray(serialized_message.data(), serialized_message.size())) return; const auto entry = handles_.find(message.handle()); if (entry == handles_.end()) return; entry->second->OnReceivedRpc(message); } void RpcBroker::DispatchRemote(const ::media::remoting::pb::RpcMessage& message) { std::vector<uint8_t> serialized_message(message.ByteSize()); CHECK(message.SerializeToArray(serialized_message.data(), serialized_message.size())); send_message_cb_.Run(std::move(serialized_message)); }
OK, I'll eliminate rpc.cc implementation. Meantime, please see my comments regarding to "optional" in proto buffer. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... File media/remoting/proto/remoting_rpc_message.proto (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:74: optional int64 timestamp_usec = 1; On 2016/09/08 01:07:34, miu wrote: > Some of these feel like they shouldn't be optional; or, if they should, there > should be a default value (e.g., in the case of |is_eos|). The reason I use optional for every field in the beginning 1. although I use proto2, but in proto3 every field will become optional 2. If we make some field to repeated, the field can't be deprecated in the future because the version mismatch will be an issue(or I think Chrome browser always forced people to update??) 3. Some fields like Rect may be straightforward to make it to repeated. but take this DecoderBuffer example, it's not. For example, - if this is EOS frame, we will set the is_eos - if it's a valid buffer, we set timestamp - if it's key frame, we set is_key_frame - if it's encrypted content, we set decrypt_config So it makes me feel like we have to spend amount of time thinking this by defining the proto structure. The best thing that we should do, is probably just to set everything to optional, but also set a default value. and for both sender and receiver who are interested in particular RPC, should check if necessary information are there, otherwise, considering it as invalid RPC message. How do you think??
On 2016/09/08 22:21:50, erickung1 wrote: > OK, I'll eliminate rpc.cc implementation. > > Meantime, please see my comments regarding to "optional" in proto buffer. > > https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... > File media/remoting/proto/remoting_rpc_message.proto (right): > > https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... > media/remoting/proto/remoting_rpc_message.proto:74: optional int64 > timestamp_usec = 1; > On 2016/09/08 01:07:34, miu wrote: > > Some of these feel like they shouldn't be optional; or, if they should, there > > should be a default value (e.g., in the case of |is_eos|). > > The reason I use optional for every field in the beginning > > 1. although I use proto2, but in proto3 every field will become optional > 2. If we make some field to repeated, the field can't be deprecated in the > future because the version mismatch will be an issue(or I think Chrome browser > always forced people to update??) > 3. Some fields like Rect may be straightforward to make it to repeated. but take > this DecoderBuffer example, it's not. > > For example, > - if this is EOS frame, we will set the is_eos > - if it's a valid buffer, we set timestamp > - if it's key frame, we set is_key_frame > - if it's encrypted content, we set decrypt_config > > So it makes me feel like we have to spend amount of time thinking this by > defining the proto structure. The best thing that we should do, is probably just > to set everything to optional, but also set a default value. and for both sender > and receiver who are interested in particular RPC, should check if necessary > information are there, otherwise, considering it as invalid RPC message. > > How do you think?? Also, for those integer...ect type, the default value in proto buffer is 0, which is the value we expect if not setting it. That also makes me think we may not need default value for every field.
On 2016/09/08 22:21:50, erickung1 wrote: > So it makes me feel like we have to spend amount of time thinking this by > defining the proto structure. The best thing that we should do, is probably just > to set everything to optional, but also set a default value. and for both sender > and receiver who are interested in particular RPC, should check if necessary > information are there, otherwise, considering it as invalid RPC message. > > How do you think?? Ah, yes. Versioning is a concern. Okay, this sounds good to me then (optional fields ok).
On 2016/09/08 22:23:35, erickung1 wrote: > Also, for those integer...ect type, the default value in proto buffer is 0, > which is the value we expect if not setting it. > That also makes me think we may not need default value for every field. Also sounds good.
up to patchset #8, ditch all RPC wrapper class and also the unit test. The utility functions are still in both rpc.cc and decoder_buffer_segment.cc. This is temp for review, and I'm going to merge into single file. Change all utility function to void as most of them doesn't have concept of "invalid" except for VideoDecoderConfig/AudioDecoderConfig. This will let sender/receiver to do error handling. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... File media/remoting/proto/remoting_rpc_message.proto (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:13: enum RpcProc { On 2016/09/08 01:07:34, miu wrote: > Perhaps this enum should be nested inside of RpcMessage (since it is only used > there)? Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:74: optional int64 timestamp_usec = 1; On 2016/09/08 01:07:34, miu wrote: > Some of these feel like they shouldn't be optional; or, if they should, there > should be a default value (e.g., in the case of |is_eos|). after the discussion, will put every proto field optional especially for version control https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:88: optional int32 width = 1; On 2016/09/08 01:07:34, miu wrote: > Why are these fields optional? Seems like the Size object can be optional, but > once you have a Size, a Size should always consist of a known width and height. after the discussion, will put every proto field optional especially for version control https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:100: optional CipherMode mode = 1; On 2016/09/08 01:07:34, miu wrote: > Mode is optional? Should there be a default value here, then? (e.g., > CIPHER_MODE_UNENCRYPTED) ditto https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:187: optional int32 x = 1; On 2016/09/08 01:07:34, miu wrote: > ditto: Same concern as Size (fields should not be optional). ditto https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:447: optional int32 handle = 1; On 2016/09/08 01:07:34, miu wrote: > Is this a session handle, or does it change per-message somehow? Please document > a little more. :) Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:450: optional RpcProc proc = 2; On 2016/09/08 01:07:34, miu wrote: > I'm probably repeating myself a lot here, so I'll just say generally (for this > whole file): Some fields should not be optional. This would allow the protobuf > deserialization code to reject bad messages missing required fields, and greatly > reduce the amount of code you have to write to validate the parsed messages > downstream. ditto https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:480: string string_value = 7; On 2016/09/08 01:07:34, miu wrote: > Should this be of type |string| or |bytes|? I'm thinking probably the latter, to > avoid char encoding interpretation? This message is to send "session id" as a string back to sender. Wouldn't be proto buffer taking care such thing already? https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... File media/remoting/rpc/decoder_buffer_segment.cc (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:29: DCHECK(config_message.has_key_id()); On 2016/09/08 01:07:34, miu wrote: > These DCHECKs suggest the fields should not have been optional in the proto. 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. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:75: buffer->set_decrypt_config(std::move(decrypt_config)); On 2016/09/08 01:07:34, miu wrote: > You can simplify here and eliminate the local variables: > > buffer->set_decrypt_config( > DeserializeDecryptConfig(buffer_message.decrypt_config()); Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:101: reinterpret_cast<const uint8_t*>(buffer_message.side_data().c_str()), On 2016/09/08 01:07:34, miu wrote: > Don't use c_str() here. Use data() instead. Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:108: void SerializeDecryptConfig(const ::media::DecryptConfig* decrypt_config, On 2016/09/08 01:07:34, miu wrote: > nit: Please pass by const-reference instead of const-pointer, since > |decrypt_config| must never be null. Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:171: int payload_version = data[pos++]; On 2016/09/08 01:07:34, miu wrote: > 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; Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:205: pb::DecoderBuffer decoder_buffer_data; On 2016/09/08 01:07:34, miu wrote: > naming nit: How about decoder_buffer_message? Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:214: int pos = 0; On 2016/09/08 01:07:34, miu wrote: > 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())); Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:221: DCHECK(decoder_buffer_data.SerializeToArray((*buffer).data() + pos, On 2016/09/08 01:07:34, miu wrote: > FYI--DCHECKs() do not evaluate in release builds. So, SerializeToArray() would > not be called in a release build. Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... File media/remoting/rpc/decoder_buffer_segment.h (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.h:23: explicit DecoderBufferSegment( On 2016/09/08 01:07:34, miu wrote: > This class should never be instantiated. It contains only one data member, > |decoder_buffer_| which is really just a temporary object and could be provided > by the callers of FromBuffer() and ToBuffer(). (Meaning, FromBuffer() and > ToBuffer() could be static methods.) > > Maybe, instead of a class, you just need two utility functions: > > void ToByteVector(const media::DecoderBuffer& buffer, std::vector<uint8_t>* > out); > > void FromArray(const uint8_t* data, size_t size, media::DecoderBuffer* out); > > IMHO, a lot less code that does everything you need. ;) Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc_... File media/remoting/rpc/rpc_unittest.cc (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc_... media/remoting/rpc/rpc_unittest.cc:46: TEST_F(RemotingRpcTest, AcquireRendererRpc) { On 2016/09/08 01:07:35, miu wrote: > General comments for rest of file: If you ditch the Rpc wrapper classes, none of > these are needed, since you assume the protobuf compiler/library are working. > However, it would still be good to write tests to confirm your converters work, > and also to confirm that invalid inputs are recognized as invalid in the various > parsing/conversion functions. ditched all the unit test, and only write test for the utility function.
Comments on Patch Set 7. (I also see you uploaded PS8. Will take a look tomorrow.) https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... File media/remoting/proto/remoting_rpc_message.proto (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:74: optional int64 timestamp_usec = 1; On 2016/09/08 22:21:50, erickung1 wrote: > On 2016/09/08 01:07:34, miu wrote: > > Some of these feel like they shouldn't be optional; or, if they should, there > > should be a default value (e.g., in the case of |is_eos|). > > The reason I use optional for every field in the beginning > > 1. although I use proto2, but in proto3 every field will become optional > 2. If we make some field to repeated, the field can't be deprecated in the > future because the version mismatch will be an issue(or I think Chrome browser > always forced people to update??) > 3. Some fields like Rect may be straightforward to make it to repeated. but take > this DecoderBuffer example, it's not. > > For example, > - if this is EOS frame, we will set the is_eos > - if it's a valid buffer, we set timestamp > - if it's key frame, we set is_key_frame > - if it's encrypted content, we set decrypt_config > > So it makes me feel like we have to spend amount of time thinking this by > defining the proto structure. The best thing that we should do, is probably just > to set everything to optional, but also set a default value. and for both sender > and receiver who are interested in particular RPC, should check if necessary > information are there, otherwise, considering it as invalid RPC message. > > How do you think?? Sounds good. Also, if we do encounter an invalid RPC message, seems like we should stop remoting immediately, since the two endpoints would be in inconsistent states. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:480: string string_value = 7; On 2016/09/08 01:07:34, miu wrote: > Should this be of type |string| or |bytes|? I'm thinking probably the latter, to > avoid char encoding interpretation? Could you please check this? Seems like it should be of type "bytes". https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... File media/remoting/rpc/decoder_buffer_segment.cc (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:29: DCHECK(config_message.has_key_id()); On 2016/09/08 01:07:34, miu wrote: > These DCHECKs suggest the fields should not have been optional in the proto. 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." https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.cc File media/remoting/rpc/rpc.cc (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:193: pb::EncryptionScheme* message, On 2016/09/08 01:07:35, miu wrote: > style: Input arguments, then output arguments. Doesn't look like this was addressed in the latest patch set. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... File media/remoting/rpc/decoder_buffer_segment.cc (right): https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:28: std::unique_ptr<::media::DecryptConfig> DeserializeDecryptConfig( 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 https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:50: std::unique_ptr<::media::DecryptConfig> decrypt_config; Looks like this variable is unused. Otherwise, please declare it where it is first used, to aid in code readability. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:91: if (has_discard) { |has_discard| seems to be true no matter what. Is that what you intended? https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:110: void SerializeDecryptConfig(const ::media::DecryptConfig& decrypt_config, 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 https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:170: reader.ReadU32(&proto_size) && 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. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:193: std::vector<uint8_t> buffer; 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); https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:197: decoder_buffer_message.ByteSize() + kDataBufferHeaderSize + 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. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:218: // Reset buffer since data is corrupted during serialization. s/data is corrupted during serialization/serialization of the data failed/ https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... File media/remoting/rpc/decoder_buffer_segment.h (right): https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.h:41: const scoped_refptr<::media::DecoderBuffer> decoder_buffer); nit: Please pass by const-ref: const ::media::DecoderBuffer& decoder_buffer. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc.cc File media/remoting/rpc/rpc.cc (right): https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:38: mode = static_cast<::media::EncryptionScheme::CipherMode>(message.mode()); The static_cast between enum types worries me: There's no compile-time check as to whether the set of possible values changes (or changes in ordering) in either the media/base files or our remoting proto. Without this compile-time check, there's nothing to stop another developer from breaking Media Remoting, and we don't want to have to reactively keep fixing code forever and ever. Also, it's possible developers will want to make changes to the media/base enums without breaking media remoting. Conversion functions would de-couple things so that can happen safely (e.g., for backwards-compatibility). So, we should have explicit enum type conversion functions to ensure the compiler won't allow anyone to commit breaking changes. Since this is tedious, I went ahead and wrote a script to generate the code for you. I'll e-mail it to you for inclusion in this change. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:39: if (message.has_encrypt_blocks()) Do we need "has" guards here? The default value will be zero, right? https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:43: DCHECK(encrypt_blocks >= 0 && skip_blocks >= 0); This will crash the process on bad external input. Instead, we should return false when this happens and shut down media remoting. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:51: if (!audio_config.IsValidConfig()) Since this is the sender generating an RPC message, trying to serialize an invalid audio config would be a bug. So, this should be: DCHECK(audio_config.IsValidConfig()); https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:112: audio_config->Initialize(codec, sample_format, channel_layout, Please see my comments from last week about simplifying this function, and also DeserializeVideoConfig() and DeserializeCdmPromiseResult(). https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:115: return true; Should be: return audio_config->IsValidConfig(); https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:153: return true; Should be: return video_config->IsValidConfig(); https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:179: if (!video_config.IsValidConfig()) ditto here: Should be DCHECK(...); https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:274: DCHECK(message.has_cdm_promise_rpc()); ditto: Return false instead. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:277: DCHECK(DeserializeCdmPromiseResult(promise_message, result)); This function won't be called in release builds. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:291: if (!session_id.empty()) Should this be a DCHECK(). Or, is it valid to sometimes send this message without a session_id? https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc.h File media/remoting/rpc/rpc.h (right): https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.h:29: extern const int kInvalidHandle; These don't seem to be used anywhere. Are you planning on using them in a future change? https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.h:32: void SerializeEncryptionScheme( General comment for these Serialize/Deserialize functions: 1. Naming nit: They aren't serializing/deserializing. They just convert between the ::media::Object type and the protobuf type. So, please name them something like ConvertFromFooProto() and ConvertToFooProto(). 2. Google C++ style: Input arguments go first, output arguments go last. 3. The "convert from" operations are taking data from an external source as input, and so they should return bools to indicate success/failure (more on this in rpc.cc file comments). On invalid or unparseable external source input, we should shut down media remoting.
PS8 was a great improvement, and I see you noticed some of the things I mentioned in my comments on PS7. We're getting close now: Just need to address the my comments for PS7.
On 2016/09/13 19:14:00, miu wrote: > PS8 was a great improvement, and I see you noticed some of the things I > mentioned in my comments on PS7. We're getting close now: Just need to address > the my comments for PS7. regarding to the type "string" or "bytes" in proto definition, following is what I found, and we may have to choose bytes over string due to JS string characteristics. (not sure if this is what you refer?) Here is the EME definition https://www.w3.org/TR/encrypted-media/#definitions in Session ID part, "....A Session ID is a unique string identifier generated by the CDM t". Since the spec is for JavaScript, we may need to change it to bytes, and same with other string field in the proto buffer. in_reply_to: 5711312218750976 send_mail: 1 subject: Define remote playback proto buffer
On 2016/09/14 17:46:26, erickung1 wrote: > On 2016/09/13 19:14:00, miu wrote: > > PS8 was a great improvement, and I see you noticed some of the things I > > mentioned in my comments on PS7. We're getting close now: Just need to address > > the my comments for PS7. > > regarding to the type "string" or "bytes" in proto definition, following is what > I found, > and we may have to choose bytes over string due to JS string characteristics. > (not sure if this is what you refer?) > > Here is the EME definition https://www.w3.org/TR/encrypted-media/#definitions > > in Session ID part, "....A Session ID is a unique string identifier generated by > the CDM t". Since the spec is for JavaScript, we may need to change it to bytes, > and same with other string field in the proto buffer. > > in_reply_to: 5711312218750976 > send_mail: 1 > subject: Define remote playback proto buffer Actually, if we don't care about the data, and only consider proto buffer itself Both bytes and string defined in .proto will be std::string in C++. I guess it doesn't really matter in char encoding?
On 2016/09/14 21:10:44, erickung1 wrote: > Both bytes and string defined in .proto will be std::string in C++. I guess it > doesn't really matter in char encoding? It may not matter on C++, but in other environments (such as JS, Java, Python), it could. The reason is that "string" asks the protobuf generator to account for the character encoding of the string and automatically convert to the platform's native type. For example, on C++ it might be UTF-8, but then it would be translated to UTF-16 in Java (as a java.lang.String). So, it matters to pay attention to this. The question here is: What data is going into this protobuf field? Is it text that might end up being parsed by the consumer, or maybe it needs to be human readable? Then "string." If it's supposed to be an opaque binary blob of bytes (e.g., a PNG image), then "bytes." AFAICT, from scanning the EME spec, a "session ID" is probably a "string." However, a "key" would probably be "bytes." A "key ID" should be "bytes" because the spec describes it as "a sequence of octets." Also, "initialization data" should definitely be "bytes." If you're uncertain about anything, you should probably look at the C++ code to determine which is appropriate.
Patch Set #9 to address all comments from Yuri except for "string" type concerns Patch Set #10 introduce enum conversion utils generated by Yuri and also merge all util function into single one(proto_utils.cc), same as unit test https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... File media/remoting/proto/remoting_rpc_message.proto (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/proto/re... media/remoting/proto/remoting_rpc_message.proto:480: string string_value = 7; On 2016/09/13 05:40:57, miu wrote: > On 2016/09/08 01:07:34, miu wrote: > > Should this be of type |string| or |bytes|? I'm thinking probably the latter, > to > > avoid char encoding interpretation? > > Could you please check this? Seems like it should be of type "bytes". Here is the EME definition https://www.w3.org/TR/encrypted-media/#definitions in Session ID part, "....A Session ID is a unique string identifier generated by the CDM t". Since the spec is for JavaScript, and it can be any type for string. so we may need to change it to bytes, and same with other field in the proto buffer. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... File media/remoting/rpc/decoder_buffer_segment.cc (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/deco... media/remoting/rpc/decoder_buffer_segment.cc:29: DCHECK(config_message.has_key_id()); On 2016/09/13 05:40:57, miu wrote: > On 2016/09/08 01:07:34, miu wrote: > > These DCHECKs suggest the fields should not have been optional in the proto. > > 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." Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.cc File media/remoting/rpc/rpc.cc (right): https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:27: #define RPC(x) pb::x On 2016/09/08 01:07:35, miu wrote: > IMHO, you shouldn't do this. I was not able to immediately understand what this > macro was doing in the code later in the file. It would have been much more > obvious to understand pb::RPC_FOO_BAR_BAZ instead of RPC(RPC_FOO_BAR_BAZ). Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:41: if (message.has_encrypt_blocks()) On 2016/09/08 01:07:35, miu wrote: > Throughout this file: The default value for integer-typed optional fields is > zero, and optional string-typed fields are empty strings. So, a lot of these > has_xxx() checks are just extra code that does nothing. > > Also, IIRC, the "getters" for protobuf messages are simple and in-lined, so > there's no need to copy them into local variables first. > > Putting it all together, for example, the code here could be more simply: > > REQUIRED(message.encrypt_blocks() >= 0); > REQUIRED(message.skip_blocks() >= 0); > > pattern = EncryptionScheme::Pattern(message.encrypt_blocks(), > message.skip_blocks()); Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:71: ::media::AudioCodec codec = ::media::kUnknownAudioCodec; On 2016/09/08 01:07:34, miu wrote: > General comment: Seems like a lot of these "converter" functions could be > simplified by eliminating all the local variables and just passing the > mapped/type-casted values directly to the media::XXXConfig::Initialize() method. > For example, this whole function could be: > > ... { > DCHECK(audio_config); > > audio_config->Initialize( > static_cast<::media::AudioCodec>(audio_message.codec()), > static_cast<::media::SampleFormat>(audio_message.sample_format()), > static_cast<::media::ChannelLayout>(audio_message.channel_layout()), > audio_message.samples_per_second(), > audio_message.extra_data(), > DeserializeEncryptionScheme(audio_message.encryption_scheme()), > base::TimeDelta::FromMicroseconds(audio_message.seek_preroll_usec()), > audio_message.codec_delay()); > return audio_config->IsValidConfig(); > } Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:102: return true; On 2016/09/08 01:07:34, miu wrote: > Suggest returning audio_config->IsValidConfig() here instead, and in > DeserializeVideoConfig(). Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:146: bool DeserializeCdmPromiseResult(const pb::CdmPromise& promise_message, On 2016/09/08 01:07:35, miu wrote: > General comment: Please make sure you validate the values that have been parsed, > and return false when they are illegal in these converter functions. For > example, this function always returns true (or if all possible field values are > legal, maybe it should have a void return type instead?). Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:170: std::vector<::media::CdmKeyInformation> info; On 2016/09/08 01:07:35, miu wrote: > nit: When populating vectors where you know their final size beforehand, please > call std::vector::reserve(). (Please check that you are doing this throughout > your change.) Done. the latest CL doesn't have such usage after the refactoring. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:193: pb::EncryptionScheme* message, On 2016/09/08 01:07:35, miu wrote: > style: Input arguments, then output arguments. Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:193: pb::EncryptionScheme* message, On 2016/09/08 01:07:35, miu wrote: > style: Input arguments, then output arguments. Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:207: void SerializeRect(pb::Rect* message, const gfx::Rect& rect) { On 2016/09/08 01:07:35, miu wrote: > here too: Input arguments, then output arguments. Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:220: decoder_config_message->set_extra_data(&config.extra_data()[0], On 2016/09/08 01:07:35, miu wrote: > nit: In C++11 code, you can now use config.extra_data().data() instead of > &config.extra_data()[0]. This seems to appear in a lot of places in this change, > so please search-and-replace throughout all the files. Done. https://codereview.chromium.org/2261503002/diff/80001/media/remoting/rpc/rpc.... media/remoting/rpc/rpc.cc:287: std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue()); On 2016/09/08 01:07:35, miu wrote: > Looks like this DictionaryValue is unused. Can you remove it? Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... File media/remoting/rpc/decoder_buffer_segment.cc (right): https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:28: std::unique_ptr<::media::DecryptConfig> DeserializeDecryptConfig( On 2016/09/13 05:40:57, miu wrote: > 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 Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:50: std::unique_ptr<::media::DecryptConfig> decrypt_config; On 2016/09/13 05:40:57, miu wrote: > Looks like this variable is unused. Otherwise, please declare it where it is > first used, to aid in code readability. 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 https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:91: if (has_discard) { On 2016/09/13 05:40:57, miu wrote: > |has_discard| seems to be true no matter what. Is that what you intended? Done. Yes, just noticed it and change the default value to false https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:110: void SerializeDecryptConfig(const ::media::DecryptConfig& decrypt_config, On 2016/09/13 05:40:57, miu wrote: > 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 Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:170: reader.ReadU32(&proto_size) && On 2016/09/13 05:40:57, miu wrote: > 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. Done. Yes, fixed in patch set#8 and also have this in the unit test https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:193: std::vector<uint8_t> buffer; On 2016/09/13 05:40:57, miu wrote: > 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); Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:197: decoder_buffer_message.ByteSize() + kDataBufferHeaderSize + On 2016/09/13 05:40:57, miu wrote: > 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. Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.cc:218: // Reset buffer since data is corrupted during serialization. On 2016/09/13 05:40:57, miu wrote: > s/data is corrupted during serialization/serialization of the data failed/ Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... File media/remoting/rpc/decoder_buffer_segment.h (right): https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/dec... media/remoting/rpc/decoder_buffer_segment.h:41: const scoped_refptr<::media::DecoderBuffer> decoder_buffer); On 2016/09/13 05:40:57, miu wrote: > nit: Please pass by const-ref: const ::media::DecoderBuffer& decoder_buffer. Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc.cc File media/remoting/rpc/rpc.cc (right): https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:39: if (message.has_encrypt_blocks()) On 2016/09/13 05:40:57, miu wrote: > Do we need "has" guards here? The default value will be zero, right? Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:43: DCHECK(encrypt_blocks >= 0 && skip_blocks >= 0); On 2016/09/13 05:40:57, miu wrote: > This will crash the process on bad external input. Instead, we should return > false when this happens and shut down media remoting. Done. Actually I just realized that both are unit32_t so it's always true. remove this DCHECK https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:51: if (!audio_config.IsValidConfig()) On 2016/09/13 05:40:57, miu wrote: > Since this is the sender generating an RPC message, trying to serialize an > invalid audio config would be a bug. So, this should be: > > DCHECK(audio_config.IsValidConfig()); Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:112: audio_config->Initialize(codec, sample_format, channel_layout, On 2016/09/13 05:40:58, miu wrote: > Please see my comments from last week about simplifying this function, and also > DeserializeVideoConfig() and DeserializeCdmPromiseResult(). Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:115: return true; On 2016/09/13 05:40:58, miu wrote: > Should be: return audio_config->IsValidConfig(); Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:153: return true; On 2016/09/13 05:40:57, miu wrote: > Should be: return video_config->IsValidConfig(); Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:179: if (!video_config.IsValidConfig()) On 2016/09/13 05:40:57, miu wrote: > ditto here: Should be DCHECK(...); Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:274: DCHECK(message.has_cdm_promise_rpc()); On 2016/09/13 05:40:57, miu wrote: > ditto: Return false instead. Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:277: DCHECK(DeserializeCdmPromiseResult(promise_message, result)); On 2016/09/13 05:40:57, miu wrote: > This function won't be called in release builds. Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.cc:291: if (!session_id.empty()) On 2016/09/13 05:40:57, miu wrote: > Should this be a DCHECK(). Or, is it valid to sometimes send this message > without a session_id? Done. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc.h File media/remoting/rpc/rpc.h (right): https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.h:29: extern const int kInvalidHandle; On 2016/09/13 05:40:58, miu wrote: > These don't seem to be used anywhere. Are you planning on using them in a future > change? I updated the code to use kReceiverHandle when RPC to acquire renderer and media_keys kInvalidHandle will be used by receiver. https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.h:32: void SerializeEncryptionScheme( On 2016/09/13 05:40:58, miu wrote: > General comment for these Serialize/Deserialize functions: > > 1. Naming nit: They aren't serializing/deserializing. They just convert between > the ::media::Object type and the protobuf type. So, please name them something > like ConvertFromFooProto() and ConvertToFooProto(). > > 2. Google C++ style: Input arguments go first, output arguments go last. > > 3. The "convert from" operations are taking data from an external source as > input, and so they should return bools to indicate success/failure (more on this > in rpc.cc file comments). On invalid or unparseable external source input, we > should shut down media remoting. I made the change for #1 #2 and most #3. When you say shutdown media remoting, do you mean DCHECK on debug build? or the normal route in the code to stop remoting??
On 2016/09/15 01:35:34, miu wrote: > On 2016/09/14 21:10:44, erickung1 wrote: > > Both bytes and string defined in .proto will be std::string in C++. I guess it > > doesn't really matter in char encoding? > > It may not matter on C++, but in other environments (such as JS, Java, Python), > it could. The reason is that "string" asks the protobuf generator to account for > the character encoding of the string and automatically convert to the platform's > native type. For example, on C++ it might be UTF-8, but then it would be > translated to UTF-16 in Java (as a java.lang.String). So, it matters to pay > attention to this. > > The question here is: What data is going into this protobuf field? Is it text > that might end up being parsed by the consumer, or maybe it needs to be human > readable? Then "string." If it's supposed to be an opaque binary blob of bytes > (e.g., a PNG image), then "bytes." > > AFAICT, from scanning the EME spec, a "session ID" is probably a "string." > However, a "key" would probably be "bytes." A "key ID" should be "bytes" because > the spec describes it as "a sequence of octets." Also, "initialization data" > should definitely be "bytes." If you're uncertain about anything, you should > probably look at the C++ code to determine which is appropriate. Thanks for the explanation. Yes, in the .proto file, only "session_id", "key system" (such as com.google.widevine as string), "security origin" (such as website url) use string type For "side data", "certificate data", "init data", "iv", "key id" are defined in bytes. I assume this should be sufficient enough??
Hi, the last CL patch #12 is to re-example all proto field and add missing enum type. This should be the final version of proto
On 2016/09/15 02:19:52, erickung1 wrote: > On 2016/09/15 01:35:34, miu wrote: > > On 2016/09/14 21:10:44, erickung1 wrote: > > > Both bytes and string defined in .proto will be std::string in C++. I guess > > Thanks for the explanation. Yes, in the .proto file, only "session_id", "key > system" > (such as com.google.widevine as string), "security origin" (such as website url) > use string type > > For "side data", "certificate data", "init data", "iv", "key id" are defined in > bytes. > > I assume this should be sufficient enough?? Sounds good to me. Now taking a look at PS12...
Mostly small stuff now, except we have concerns about the use of "frame_id" in the RPCs: https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc.h File media/remoting/rpc/rpc.h (right): https://codereview.chromium.org/2261503002/diff/120001/media/remoting/rpc/rpc... media/remoting/rpc/rpc.h:32: void SerializeEncryptionScheme( On 2016/09/15 02:13:33, erickung1 wrote: > > 3. The "convert from" operations are taking data from an external source as > > input, and so they should return bools to indicate success/failure (more on > > When you say shutdown media remoting, do you mean DCHECK on debug build? or the > normal route in the code to stop remoting?? Normal route. General guideline: If a value is not what it should be because there is a bug in the Chrome code, then crash (DCHECK). However, if the value is wrong because of a transient/environmental error, or because of a receiver-side bug, then just shut down remoting immediately (because it's not a Chrome code bug). https://codereview.chromium.org/2261503002/diff/220001/media/remoting/proto/r... File media/remoting/proto/remoting_rpc_message.proto (right): https://codereview.chromium.org/2261503002/diff/220001/media/remoting/proto/r... media/remoting/proto/remoting_rpc_message.proto:287: message RendererFlushUntil { Why would the receiver ever request a flush? If it is issued before a seek, as an optimization, then that would better be handled by the sender as part of the seek operation. I'm not sure, from looking at the RPC proto here, how seeking is supposed to work. We should discuss this in our next meeting, just to make sure we're on the same page. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/proto/r... media/remoting/proto/remoting_rpc_message.proto:288: optional uint32 audio_frame_id = 1; This came up in discussion with xjz@ yesterday: There shouldn't be any references to concepts related to the Cast Streaming protocol in these RPCs. That's crossing abstraction layers, and would be forcing the coupling of the remoting media stack with the Cast Streaming protocol. This is bad because a change to either could easily and unexpectedly break things. As for this LOC, the concept of a frame_id does not exist anywhere else in the Chrome media stack (e.g., try running: `cd src/media && git grep -iP 'frame.?id'` | grep -vP '^(capture|cast|gpu)/'). It seems that the frame timestamps are used for finding/synchronizing among individual frames. So, please go through and scrub the RPC stack to make sure only media stack concepts are being used. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/proto/r... media/remoting/proto/remoting_rpc_message.proto:305: optional uint32 frame_id = 2; Instead of frame_id, this could be: 1. read until "timestamp_usec" 2. read up to N additional bytes' worth of frames IMHO, #1 would probably work better because it is independent of the frame rate of the content, and probably would result in simpler code on both the receiver and sender side. Though, #2 could be better if the receiver has a rough idea of its target buffer fill (and capacity). https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... File media/remoting/rpc/proto_enum_utils.h (right): https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_enum_utils.h:92: base::Optional<::media::EmeInitDataType> ToMediaEmeIniDataType( s/ToMediaEmeIniDataType/ToMediaEmeInitDataType/ ^ https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... File media/remoting/rpc/proto_utils.cc (right): https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils.cc:24: const int kPayloadVersionFieldSize = 1; For these three constants: 1. s/const/constexpr/ 2. s/int/size_t/ 3. Use self-documenting values on the RHS. Meaning: sizeof(uint8_t) instead of 1, sizeof(uint16_t) instead of 2, etc. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils.cc:30: std::vector<::media::SubsampleEntry> entries; nit: Please move declaration down to where this is first used. Also, call entries.reserve(config_message.sub_samples_size()) since the final size of the vector is known ahead of time. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils.cc:372: for (int i = 0; i < keychange_message.key_information_size(); ++i) { nit: Please add: key_information->reserve(keychange_message.key_information_size()); https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils.cc:400: if (!session_id.empty()) Is an empty |session_id| valid? If that would indicate a bug in Chrome code, please use DCHECK(!session_id.empty()) instead. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... File media/remoting/rpc/proto_utils.h (right): https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils.h:30: extern const int kInvalidHandle; C++11 provides a cleaner solution to things like this: constexpr int kInvalidHandle = -1; constexpr int kReceiverHandle = 0; https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils.h:34: // byte array. The idea is to serialize media::DecoderBuffer except for actual The second sentence here is a bit confusing. I think you meant to say something like: "pb::DecoderBuffer carries all the fields of media::DecoderBuffer except for |data|. The code here (de)serializes both together." OOC, why doesn't pb::DecoderBuffer just have the |data| field also? (Please answer in code comments.) https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... File media/remoting/rpc/proto_utils_unittest.cc (right): https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils_unittest.cc:103: TEST_F(ProtoUtilsTest, AudioDecoderCondigConversionTest) { s/Condig/Config/ https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils_unittest.cc:105: const EncryptionScheme encrypteion_scheme( s/encrypteion_scheme/encryption_scheme/ ^
The current AV transmission design concept is to have a place to control the AV data flow - Cast streaming sender only queue limited data waiting for transmission by using ReadUntil RPC. Therefore, all data are stored in chrome chunk demuxer - We have MediaBaseRunner to balance sending AV data to make sure not sending too much for one stream. This runner and the counter are meant to maintain the flow control and avoid running out of memory(on both side) - We would consider frame id as counter due to the fact that RPC and AV data are sent in different protocol, which is not serialized in the queue. frame data sent earlier than RPC message may be delivered to receiver in different order. - Reason not to use timestamp is because video's PTS is not incremental, and we don't have DTS information from demuxer output. I'm thinking of some other approach. For seeking, we can add RTCP message in the new frame indicating disgarding earlier data. Once receiver receives Flush RPC, it will drop all incoming frames until the frame with such RTCP message is received. for Readuntil, unless we can make sure cast sender is capable of queuing lots of data(responsible for flow control), we still need to have counter-like idea. using byte counts may be OK, but the calculation on both side will be more complicated than using frame count, given the fact that we already have frame count concept in RTP packet. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/proto/r... File media/remoting/proto/remoting_rpc_message.proto (right): https://codereview.chromium.org/2261503002/diff/220001/media/remoting/proto/r... media/remoting/proto/remoting_rpc_message.proto:287: message RendererFlushUntil { On 2016/09/16 18:14:13, miu wrote: > Why would the receiver ever request a flush? If it is issued before a seek, as > an optimization, then that would better be handled by the sender as part of the > seek operation. > > I'm not sure, from looking at the RPC proto here, how seeking is supposed to > work. We should discuss this in our next meeting, just to make sure we're on the > same page. this is the message sender sends to receiver, to indicates flushing those pending data(which may be in the air due to the fact that RPC message may arrive earlier than AV data through UDP, even RPC is sent later than AV data. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/proto/r... media/remoting/proto/remoting_rpc_message.proto:305: optional uint32 frame_id = 2; On 2016/09/16 18:14:13, miu wrote: > Instead of frame_id, this could be: > > 1. read until "timestamp_usec" > 2. read up to N additional bytes' worth of frames > > IMHO, #1 would probably work better because it is independent of the frame rate > of the content, and probably would result in simpler code on both the receiver > and sender side. Though, #2 could be better if the receiver has a rough idea of > its target buffer fill (and capacity). #2 is not ideal because it depends on the bit rate of the stream #1 works only for audio, but not for video because PTS can jump due to B frame(not like mirroring, that's the reason the timestamp in RTP is incremental and meaningless). Also on render process side we don't have DTS information in the renderer. I feel like the reason we kind of need a counter concept(or frame id) is because we use two different protocol to send data over, which is different from existing media pipeline(that everything runs in the single thread)
https://codereview.chromium.org/2261503002/diff/220001/media/remoting/proto/r... File media/remoting/proto/remoting_rpc_message.proto (right): https://codereview.chromium.org/2261503002/diff/220001/media/remoting/proto/r... media/remoting/proto/remoting_rpc_message.proto:287: message RendererFlushUntil { On 2016/09/16 18:14:13, miu wrote: > Why would the receiver ever request a flush? If it is issued before a seek, as > an optimization, then that would better be handled by the sender as part of the > seek operation. > > I'm not sure, from looking at the RPC proto here, how seeking is supposed to > work. We should discuss this in our next meeting, just to make sure we're on the > same page. This is the RPC message sent from sender to receiver. This is to tell receiver that there is flush operation and please discard the pending data in the pipeline so that seeking can be done/resume from the new position asap. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/proto/r... media/remoting/proto/remoting_rpc_message.proto:288: optional uint32 audio_frame_id = 1; On 2016/09/16 18:14:13, miu wrote: > This came up in discussion with xjz@ yesterday: There shouldn't be any > references to concepts related to the Cast Streaming protocol in these RPCs. > That's crossing abstraction layers, and would be forcing the coupling of the > remoting media stack with the Cast Streaming protocol. This is bad because a > change to either could easily and unexpectedly break things. > > As for this LOC, the concept of a frame_id does not exist anywhere else in the > Chrome media stack (e.g., try running: `cd src/media && git grep -iP > 'frame.?id'` | grep -vP '^(capture|cast|gpu)/'). It seems that the frame > timestamps are used for finding/synchronizing among individual frames. > > So, please go through and scrub the RPC stack to make sure only media stack > concepts are being used. (offline discussion is done) After discussion, we will keep this field but rename it to count since this is to allow receiver to know which packets needs to be expected as the first new packet after the flush/seek operation. The reason is because the RPC and data streaming can be done in different pipe/protocol, so the data delivered to receiver may not in the right order as is sends out from the sender. having a name/indication is necessary. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/proto/r... media/remoting/proto/remoting_rpc_message.proto:305: optional uint32 frame_id = 2; On 2016/09/16 21:18:38, erickung1 wrote: > On 2016/09/16 18:14:13, miu wrote: > > Instead of frame_id, this could be: > > > > 1. read until "timestamp_usec" > > 2. read up to N additional bytes' worth of frames > > > > IMHO, #1 would probably work better because it is independent of the frame > rate > > of the content, and probably would result in simpler code on both the receiver > > and sender side. Though, #2 could be better if the receiver has a rough idea > of > > its target buffer fill (and capacity). > > #2 is not ideal because it depends on the bit rate of the stream > #1 works only for audio, but not for video because PTS can jump due to B > frame(not like mirroring, that's the reason the timestamp in RTP is incremental > and meaningless). Also on render process side we don't have DTS information in > the renderer. > > I feel like the reason we kind of need a counter concept(or frame id) is because > we use two different protocol to send data over, which is different from > existing media pipeline(that everything runs in the single thread) > after discussion, we change the name to count https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... File media/remoting/rpc/proto_enum_utils.h (right): https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_enum_utils.h:92: base::Optional<::media::EmeInitDataType> ToMediaEmeIniDataType( On 2016/09/16 18:14:13, miu wrote: > s/ToMediaEmeIniDataType/ToMediaEmeInitDataType/ > ^ Done. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... File media/remoting/rpc/proto_utils.cc (right): https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils.cc:24: const int kPayloadVersionFieldSize = 1; On 2016/09/16 18:14:13, miu wrote: > For these three constants: > > 1. s/const/constexpr/ > 2. s/int/size_t/ > 3. Use self-documenting values on the RHS. Meaning: sizeof(uint8_t) instead of > 1, sizeof(uint16_t) instead of 2, etc. Done. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils.cc:30: std::vector<::media::SubsampleEntry> entries; On 2016/09/16 18:14:13, miu wrote: > nit: Please move declaration down to where this is first used. Also, call > entries.reserve(config_message.sub_samples_size()) since the final size of the > vector is known ahead of time. Done. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils.cc:372: for (int i = 0; i < keychange_message.key_information_size(); ++i) { On 2016/09/16 18:14:13, miu wrote: > nit: Please add: > key_information->reserve(keychange_message.key_information_size()); Done. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils.cc:400: if (!session_id.empty()) On 2016/09/16 18:14:13, miu wrote: > Is an empty |session_id| valid? If that would indicate a bug in Chrome code, > please use DCHECK(!session_id.empty()) instead. Done. I decided to remove the if condition, since it is still OK to set empty string session id The reason is because there are two types of promise, NewSessionCdmPromise and SimpleCdmPromise from the proto design's perspective, we create a promise proto structure and add "session_id" field as optional. That's the reason some RPC message is required to set it and some ore not. https://cs.chromium.org/chromium/src/media/base/media_keys.h?q=media_keys.h&s... Only when LoadSession() and CreateSessionAndGenerateRequest() would require using Promise with session id https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... File media/remoting/rpc/proto_utils.h (right): https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils.h:30: extern const int kInvalidHandle; On 2016/09/16 18:14:13, miu wrote: > C++11 provides a cleaner solution to things like this: > > constexpr int kInvalidHandle = -1; > constexpr int kReceiverHandle = 0; Done. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils.h:34: // byte array. The idea is to serialize media::DecoderBuffer except for actual On 2016/09/16 18:14:14, miu wrote: > The second sentence here is a bit confusing. I think you meant to say something > like: "pb::DecoderBuffer carries all the fields of media::DecoderBuffer except > for |data|. The code here (de)serializes both together." > > OOC, why doesn't pb::DecoderBuffer just have the |data| field also? (Please > answer in code comments.) Done. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... File media/remoting/rpc/proto_utils_unittest.cc (right): https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils_unittest.cc:103: TEST_F(ProtoUtilsTest, AudioDecoderCondigConversionTest) { On 2016/09/16 18:14:14, miu wrote: > s/Condig/Config/ Done. https://codereview.chromium.org/2261503002/diff/220001/media/remoting/rpc/pro... media/remoting/rpc/proto_utils_unittest.cc:105: const EncryptionScheme encrypteion_scheme( On 2016/09/16 18:14:14, miu wrote: > s/encrypteion_scheme/encryption_scheme/ > ^ Done.
Could you please add some documentation (e.g. a README in media/remoting) describing what's the purpose of this new folder? It seems "remoting" has been heavily used in the codebase exclusively for "chromoting"; some documentation would help avoid confusion. Also, this makes me wonder whether we should use the name "remoting", how about just "remote"? This is not a big deal though and I'll leave the decision to the new owners of this folder.
On 2016/09/19 18:11:52, xhwang (slow) wrote: > Could you please add some documentation (e.g. a README in media/remoting) > describing what's the purpose of this new folder? It seems "remoting" has been > heavily used in the codebase exclusively for "chromoting"; some documentation > would help avoid confusion. > > Also, this makes me wonder whether we should use the name "remoting", how about > just "remote"? This is not a big deal though and I'll leave the decision to the > new owners of this folder. Add README with a brief description about what is this folder for. After the discussion with miu@, we will keep the name as is for now and consider change later on because perhaps both media remoting and chromeoting should use specific name to avoid confusion.
lgtm! :)
One question on the deps change. https://chromiumcodereview.appspot.com/2261503002/diff/260001/media/BUILD.gn File media/BUILD.gn (right): https://chromiumcodereview.appspot.com/2261503002/diff/260001/media/BUILD.gn#... media/BUILD.gn:483: deps += [ "//media/remoting:rpc" ] hmm, why should media depend on media/remoting? I thought it should be the other way around?
https://chromiumcodereview.appspot.com/2261503002/diff/260001/media/BUILD.gn File media/BUILD.gn (right): https://chromiumcodereview.appspot.com/2261503002/diff/260001/media/BUILD.gn#... media/BUILD.gn:483: deps += [ "//media/remoting:rpc" ] On 2016/09/22 23:55:19, xhwang (slow) wrote: > hmm, why should media depend on media/remoting? I thought it should be the other > way around? Yes, after the discussion, I removed all the change here. media/remoting will be acomponent that media/blink will be dependent on once all other part of CLs are merged.
Thanks! Now you don't need media/OWNERS' review :) Feel free to land it since you have miu's approval.
Description was changed from ========== Define remote playback proto buffer This allows us to send RPC message to remote device for content playback. Please refer full WIP CL (https://codereview.chromium.org/2089133003) BUG= ========== to ========== Define remote playback proto buffer This allows us to send RPC message to remote device for content playback. ==========
The CQ bit was checked by erickung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2261503002/#ps300001 (title: "Remove all change in src/BUILD.gn and media_options.gni")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Define remote playback proto buffer This allows us to send RPC message to remote device for content playback. ========== to ========== Define remote playback proto buffer This allows us to send RPC message to remote device for content playback. ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Define remote playback proto buffer This allows us to send RPC message to remote device for content playback. ========== to ========== Define remote playback proto buffer This allows us to send RPC message to remote device for content playback. Committed: https://crrev.com/1fc58a4b0bd5d0a58ce8a353a0c7840cd6561f75 Cr-Commit-Position: refs/heads/master@{#420821} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/1fc58a4b0bd5d0a58ce8a353a0c7840cd6561f75 Cr-Commit-Position: refs/heads/master@{#420821} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
