Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2310753002:
Media Remoting: Data/Control plumbing between renderer and Media Router. (Closed)
|
Created:
4 years, 3 months ago by miu Modified:
4 years, 2 months ago Reviewers:
Ken Rockot(use gerrit already), erickung1, xjz, mark a. foltz, apacible, dcheng, DaleCurtis, nasko, xhwang CC:
Aaron Boodman, abarth-chromium, alokp+watch_chromium.org, avayvod+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, isheriff+watch_chromium.org, jam, jasonroberts+watch_google.com, mcasas+watch+vc_chromium.org, media-router+watch_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, xjz+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia Remoting: Data/Control plumbing between renderer and Media Router.
Introduces the new media::mojom::Remoting* interfaces, which provide the
control and data flows across browser processes for a new media remoting
feature. These interfaces allow a renderer to connect the local media
pipeline to a remote media rendering stack.
Also, a Cast-specific implementation of these interfaces is provided,
which allows media remoting sessions to be established within the
context of a Cast Tab Mirroring session. This implementation uses the
Chrome Media Router infrastructure to perform the discovery and
selection of remoting-capable devices, establishing media routes for
this new feature.
This change provides one major component, among others, for a complete
end-to-end media remoting feature. By itself, no new code paths are
activated yet. Future code changes will provide integration with other
efforts, such as:
https://codereview.chromium.org/2307653002/
https://codereview.chromium.org/2261503002/
https://codereview.chromium.org/2342163003/
...and the Media Router component extension.
BUG=643964
Committed: https://crrev.com/1ddf18c6f77565d5e80beecaf965b73bb035a913
Cr-Commit-Position: refs/heads/master@{#420812}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Updated/Moved TODO comment in render_frame_impl.cc. And REBASE. #
Total comments: 6
Patch Set 3 : Comment tweaks, per comments from rockot@ and apacible@. #Patch Set 4 : REBASE. Plus, merge with CastRemotingSender and add more unit tests. #
Total comments: 9
Patch Set 5 : xjz's PS4 comments addressed. #
Total comments: 42
Patch Set 6 : dcheng's comments on PS5 addressed #Patch Set 7 : Add input operation queuing to resolve race between data and message pipes. #
Total comments: 2
Patch Set 8 : Fix minor typos. #Patch Set 9 : Just a REBASE on ToT before commit. #
Total comments: 1
Messages
Total messages: 61 (32 generated)
Brought things over from (https://codereview.chromium.org/2189533002/), and expanded the change to provide more context for how the new remoting Mojo interfaces will be used. Also, diagrams, overview, and more detailed design can be found in the design doc (linked from crbug). The unit tests in chrome/browser/media/cast_remoting_connector_unittest.cc are a good reference to see how a RemotingSource and Remoter will be used to establish a media remoting session, and how the local media pipeline and remote device will pass control messages and data to each other. rockot and dalecurtis: PTAL at media/mojo/interfaces/* (now *without* IDs!). apacible and xjz: PTAL at chrome/browser/media/* erickung and xhwang: FYI, but comments are welcome, of course! Thanks in advance! :)
The CQ bit was checked by miu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Looks reasonable to me, defer to rockot@ for design review.
mfoltz@chromium.org changed reviewers: + mfoltz@chromium.org
Will look if I have time (don't block on my review).
LGTM https://codereview.chromium.org/2310753002/diff/1/chrome/browser/media/cast_r... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2310753002/diff/1/chrome/browser/media/cast_r... chrome/browser/media/cast_remoting_connector.cc:492: notifyee->OnSinkAvailable(); Do we need check |message_observer_| before sending OnSinkAvailable()? https://codereview.chromium.org/2310753002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2310753002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2639: // media::Renderers. This seems different from our current design. But we'll find it out later. :)
The CQ bit was checked by miu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2310753002/diff/1/chrome/browser/media/cast_r... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2310753002/diff/1/chrome/browser/media/cast_r... chrome/browser/media/cast_remoting_connector.cc:492: notifyee->OnSinkAvailable(); On 2016/09/09 17:38:48, xjz wrote: > Do we need check |message_observer_| before sending OnSinkAvailable()? No, because this method is being called from that very object! :) https://codereview.chromium.org/2310753002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2310753002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2639: // media::Renderers. On 2016/09/09 17:38:48, xjz wrote: > This seems different from our current design. But we'll find it out later. :) Updated and moved this TODO comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
interfaces lgtm, just some minor documentation nits https://codereview.chromium.org/2310753002/diff/20001/media/mojo/interfaces/r... File media/mojo/interfaces/remoting.mojom (right): https://codereview.chromium.org/2310753002/diff/20001/media/mojo/interfaces/r... media/mojo/interfaces/remoting.mojom:17: // Consumes |size| bytes of data from the data pipe, which is a chunk of the doc nit: From which data pipe? https://codereview.chromium.org/2310753002/diff/20001/media/mojo/interfaces/r... media/mojo/interfaces/remoting.mojom:35: // including the last SendFrame() call; and also discard any data chunks doc nit: Is this an accurate guarantee? Can't a call to CancelInFlightData() race with transmission of in-flight data?
lgtm https://codereview.chromium.org/2310753002/diff/20001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2310753002/diff/20001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:436: case media_router::RouteMessage::TEXT: // This is a control message. What does "control message" mean?
https://codereview.chromium.org/2310753002/diff/20001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2310753002/diff/20001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:436: case media_router::RouteMessage::TEXT: // This is a control message. On 2016/09/12 20:57:59, apacible wrote: > What does "control message" mean? This is explained in the header comments for this class, where the interactions between CastRemotingConnector and the Cast Provider is discussed. I made some changes in the .h and .cc files to clarify/elaborate a bit more. https://codereview.chromium.org/2310753002/diff/20001/media/mojo/interfaces/r... File media/mojo/interfaces/remoting.mojom (right): https://codereview.chromium.org/2310753002/diff/20001/media/mojo/interfaces/r... media/mojo/interfaces/remoting.mojom:17: // Consumes |size| bytes of data from the data pipe, which is a chunk of the On 2016/09/12 15:26:05, Ken Rockot wrote: > doc nit: From which data pipe? The one and only. ;-) So, my comments for the whole RemotingDataStreamSender interface made it look like one of these controlled both data pipes. Instead, there is one RDSSender per data pipe. I fixed the comment to clarify this. https://codereview.chromium.org/2310753002/diff/20001/media/mojo/interfaces/r... media/mojo/interfaces/remoting.mojom:35: // including the last SendFrame() call; and also discard any data chunks On 2016/09/12 15:26:05, Ken Rockot wrote: > doc nit: Is this an accurate guarantee? Can't a call to CancelInFlightData() > race with transmission of in-flight data? Yes. Cancel in this context means "I decided not to send the data after-the-fact, so just don't send any more data than you already have." I added a sentence to explain that there is no guarantee as to how much of the in-flight data will reach the remote.
Patchset #4 (id:60001) has been deleted
miu@chromium.org changed reviewers: + nasko@chromium.org
nasko: OWNERS RS for content/renderer/render_frame_impl.cc (the factory is to be used in follow-up change currently being worked on by a teammate). xjz: PTAL at chrome/browser/media/cast_remoting_sender* (after merging with files from your recently-landed change).
https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_sender.cc (right): https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_sender.cc:359: DCHECK(cast_environment_->CurrentlyOn(media::cast::CastEnvironment::MAIN)); nit: DCHECK_NE(flow_control_state_, CONSUME_PAUSED); https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_sender.cc:397: : media::cast::EncodedFrame::DEPENDENT; When |is_first_frame_to_be_send| is true, |flow_control_state_| seems always be RESTARTING. So maybe: remoting_frame.dependency = (flow_control_state_ == RESTARTING ? ? media::cast::EncodedFrame::KEY : media::cast::EncodedFrame::DEPENDENT); flow_control_state_ = CONSUMING; https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_sender.cc:454: I might miss one thing here. We need to increase the |last_sent_frame_id_| by one if there is one frame stored in |next_frame_data_|. And when this is called, is it possible that there are still some frames stored in Mojo data pipeline? If yes, we may need to increase |last_sent_frame_id_| by that number too, since we need to match the frame_id in renderer. As we discussed before, another solution to prevent this mismatching is to pass the ID from renderer in this function... https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_sender.h (right): https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_sender.h:156: RESTARTING, // Starting new, or restarting after CancelInFlightData(). nit: How about just STARTING? Actually this status seems indicating both starting and restarting. So maybe either one is good. https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_sender_unittest.cc (right): https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_sender_unittest.cc:3: // found in the LICENSE file. Great job! Awesome tests!!! :)
This CL requires an IPC security review, but I cannot do it today/tomorrow. The content/ part looks fine, but if I stamp the CL, it will be good for IPC security too, so I'll hold off on stamping it until that is complete. If you want faster turnaround on IPC security review, pick someone from ipc/SECURITY_OWNERS.
miu@chromium.org changed reviewers: + dcheng@chromium.org
dcheng: Understand you are very busy/backlogged. nasko@ noted I needed a security owners review for this change, and you have all the context from xjz's recent change (CastRemotingSender). So, PTAL for security review. Or, feel free to suggest someone else. ;-)
The CQ bit was checked by miu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the re-review, xjz. Responded to all comments from PS4: https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_sender.cc (right): https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_sender.cc:359: DCHECK(cast_environment_->CurrentlyOn(media::cast::CastEnvironment::MAIN)); On 2016/09/15 18:01:49, xjz wrote: > nit: DCHECK_NE(flow_control_state_, CONSUME_PAUSED); Done. https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_sender.cc:397: : media::cast::EncodedFrame::DEPENDENT; On 2016/09/15 18:01:49, xjz wrote: > When |is_first_frame_to_be_send| is true, |flow_control_state_| seems always be > RESTARTING. So maybe: > remoting_frame.dependency = (flow_control_state_ == RESTARTING ? > ? media::cast::EncodedFrame::KEY > : media::cast::EncodedFrame::DEPENDENT); > flow_control_state_ = CONSUMING; Good point. Simplified this a bit, but not exactly as you suggested. PTAL. https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_sender.cc:454: On 2016/09/15 18:01:49, xjz wrote: > I might miss one thing here. We need to increase the |last_sent_frame_id_| by > one if there is one frame stored in |next_frame_data_|. And when this is called, > is it possible that there are still some frames stored in Mojo data pipeline? If > yes, we may need to increase |last_sent_frame_id_| by that number too, since we > need to match the frame_id in renderer. As we discussed before, another solution > to prevent this mismatching is to pass the ID from renderer in this function... As discussed, we shouldn't be crossing abstraction layers (i.e., protocol frame_ids should not be visible to the application layer). However, in discussing this, we realized we can't just blanketly call CancelSendingFrames() if it's possible any packet made it to the network already. So, for now, I'm going to comment-out this code with a TODO and file a crbug (http://crbug.com/647423) for follow-up. Basically, it's okay to not cancel frames from the transport. However, there is likely to be a large performance gain from adding an optimization to do so when it doesn't break the protocol design constraints. https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_sender.h (right): https://codereview.chromium.org/2310753002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_sender.h:156: RESTARTING, // Starting new, or restarting after CancelInFlightData(). On 2016/09/15 18:01:49, xjz wrote: > nit: How about just STARTING? Actually this status seems indicating both > starting and restarting. So maybe either one is good. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:85: int32_t GetStreamIdFromStartedMessage(const std::string& message, Nit: StringPiece here will net slightly more efficient code (the substr() on line 96 won't have to create an actual new string object) https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:371: pending_audio_sender_request_.PassMessagePipe().reset(); Would it make sense to call ResetWithReason here? https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:427: const std::vector<media_router::RouteMessage>& messages) { For fuzzing, I think this is one of the interfaces we'll want to fuzz: we'll want to feed in RouteMessages with random content (I /think/, but I don't remember off the top of my head, if we validate that the type and message contents match somewhere upstream when we turn the mojom into a C++. Incidentally a StructTrait would be really good for this =) https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:436: DCHECK(message.text); Not for this CL, but we should consider making the mojom definition for this a union (then we can avoid the enum doesn't match the data type problem) https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:449: sizeof(kStartedStreamsMessageAudioIdSpecifier) - 1), Nit: use strlen here and below, our compilers should all be evaluating this at compile-time in release builds. Or just use StringPiece in GetStreamIdFromStartedMessage for both args. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.h (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:46: // CastRemotingConnector must mitigate among simultaneous requests for media mitigate => mediate? https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:57: // it can look-up and pass the mojo data pipe handles to CastRemotingSenders, CastRemotingSender? presumably there's only one, since only one remoting source can actually remote media at a time? https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:66: : protected content::WebContentsUserData<CastRemotingConnector>, All inheritance should be public: https://google.github.io/styleguide/cppguide.html#Inheritance Otherwise, just make it another class and make it a member of this one. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:91: // WebContents and RenderFrameHost) to work with. Does TestWebContents help this? I think it'd allow consolidation of the dual constructors, which could be nice? https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:165: // shutdown an active remoting session. Nit: document |ignored| here too? https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_sender.cc (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:155: base::Unretained(&sender->error_callback_))); Does set_connection_error(sender->error_callback_) work? https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:323: DCHECK_NE(flow_control_state_, CONSUME_PAUSED); why is that? What stops these messages from being forwarded when we're in CONSUME_PAUSED? https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:336: LOG(DFATAL) We probably shouldn't dcheck here, this is coming from the source, which (presumably) usually lives in a renderer process. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:346: if (result != MOJO_RESULT_OK) { Is it possible for MOJO_RESULT_OUT_OF_RANGE to be a valid return value here (e.g. no data is read because the data is not all in the pipe yet) here? https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_sender_unittest.cc (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender_unittest.cc:196: sender_->ConsumeDataChunk(offset, size, total_payload_size); It might make things slightly easier to just make sender_ protected: and have things call this directly? I'm not quite sure I understand the significance of these helpers otherwise. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender_unittest.cc:588: if ((i >= static_cast<size_t>(media::cast::kMaxUnackedFrames + 2)) && These casts might be avoidable by writing media::cast::kMaxUnackedFrames + 2U https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/route_message.cc (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/route_message.cc:19: return "illegal value"; We might even just DCHECK in this case... we should have sanitized these values earlier upstream (if they're not sanitized at this point, that's a bad sign) https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/route_message.cc:28: base::EscapeJSONString(src, true, &result); This assumes UTF8 input and will happily mangle the input if that's not the case. Is that OK here?
The CQ bit was checked by miu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dcheng: Addressed all your comments, and uploaded changes as PS6. PTAL. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:85: int32_t GetStreamIdFromStartedMessage(const std::string& message, On 2016/09/20 08:20:12, dcheng wrote: > Nit: StringPiece here will net slightly more efficient code (the substr() on > line 96 won't have to create an actual new string object) Done. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:371: pending_audio_sender_request_.PassMessagePipe().reset(); On 2016/09/20 08:20:12, dcheng wrote: > Would it make sense to call ResetWithReason here? This is brand-new, so I hadn't considered it. Having looked at it, I think it is not needed by our use case: 1. We already make Mojo calls back to the renderer to indicate the reasons for shutting down (media.mojom.RemotingStopReason). 2. From a structured data POV, I kind of like how my Mojo interface defines an enumeration of reasons for shutting down. The ResetWithReason() just provides a uint32_t, but no decent way to check that each end of the pipe is "speaking the same language" w.r.t. the values. (BTW--I added a comment on http://crbug.com/634502 to discuss this.) That said, I did make a small change here: There seems to be a new assign nullptr assignment operator, which I feel makes the code look cleaner. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:427: const std::vector<media_router::RouteMessage>& messages) { On 2016/09/20 08:20:12, dcheng wrote: > For fuzzing, I think this is one of the interfaces we'll want to fuzz: we'll > want to feed in RouteMessages with random content (I /think/, but I don't > remember off the top of my head, if we validate that the type and message > contents match somewhere upstream when we turn the mojom into a C++. > Incidentally a StructTrait would be really good for this =) Acknowledged. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:436: DCHECK(message.text); On 2016/09/20 08:20:12, dcheng wrote: > Not for this CL, but we should consider making the mojom definition for this a > union (then we can avoid the enum doesn't match the data type problem) Agreed. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:449: sizeof(kStartedStreamsMessageAudioIdSpecifier) - 1), On 2016/09/20 08:20:12, dcheng wrote: > Nit: use strlen here and below, our compilers should all be evaluating this at > compile-time in release builds. > > Or just use StringPiece in GetStreamIdFromStartedMessage for both args. Done. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.h (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:46: // CastRemotingConnector must mitigate among simultaneous requests for media On 2016/09/20 08:20:12, dcheng wrote: > mitigate => mediate? Done. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:57: // it can look-up and pass the mojo data pipe handles to CastRemotingSenders, On 2016/09/20 08:20:12, dcheng wrote: > CastRemotingSender? presumably there's only one, since only one remoting source > can actually remote media at a time? There is one source consisting of one audio track and one video track. So, there are two data pipes and two CastRemotingSenders (each read from one data pipe and send out one RTP stream over the network): audio data --> audio data pipe --> audio CastRemotingSender --> audio RTP stream video data --> video data pipe --> video CastRemotingSender --> video RTP stream https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:66: : protected content::WebContentsUserData<CastRemotingConnector>, On 2016/09/20 08:20:12, dcheng wrote: > All inheritance should be public: > https://google.github.io/styleguide/cppguide.html#Inheritance > > Otherwise, just make it another class and make it a member of this one. Done. Changed to inherit from base::SupportsUserData::Data instead. My intention was to hide the static Create/From() methods defined in WebContentsUserData since I only want to expose a Get() in the public interface of this class. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:91: // WebContents and RenderFrameHost) to work with. On 2016/09/20 08:20:12, dcheng wrote: > Does TestWebContents help this? I think it'd allow consolidation of the dual > constructors, which could be nice? No longer an issue (no longer using WebContentsUserData, and only one ctor now). https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:165: // shutdown an active remoting session. On 2016/09/20 08:20:12, dcheng wrote: > Nit: document |ignored| here too? The 2nd arg is described in the interface. It is not being used by the override impl. I'm not sure what there is to document. ;-) https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_sender.cc (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:155: base::Unretained(&sender->error_callback_))); On 2016/09/20 08:20:12, dcheng wrote: > Does set_connection_error(sender->error_callback_) work? Whoops! Left-overs, ya know? Done. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:323: DCHECK_NE(flow_control_state_, CONSUME_PAUSED); On 2016/09/20 08:20:12, dcheng wrote: > why is that? What stops these messages from being forwarded when we're in > CONSUME_PAUSED? Short answer: We call binding_.PauseIncomingMethodCallProcessing(). This mechanism is comprehensively unit tested (CastRemotingSenderTest.ManagesFlowControl). Long answer: We transition into the CONSUME_PAUSED state when we've run out of buffer capacity in the network transport. In this state, CastRemotingSender must stop consuming from the mojo data pipe because there is nowhere for the data to go. However, the other end (in the render process) will continue to push more data into the data pipe, and then make calls on the interface to instruct CastRemotingSender to pull the data out from the pipe. Therefore, when we transition into the CONSUME_PAUSED state, we also tell Mojo to pause incoming method calls. This is a super-convenient Mojo feature: CastRemotingSender isn't being told to consume from the data pipe and so the data will stay there until we are ready for it. Once some data has been sent over the network, and we have room again in the network transport buffers, we tell Mojo to resume incoming method calls. This will cause CastRemotingSender to resume pulling data out of the data pipe again. One other concern is whether there is an upper-bound to how many mojo method calls will be queued-up. There is! It is bound because the data pipe will eventually fill to capacity. Once the render process cannot push more data into the data pipe, it will also stop making calls on the interface to have CastRemotingSender try to consume from the data pipe. It is simply forced to wait until it sees the data pipe start draining again. All in all, a pretty natural way to handle flow control. :-) https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:336: LOG(DFATAL) On 2016/09/20 08:20:12, dcheng wrote: > We probably shouldn't dcheck here, this is coming from the source, which > (presumably) usually lives in a renderer process. Relaxed to ERROR level. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:346: if (result != MOJO_RESULT_OK) { On 2016/09/20 08:20:12, dcheng wrote: > Is it possible for MOJO_RESULT_OUT_OF_RANGE to be a valid return value here > (e.g. no data is read because the data is not all in the pipe yet) here? This should never happen. The render process is always supposed to push data into the data pipe before calling the interface methods that would have CastRemotingSender pull from the data pipe. So, the data better be there. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_sender_unittest.cc (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender_unittest.cc:196: sender_->ConsumeDataChunk(offset, size, total_payload_size); On 2016/09/20 08:20:13, dcheng wrote: > It might make things slightly easier to just make sender_ protected: and have > things call this directly? I'm not quite sure I understand the significance of > these helpers otherwise. Required by the style guide: https://google.github.io/styleguide/cppguide.html#Inheritance (note in the "Decision" subsection that it explicitly states data members must be private). Another reason was in the naming here: I wanted to call-out in the test code that we are only "posting tasks" that make mojo method calls, which means the task runner has to be spun up for those tasks to actually execute. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender_unittest.cc:588: if ((i >= static_cast<size_t>(media::cast::kMaxUnackedFrames + 2)) && On 2016/09/20 08:20:13, dcheng wrote: > These casts might be avoidable by writing media::cast::kMaxUnackedFrames + 2U Done. Good idea. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/route_message.cc (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/route_message.cc:19: return "illegal value"; On 2016/09/20 08:20:13, dcheng wrote: > We might even just DCHECK in this case... we should have sanitized these values > earlier upstream (if they're not sanitized at this point, that's a bad sign) Done. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/route_message.cc:28: base::EscapeJSONString(src, true, &result); On 2016/09/20 08:20:13, dcheng wrote: > This assumes UTF8 input and will happily mangle the input if that's not the > case. Is that OK here? Ah! Good catch. Base64 encoding is probably more-appropriate here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.h (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:165: // shutdown an active remoting session. On 2016/09/21 03:15:50, miu wrote: > On 2016/09/20 08:20:12, dcheng wrote: > > Nit: document |ignored| here too? > > The 2nd arg is described in the interface. It is not being used by the override > impl. I'm not sure what there is to document. ;-) Ah I missed that this was an interface method, thanks. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_sender.cc (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:323: DCHECK_NE(flow_control_state_, CONSUME_PAUSED); On 2016/09/21 03:15:50, miu wrote: > On 2016/09/20 08:20:12, dcheng wrote: > > why is that? What stops these messages from being forwarded when we're in > > CONSUME_PAUSED? > > Short answer: We call binding_.PauseIncomingMethodCallProcessing(). This > mechanism is comprehensively unit tested > (CastRemotingSenderTest.ManagesFlowControl). > > Long answer: > > We transition into the CONSUME_PAUSED state when we've run out of buffer > capacity in the network transport. In this state, CastRemotingSender must stop > consuming from the mojo data pipe because there is nowhere for the data to go. > > However, the other end (in the render process) will continue to push more data > into the data pipe, and then make calls on the interface to instruct > CastRemotingSender to pull the data out from the pipe. Therefore, when we > transition into the CONSUME_PAUSED state, we also tell Mojo to pause incoming > method calls. This is a super-convenient Mojo feature: CastRemotingSender isn't > being told to consume from the data pipe and so the data will stay there until > we are ready for it. > > Once some data has been sent over the network, and we have room again in the > network transport buffers, we tell Mojo to resume incoming method calls. This > will cause CastRemotingSender to resume pulling data out of the data pipe again. > > One other concern is whether there is an upper-bound to how many mojo method > calls will be queued-up. There is! It is bound because the data pipe will > eventually fill to capacity. Once the render process cannot push more data into > the data pipe, it will also stop making calls on the interface to have > CastRemotingSender try to consume from the data pipe. It is simply forced to > wait until it sees the data pipe start draining again. > > All in all, a pretty natural way to handle flow control. :-) OK,thanks, I just wanted to make sure we weren't relying on the renderer to stop making calls and that we had something browser-side. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:346: if (result != MOJO_RESULT_OK) { On 2016/09/21 03:15:50, miu wrote: > On 2016/09/20 08:20:12, dcheng wrote: > > Is it possible for MOJO_RESULT_OUT_OF_RANGE to be a valid return value here > > (e.g. no data is read because the data is not all in the pipe yet) here? > > This should never happen. The render process is always supposed to push data > into the data pipe before calling the interface methods that would have > CastRemotingSender pull from the data pipe. So, the data better be there. Right, it's not clear to me if there's synchronization between a data pipe and a message pipe though. In mojo, unless the interfaces are associated, there's no ordering guarantee. e.g. if you have interface_one->Method(); interface_two->Method(); The calls could arrive in either order. I'm concerned that the same is true for data pipes: if we push data into the data pipe and then call this interface method, what synchronization guarantees does mojo provide? If it's consistent with how message pipes for interfaces work, I'd expect no ordering guarantees. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_sender_unittest.cc (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender_unittest.cc:196: sender_->ConsumeDataChunk(offset, size, total_payload_size); On 2016/09/21 03:15:50, miu wrote: > On 2016/09/20 08:20:13, dcheng wrote: > > It might make things slightly easier to just make sender_ protected: and have > > things call this directly? I'm not quite sure I understand the significance of > > these helpers otherwise. > > Required by the style guide: > https://google.github.io/styleguide/cppguide.html#Inheritance (note in the > "Decision" subsection that it explicitly states data members must be private). > From https://google.github.io/styleguide/cppguide.html#Access_Control Make data members private, unless they are static const (and follow the naming convention for constants). For technical reasons, we allow data members of a test fixture class to be protected when using Google Test). > Another reason was in the naming here: I wanted to call-out in the test code > that we are only "posting tasks" that make mojo method calls, which means the > task runner has to be spun up for those tasks to actually execute. I think that the async part is just part of the contract that individual test cases need to know about anyway, but this is fine too.
On 2016/09/22 at 02:29:15, dcheng wrote: > https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... > File chrome/browser/media/cast_remoting_connector.h (right): > > https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... > chrome/browser/media/cast_remoting_connector.h:165: // shutdown an active remoting session. > On 2016/09/21 03:15:50, miu wrote: > > On 2016/09/20 08:20:12, dcheng wrote: > > > Nit: document |ignored| here too? > > > > The 2nd arg is described in the interface. It is not being used by the override > > impl. I'm not sure what there is to document. ;-) > > Ah I missed that this was an interface method, thanks. > > https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... > File chrome/browser/media/cast_remoting_sender.cc (right): > > https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... > chrome/browser/media/cast_remoting_sender.cc:323: DCHECK_NE(flow_control_state_, CONSUME_PAUSED); > On 2016/09/21 03:15:50, miu wrote: > > On 2016/09/20 08:20:12, dcheng wrote: > > > why is that? What stops these messages from being forwarded when we're in > > > CONSUME_PAUSED? > > > > Short answer: We call binding_.PauseIncomingMethodCallProcessing(). This > > mechanism is comprehensively unit tested > > (CastRemotingSenderTest.ManagesFlowControl). > > > > Long answer: > > > > We transition into the CONSUME_PAUSED state when we've run out of buffer > > capacity in the network transport. In this state, CastRemotingSender must stop > > consuming from the mojo data pipe because there is nowhere for the data to go. > > > > However, the other end (in the render process) will continue to push more data > > into the data pipe, and then make calls on the interface to instruct > > CastRemotingSender to pull the data out from the pipe. Therefore, when we > > transition into the CONSUME_PAUSED state, we also tell Mojo to pause incoming > > method calls. This is a super-convenient Mojo feature: CastRemotingSender isn't > > being told to consume from the data pipe and so the data will stay there until > > we are ready for it. > > > > Once some data has been sent over the network, and we have room again in the > > network transport buffers, we tell Mojo to resume incoming method calls. This > > will cause CastRemotingSender to resume pulling data out of the data pipe again. > > > > One other concern is whether there is an upper-bound to how many mojo method > > calls will be queued-up. There is! It is bound because the data pipe will > > eventually fill to capacity. Once the render process cannot push more data into > > the data pipe, it will also stop making calls on the interface to have > > CastRemotingSender try to consume from the data pipe. It is simply forced to > > wait until it sees the data pipe start draining again. > > > > All in all, a pretty natural way to handle flow control. :-) > > OK,thanks, I just wanted to make sure we weren't relying on the renderer to stop making calls and that we had something browser-side. > > https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... > chrome/browser/media/cast_remoting_sender.cc:346: if (result != MOJO_RESULT_OK) { > On 2016/09/21 03:15:50, miu wrote: > > On 2016/09/20 08:20:12, dcheng wrote: > > > Is it possible for MOJO_RESULT_OUT_OF_RANGE to be a valid return value here > > > (e.g. no data is read because the data is not all in the pipe yet) here? > > > > This should never happen. The render process is always supposed to push data > > into the data pipe before calling the interface methods that would have > > CastRemotingSender pull from the data pipe. So, the data better be there. > > Right, it's not clear to me if there's synchronization between a data pipe and a message pipe though. > > In mojo, unless the interfaces are associated, there's no ordering guarantee. e.g. if you have > > interface_one->Method(); > interface_two->Method(); > > The calls could arrive in either order. > > I'm concerned that the same is true for data pipes: if we push data into the data pipe and then call this interface method, what synchronization guarantees does mojo provide? If it's consistent with how message pipes for interfaces work, I'd expect no ordering guarantees. > Daniel is correct. While in practice it will probably just work, i.e. events will in most cases be ordered between message pipes and data pipes going between the same processes, we cannot and absolutely do not make that guarantee, and it is definitely incorrect to rely on any such assumption. Data pipes provide ways to synchronize between the producer and the consumer, but given that they haven't gotten much heavy use in production yet it's entirely possible that the APIs are just insufficient for what you want. For example, thinking about this now, I could imagine wanting to configure a consumer pipe such that it doesn't even signal as readable until a minimum amount of data is available. We don't support something like that at the moment. In any case it should be possible to design around whatever's missing here for now. For example if you receive messages on a pipe which tell you to expect X bytes, you could accumulate that information and independently pull bytes off the data pipe as they're available, keeping expectations in sync. It would be helpful to understand how we can make the data pipe API more useful if it doesn't currently meet your needs. > https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... > File chrome/browser/media/cast_remoting_sender_unittest.cc (right): > > https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... > chrome/browser/media/cast_remoting_sender_unittest.cc:196: sender_->ConsumeDataChunk(offset, size, total_payload_size); > On 2016/09/21 03:15:50, miu wrote: > > On 2016/09/20 08:20:13, dcheng wrote: > > > It might make things slightly easier to just make sender_ protected: and have > > > things call this directly? I'm not quite sure I understand the significance of > > > these helpers otherwise. > > > > Required by the style guide: > > https://google.github.io/styleguide/cppguide.html#Inheritance (note in the > > "Decision" subsection that it explicitly states data members must be private). > > > > From https://google.github.io/styleguide/cppguide.html#Access_Control > Make data members private, unless they are static const (and follow the naming convention for constants). For technical reasons, we allow data members of a test fixture class to be protected when using Google Test). > > > Another reason was in the naming here: I wanted to call-out in the test code > > that we are only "posting tasks" that make mojo method calls, which means the > > task runner has to be spun up for those tasks to actually execute. > > I think that the async part is just part of the contract that individual test cases need to know about anyway, but this is fine too.
The CQ bit was checked by miu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixed the race between the message pipe and the data pipe in PS7. PTAL. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_sender.cc (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:346: if (result != MOJO_RESULT_OK) { On 2016/09/22 02:29:15, dcheng wrote: > On 2016/09/21 03:15:50, miu wrote: > > On 2016/09/20 08:20:12, dcheng wrote: > > > Is it possible for MOJO_RESULT_OUT_OF_RANGE to be a valid return value here > > > (e.g. no data is read because the data is not all in the pipe yet) here? > > In mojo, unless the interfaces are associated, there's no ordering guarantee. > e.g. if you have Good catch on this, and rockot@ confirmed. I re-worked the "flow control" logic a bit, and added a queue of retryable input operations. Now, whenever ConsumeDataChunk() is called, the internal impl actually checks whether the data pipe has the data. If not, it'll retry at a later point. I used a mojo::Watcher to signal when data becomes available, to retry the Consume operation. Because I had to maintain my own queue, I no longer use Mojo's Pause/ResumeMethodCallProcessing() feature. However, my earlier comments on why the input queue will not grow unbounded are still valid. I also added some extra unit tests to confirm all this new functionality is working. https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_sender_unittest.cc (right): https://codereview.chromium.org/2310753002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender_unittest.cc:196: sender_->ConsumeDataChunk(offset, size, total_payload_size); On 2016/09/22 02:29:15, dcheng wrote: > From https://google.github.io/styleguide/cppguide.html#Access_Control > Make data members private, unless they are static const (and follow the naming > convention for constants). For technical reasons, we allow data members of a > test fixture class to be protected when using Google Test). Ah, okay. Good to know. > > Another reason was in the naming here: I wanted to call-out in the test code > > that we are only "posting tasks" that make mojo method calls, which means the > > task runner has to be spun up for those tasks to actually execute. > > I think that the async part is just part of the contract that individual test > cases need to know about anyway, but this is fine too. Ok. IMHO the naming is likely help future code maintainers understand this subtlety as they read the test code, so I'll leave it as-is.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mojo lgtm https://codereview.chromium.org/2310753002/diff/140001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_sender.cc (right): https://codereview.chromium.org/2310753002/diff/140001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:368: << "Unexpcted result when discarding from data pipe (" Nit: Unexpected
https://codereview.chromium.org/2310753002/diff/140001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_sender.cc (right): https://codereview.chromium.org/2310753002/diff/140001/chrome/browser/media/c... chrome/browser/media/cast_remoting_sender.cc:368: << "Unexpcted result when discarding from data pipe (" On 2016/09/23 04:51:07, dcheng wrote: > Nit: Unexpected Done.
nasko: All ready now for OWNERS review for content/renderer/render_frame_impl.*
content/ LGTM, but please put a link to the CL that will be adding the RenderFrameImpl functionality in the description of this CL before committing it.
Description was changed from ========== Media Remoting: Data/Control plumbing between renderer and Media Router. Introduces the new media::mojom::Remoting* interfaces, which provide the control and data flows across browser processes for a new media remoting feature. These interfaces allow a renderer to connect the local media pipeline to a remote media rendering stack. Also, a Cast-specific implementation of these interfaces is provided, which allows media remoting sessions to be established within the context of a Cast Tab Mirroring session. This implementation uses the Chrome Media Router infrastructure to perform the discovery and selection of remoting-capable devices, establishing media routes for this new feature. This change provides one major component, among others, for a complete end-to-end media remoting feature. By itself, no new code paths are activated yet. Future code changes will provide integration with other efforts, such as: https://codereview.chromium.org/2307653002/ https://codereview.chromium.org/2261503002/ ...and the Media Router component extension. BUG=643964 ========== to ========== Media Remoting: Data/Control plumbing between renderer and Media Router. Introduces the new media::mojom::Remoting* interfaces, which provide the control and data flows across browser processes for a new media remoting feature. These interfaces allow a renderer to connect the local media pipeline to a remote media rendering stack. Also, a Cast-specific implementation of these interfaces is provided, which allows media remoting sessions to be established within the context of a Cast Tab Mirroring session. This implementation uses the Chrome Media Router infrastructure to perform the discovery and selection of remoting-capable devices, establishing media routes for this new feature. This change provides one major component, among others, for a complete end-to-end media remoting feature. By itself, no new code paths are activated yet. Future code changes will provide integration with other efforts, such as: https://codereview.chromium.org/2307653002/ https://codereview.chromium.org/2261503002/ https://codereview.chromium.org/2342163003/ ...and the Media Router component extension. BUG=643964 ==========
The CQ bit was checked by miu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, apacible@chromium.org, xjz@chromium.org, dcheng@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2310753002/#ps180001 (title: "Just a REBASE on ToT before commit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
media/mojo lgtm https://codereview.chromium.org/2310753002/diff/180001/media/mojo/interfaces/... File media/mojo/interfaces/remoting.mojom (right): https://codereview.chromium.org/2310753002/diff/180001/media/mojo/interfaces/... media/mojo/interfaces/remoting.mojom:1: // Copyright 2016 The Chromium Authors. All rights reserved. I was debating whether this file should be here or in media/remoting/interfaces. But it seems media/mojo/interfaces has been the default place for all media mojo interfaces (e.g. image_capture), so this seems fine to me now :)
The CQ bit was checked by miu@chromium.org
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 ========== Media Remoting: Data/Control plumbing between renderer and Media Router. Introduces the new media::mojom::Remoting* interfaces, which provide the control and data flows across browser processes for a new media remoting feature. These interfaces allow a renderer to connect the local media pipeline to a remote media rendering stack. Also, a Cast-specific implementation of these interfaces is provided, which allows media remoting sessions to be established within the context of a Cast Tab Mirroring session. This implementation uses the Chrome Media Router infrastructure to perform the discovery and selection of remoting-capable devices, establishing media routes for this new feature. This change provides one major component, among others, for a complete end-to-end media remoting feature. By itself, no new code paths are activated yet. Future code changes will provide integration with other efforts, such as: https://codereview.chromium.org/2307653002/ https://codereview.chromium.org/2261503002/ https://codereview.chromium.org/2342163003/ ...and the Media Router component extension. BUG=643964 ========== to ========== Media Remoting: Data/Control plumbing between renderer and Media Router. Introduces the new media::mojom::Remoting* interfaces, which provide the control and data flows across browser processes for a new media remoting feature. These interfaces allow a renderer to connect the local media pipeline to a remote media rendering stack. Also, a Cast-specific implementation of these interfaces is provided, which allows media remoting sessions to be established within the context of a Cast Tab Mirroring session. This implementation uses the Chrome Media Router infrastructure to perform the discovery and selection of remoting-capable devices, establishing media routes for this new feature. This change provides one major component, among others, for a complete end-to-end media remoting feature. By itself, no new code paths are activated yet. Future code changes will provide integration with other efforts, such as: https://codereview.chromium.org/2307653002/ https://codereview.chromium.org/2261503002/ https://codereview.chromium.org/2342163003/ ...and the Media Router component extension. BUG=643964 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Data/Control plumbing between renderer and Media Router. Introduces the new media::mojom::Remoting* interfaces, which provide the control and data flows across browser processes for a new media remoting feature. These interfaces allow a renderer to connect the local media pipeline to a remote media rendering stack. Also, a Cast-specific implementation of these interfaces is provided, which allows media remoting sessions to be established within the context of a Cast Tab Mirroring session. This implementation uses the Chrome Media Router infrastructure to perform the discovery and selection of remoting-capable devices, establishing media routes for this new feature. This change provides one major component, among others, for a complete end-to-end media remoting feature. By itself, no new code paths are activated yet. Future code changes will provide integration with other efforts, such as: https://codereview.chromium.org/2307653002/ https://codereview.chromium.org/2261503002/ https://codereview.chromium.org/2342163003/ ...and the Media Router component extension. BUG=643964 ========== to ========== Media Remoting: Data/Control plumbing between renderer and Media Router. Introduces the new media::mojom::Remoting* interfaces, which provide the control and data flows across browser processes for a new media remoting feature. These interfaces allow a renderer to connect the local media pipeline to a remote media rendering stack. Also, a Cast-specific implementation of these interfaces is provided, which allows media remoting sessions to be established within the context of a Cast Tab Mirroring session. This implementation uses the Chrome Media Router infrastructure to perform the discovery and selection of remoting-capable devices, establishing media routes for this new feature. This change provides one major component, among others, for a complete end-to-end media remoting feature. By itself, no new code paths are activated yet. Future code changes will provide integration with other efforts, such as: https://codereview.chromium.org/2307653002/ https://codereview.chromium.org/2261503002/ https://codereview.chromium.org/2342163003/ ...and the Media Router component extension. BUG=643964 Committed: https://crrev.com/1ddf18c6f77565d5e80beecaf965b73bb035a913 Cr-Commit-Position: refs/heads/master@{#420812} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/1ddf18c6f77565d5e80beecaf965b73bb035a913 Cr-Commit-Position: refs/heads/master@{#420812} |
