Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2457563002:
Media Remoting: Add remoting control logic for encrypted contents. (Closed)
|
Created:
4 years, 1 month ago by xjz Modified:
4 years, 1 month ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, chromoting-reviews_chromium.org, nasko+codewatch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia Remoting: Add remoting control logic for encrypted contents.
This CL does the following:
1. Adds a RemotingCdmController to control whether to create CDM
remotely for Media Remoting.
2. Renames the RemotingController to RemotingRendererController.
3. Separates the implementation of RemotingSource from
RemotingRenderController to RemotingSourceImpl. After OnSetCdm() is
called, one RemotingSourceImpl is shared by both
RemotingRenderController and RemotingCdmController.
BUG=643964
Committed: https://crrev.com/df8ea429178910ac6293e8e014fe307438fa099a
Cr-Commit-Position: refs/heads/master@{#430730}
Patch Set 1 : PS #4 from https://codereview.chromium.org/2406483002/ #Patch Set 2 : Rebase Only. #
Total comments: 11
Patch Set 3 : Addressed comments. #
Total comments: 10
Patch Set 4 : Addressed comments. #
Total comments: 8
Patch Set 5 : Addressed comments. #
Total comments: 84
Patch Set 6 : Rebased. Integrated with RemoteRendererImpl. #Patch Set 7 : Addressed xhwang's comments. #
Total comments: 8
Patch Set 8 : Addressed comments. #Patch Set 9 : Create RemotingSourceImpl before creating CDM. #Patch Set 10 : Add RemotingSinkObserver. #Patch Set 11 : Bug fix. #
Total comments: 30
Patch Set 12 : Addressed comments from PS#11. #
Total comments: 4
Patch Set 13 : Rebase. #
Total comments: 2
Patch Set 14 : Addressed comments from PS#12. Fixed ASAN. #Messages
Total messages: 72 (37 generated)
Description was changed from ========== Media Remoting: Add romoting control logic for encrypted contents. This CL does the following: 1. Adds a RemotingCdmController to control whether to create CDM remotely for Media Remoting. 2. Renames the RemotingController to RemotingRendererController. 3. Separates the implementation of RemotingSource from RemotingRenderController to RemotingSourceImpl. After OnSetCdm() is called, one RemotingSourceImpl is shared by both RemotingRenderController and RemotingCdmController. BUG=643964 ========== to ========== Media Remoting: Add romoting control logic for encrypted contents. This CL does the following: 1. Adds a RemotingCdmController to control whether to create CDM remotely for Media Remoting. 2. Renames the RemotingController to RemotingRendererController. 3. Separates the implementation of RemotingSource from RemotingRenderController to RemotingSourceImpl. After OnSetCdm() is called, one RemotingSourceImpl is shared by both RemotingRenderController and RemotingCdmController. BUG=643964 ==========
xjz@chromium.org changed reviewers: + miu@chromium.org, xhwang@chromium.org
The CQ bit was checked by xjz@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...
xjz@chromium.org changed reviewers: + nasko@chromium.org
nasko: PTAL changes on content/renderer/*. xhwang: PTAL changes on media/base/*. miu: PS #1 is identical to PS #4 in https://codereview.chromium.org/2406483002/. PTAL. Thanks all in advance!
Getting there. Comments on PS2: https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... File media/remoting/remoting_cdm.h (right): https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... media/remoting/remoting_cdm.h:61: const std::unique_ptr<RemotingCdmController> remoting_controller_; nit: Add a blank line above this line. https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... File media/remoting/remoting_cdm_controller.cc (right): https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... media/remoting/remoting_cdm_controller.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Weird. Now this file is being based on something unrelated. Maybe the similarity setting is too low now. Note: You can try different values with: git diff --stat -M<X>% origin/master The file list this generates will show, graphically, when the file is new versus derived/renamed from some other file. https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... File media/remoting/remoting_cdm_controller.h (right): https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... media/remoting/remoting_cdm_controller.h:37: scoped_refptr<RemotingSourceImpl> remoting_source() { Return RemotingSourceImpl* instead of scoped_refptr. https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:28: DCHECK(!switch_renderer_cb_.is_null()); This DCHECK() should probably be moved down to just before switch_renderer_cb_.Run(). (From looking at the code, I believe OnStarted(false) could be called multiple times in some edge cases.) if (remote_rendering_started_) { DCHECK(!switch_renderer_cb_.is_null()); switch_renderer_cb_.Run(); } else { ... https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:193: if (!remote_rendering_started_ && IsTerminated() && This logic is a bit complex, and it's hard to tell when it will trigger. Let's step back and think a bit more about this. The only time we would render the "EME needs reload" interstitial is when we transition to shut down a remoting session of encrytped content. Thus, I think you should delete this block of code here, and just add a simple check below. (See next comment.) https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:212: switch_renderer_cb_.Run(); For the encryption shutdown case, this else clause could be: } else { // When using encryption, local playback cannot resume, so don't switch the renderer. Instead, the RemotingRendererImpl will show a "need to reload" interstitial to indicate to the user that the page needs to be reloaded. if (!is_encrypted_) switch_renderer_cb_.Run(); remoting_source_->StopRemoting(this); } Note that, after this point, ShouldBeRemoting() will never return true ever again for the is_encrypted_ case; and so the RemotingRendererImpl will be switched again; and so the "need to reload" interstitial should remain until the page is reloaded by the user. https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:217: bool RemotingRendererController::IsRenderingRemotely() const { nit: Consider inlining this simple accessor in the header file: bool remote_rendering_started() const { return remote_rendering_started_; } https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:223: bool RemotingRendererController::IsTerminated() const { This method really doesn't do much. After other suggestion, it looks like it only might be called in two places. So, can we delete this method and just use the "state() == PERMANENTLY_STOPPED" expression directly where it is needed? https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... File media/remoting/remoting_renderer_controller.h (right): https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... media/remoting/remoting_renderer_controller.h:73: bool session_permanently_terminated_ = false; See comments in .cc file. I think we can remove this boolean. Hint: In each code review pass, adding booleans indicates to me that the logic may be more complex than it has to be. So, try to think of more functional-style approaches (i.e., would a boolean always be true or false if some other variable were a certain value?). https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... File media/remoting/remoting_renderer_factory.cc (right): https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... media/remoting/remoting_renderer_factory.cc:25: if (remoting_controller_ && remoting_controller_->IsTerminated()) { On 2016/10/26 22:00:27, xjz wrote: > On 2016/10/25 04:21:26, miu wrote: > > Seems like this should be: > > > > if (remoting_controller_ && > > remoting_controller_->IsRemoting() && > > !remoting_controller_->IsTerminated()) { > > > > Rather than having two separate IsRemoting() and IsTerminated() methods, you > > might just combine them and rename to something like IsRenderingRemotely(). > > Renamed IsRemoting() to IsRenderingRemotely(). I originally thought that we > should switch to remoting renderer if remoting session is started or is > permanently terminated. On a second thought, I think for the latter, we may just > create a separate renderer to only render that failure page (with apacible's > change). So now I separated the two cases. WDYT? Based on prior comments (in the control logic for shutting down the EME case), I think it would be simpler not to switch Renderers at all. The RemotingRendererImpl knows when the session is permanently terminated, so it can use that as a signal that the interstitial it is rendering should change to the "reload to play EME content" interstitial versus the "now casting" interstitial that was being shown previously.
xjz@chromium.org changed reviewers: + erickung@chromium.org
https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:193: if (!remote_rendering_started_ && IsTerminated() && On 2016/10/26 23:53:10, miu wrote: > This logic is a bit complex, and it's hard to tell when it will trigger. Let's > step back and think a bit more about this. The only time we would render the > "EME needs reload" interstitial is when we transition to shut down a remoting > session of encrytped content. > > Thus, I think you should delete this block of code here, and just add a simple > check below. (See next comment.) These codes here are for the case that OnSetCdm() is called, and the remoting session that created the remoting cdm was terminated. In this case, both |remote_rendering_started_| and |should_be_remoting| are false. But I think for this case, we need to switch renderer to show the failure page. right? https://codereview.chromium.org/2457563002/diff/20001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:212: switch_renderer_cb_.Run(); On 2016/10/26 23:53:10, miu wrote: > For the encryption shutdown case, this else clause could be: > > } else { > // When using encryption, local playback cannot resume, so don't switch the > renderer. Instead, the RemotingRendererImpl will show a "need to reload" > interstitial to indicate to the user that the page needs to be reloaded. > if (!is_encrypted_) > switch_renderer_cb_.Run(); > remoting_source_->StopRemoting(this); > } > > Note that, after this point, ShouldBeRemoting() will never return true ever > again for the is_encrypted_ case; and so the RemotingRendererImpl will be > switched again; and so the "need to reload" interstitial should remain until the > page is reloaded by the user. How does RemotingRendererImpl know a session is shut down? Do we have a message for that?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
content/ changes LGTM
Patchset #5 (id:80001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:100001) has been deleted
Addressed miu's comments. PTAL. miu: Sorry, I accidentally deleted PS#2 with your comments on while trying upload the patch with different similarity values. Re-uploaded same PS#2, and manually typed all your comments in the quoted text with my replies. Hope you are OK with that. :) https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... File media/remoting/remoting_cdm.h (right): https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... media/remoting/remoting_cdm.h:61: const std::unique_ptr<RemotingCdmController> remoting_controller_; > From miu: > nit: Add a blank line above this line. Done. https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... File media/remoting/remoting_cdm_controller.cc (right): https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... media/remoting/remoting_cdm_controller.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. > Frome miu: > Weird. Now this file is being based on something unrelated. Maybe the >similarity setting is too low now. > Note: You can try different values with: > git diff --stat -M<X>% origin/master > The file list this generates will show, graphically, when the file is > new versus derived/renamed from some other file. 20% seems work well locally, but not with "git cl upload". Tried with different values, and used 30%. https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... File media/remoting/remoting_cdm_controller.h (right): https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... media/remoting/remoting_cdm_controller.h:37: scoped_refptr<RemotingSourceImpl> remoting_source() { > From miu: > Return RemotingSourceImpl* instead of scoped_refptr. Done. https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:28: DCHECK(!switch_renderer_cb_.is_null()); > From miu: > This DCHECK() should probably be moved down to just before > switch_renderer_cb_.Run(). (From looking at the code, I believe > OnStarted(false) could be called multiple times in some edge cases.) > > if (remote_rendering_started_) { > DCHECK(!switch_renderer_cb_.is_null()); > switch_renderer_cb_.Run(); > } else { > ... Done. https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:193: if (!remote_rendering_started_ && IsTerminated() && > From miu: > This logic is a bit complex, and it's hard to tell when it will trigger. > Let's step back and think a bit more about this. The only time we would > render the "EME needs reload" interstitial is when we transition to shut > down a remoting session of encrytped content. > > Thus, I think you should delete this block of code here, and just add a > simple check below. (See next comment.) As discussed, modified the logic so that we can switch to remoting renderer for this edge case. https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:212: switch_renderer_cb_.Run(); > From miu: > For the encryption shutdown case, this else clause could be: > > } else { > // When using encryption, local playback cannot resume, so don't > switch the renderer. Instead, the RemotingRendererImpl will show a "need > to reload" interstitial to indicate to the user that the page needs to > be reloaded. > if (!is_encrypted_) > switch_renderer_cb_.Run(); > remoting_source_->StopRemoting(this); > } > > Note that, after this point, ShouldBeRemoting() will never return true > ever again for the is_encrypted_ case; and so the RemotingRendererImpl > will be switched again; and so the "need to reload" interstitial should > remain until the page is reloaded by the user. As discussed, modified the logic and will let RemotingRendererImpl handle the failure case. https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:217: bool RemotingRendererController::IsRenderingRemotely() const { > From miu: > nit: Consider inlining this simple accessor in the header file: > > bool remote_rendering_started() const { return > remote_rendering_started_; } Done. https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:223: bool RemotingRendererController::IsTerminated() const { > From miu: > This method really doesn't do much. After other suggestion, it looks > like it only might be called in two places. So, can we delete this > method and just use the "state() == PERMANENTLY_STOPPED" expression > directly where it is needed? Done. https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.h (right): https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... media/remoting/remoting_renderer_controller.h:73: bool session_permanently_terminated_ = false; > From miu: > See comments in .cc file. I think we can remove this boolean. > > Hint: In each code review pass, adding booleans indicates to me that the > logic may be more complex than it has to be. So, try to think of more > functional-style approaches (i.e., would a boolean always be true or > false if some other variable were a certain value?). Done. https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... File media/remoting/remoting_renderer_factory.cc (right): https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... media/remoting/remoting_renderer_factory.cc:25: if (remoting_controller_ && remoting_controller_->IsTerminated()) { > From miu: > Based on prior comments (in the control logic for shutting down the EME > case), I think it would be simpler not to switch Renderers at all. The > RemotingRendererImpl knows when the session is permanently terminated, > so it can use that as a signal that the interstitial it is rendering > should change to the "reload to play EME content" interstitial versus > the "now casting" interstitial that was being shown previously. Done.
The CQ bit was checked by xjz@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/120001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:212: switch_renderer_cb_.Run(); On 2016/10/27 22:45:58, xjz wrote: > > From miu: > > For the encryption shutdown case, this else clause could be: > > > > } else { > > // When using encryption, local playback cannot resume, so don't > > switch the renderer. Instead, the RemotingRendererImpl will show a "need > > to reload" interstitial to indicate to the user that the page needs to > > be reloaded. > > if (!is_encrypted_) > > switch_renderer_cb_.Run(); > > remoting_source_->StopRemoting(this); > > } > > > > Note that, after this point, ShouldBeRemoting() will never return true > > ever again for the is_encrypted_ case; and so the RemotingRendererImpl > > will be switched again; and so the "need to reload" interstitial should > > remain until the page is reloaded by the user. > > As discussed, modified the logic and will let RemotingRendererImpl handle the > failure case. Seems now like we were discussing two different things. ;) You were talking about: CDM fails before remote rendering is started. I was talking about: We were remoting encrypted content, and now are shutting down. In both cases, we want to be using the RemotingRendererImpl because it will be used to show interstitials to the user to notify them to reload the page to continue with local rendering. https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:47: switch (remoting_source_->state()) { Looking at this code again, I think you can simultaneously get rid of the |session_can_remote_| bool and put this logic in ShouldBeRemoting() where it will be clearer why we "can remote." So, the whole OnSessionStateChanged() method could be: void RemotingRendererController::OnSessionStateChanged() { DCHECK(thread_checker_.CalledOnValidThread()); VLOG(1) << "OnSessionStateChanged: " << remoting_source_->state(); UpdateAndMaybeSwitch(); } https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:174: return remoting_source_->state() == RemotingSessionState::SESSION_STARTED || Hmm...Things moved around and now I'm confused again: 1. I could be wrong, but shouldn't this also include SESSION_STARTING and SESSION_STOPPING? 2. I'm also not sure we should be unconditionally skipping the checking of the other dependencies (the ones you moved below this). I think we were discussing two completely different things the other day. ;) IIUC, you were talking about: CDM fails (and ends session) before remote rendering is started. Instead, I was talking about: Ecrypted content was remotely rendering, and now is shutting down. In both cases, we want to be using the RemotingRendererImpl because it will be used to show interstitials to the user to notify them to reload the page to continue with local rendering. Just to make sure we're on the same page, here are the transitions I think we need to consider in this logic: 1. The non-EME case: If the session can start (or is already running), all codec dependencies are met, and is_fullscreen_ == true; we want to be using the remoting renderer. If any of these changes, we return false to go back to using the local renderer. 2. EME: We were never before remotely rendering, and we are in the CAN_START or STARTING state: Return false to keep the local renderer because the CDM has not started remoting yet. 3. EME: We were never before remotely rendering, and CDM has shut down the session: Always return true so we'll be using the RemotingRendererImpl to show the "error" interstitial. 4. EME: We were never before remotely rendering, and the CDM has started running (the session is now in the STARTED state to indicate this): Return true since we are now able to remotely render. 5. EME: We were successfully remotely rendering, and the session is stopping or has shut down: Return true so we'll be using the RemotingRendererImpl to show the "reload to continue" interstitial. I was trying to think of how to describe suggested changes to the code, but I ended up having to work it all out in an editor. So, taking all of the above into account, here's what I came up with: bool RemotingRendererController::ShouldBeRemoting() { DCHECK(thread_checker_.CalledOnValidThread()); if (!switch_renderer_cb_.is_null()) return false; // No way to switch to a remoting Renderer impl. switch (remoting_source_->state()) { case SESSION_UNAVAILABLE: return false; // Cannot remote media without a remote sink. case SESSION_CAN_START: case SESSION_STARTING: if (is_encrypted_) return false; // Prerequisite: Remoting CDM not running yet. break; // Non-encrypted media remoting is possible. case SESSION_STARTED: break; // Both encrypted and non-encrypted media remoting are possible. case SESSION_STOPPING: case SESSION_PERMANENTLY_STOPPED: // If content is encrypted, and the session is being stopped (or has // stopped), return true to indicate that RemotingRendererImpl should be // used to display an interstitial since local rendering cannot be // resumed. Otherwise, return false to use local media rendering. return is_encrypted_; } // TODO(xjz): Remoting CDM should not start until supported codecs are detected. if ((!has_audio_ && !has_video_) || (has_video_ && !IsVideoCodecSupported()) || (has_audio_ && !IsAudioCodecSupported())) { return false; } // Normally, entering fullscreen is the signal that starts remote rendering. // However, current technical limitations require encrypted content be remoted // without waiting for a user signal. return is_fullscreen_ || is_encrytped_; } https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:214: DCHECK(!is_encrypted_); Please add comment to explain this DCHECK (something like: "For encrypted content, it's only valid to switch to the remoting renderer, and never back to the local renderer."). https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... File media/remoting/remoting_renderer_controller_unittest.cc (right): https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... media/remoting/remoting_renderer_controller_unittest.cc:161: bool is_mirroring_ = false; naming: The name you had before was much better: It was very accurate, and "mirroring" is not accurate at all, IMHO. https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... File media/remoting/remoting_source_impl.h (right): https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... media/remoting/remoting_source_impl.h:84: void ShutDown(); naming nit: Shutdown() ^^^
Patchset #4 (id:150001) has been deleted
Addressed comments. PTAL. https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:47: switch (remoting_source_->state()) { On 2016/10/29 02:22:16, miu wrote: > Looking at this code again, I think you can simultaneously get rid of the > |session_can_remote_| bool and put this logic in ShouldBeRemoting() where it > will be clearer why we "can remote." So, the whole OnSessionStateChanged() > method could be: > > void RemotingRendererController::OnSessionStateChanged() { > DCHECK(thread_checker_.CalledOnValidThread()); > VLOG(1) << "OnSessionStateChanged: " << remoting_source_->state(); > UpdateAndMaybeSwitch(); > } Done. https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:174: return remoting_source_->state() == RemotingSessionState::SESSION_STARTED || On 2016/10/29 02:22:16, miu wrote: > Hmm...Things moved around and now I'm confused again: > > 1. I could be wrong, but shouldn't this also include SESSION_STARTING and > SESSION_STOPPING? Yes, I should include SESSION_STOPPING here. But the session should never be in SESSION_STARTING state when remoting cdm was created. So always return false for encrypted content when session is starting. > > 2. I'm also not sure we should be unconditionally skipping the checking of the > other dependencies (the ones you moved below this). As discussed offline, we will let RemotingRendererImpl handle the failure for codec support for encrypted contents. Moved other two checks back (callback and whether we have video or audio). > > > I think we were discussing two completely different things the other day. ;) > IIUC, you were talking about: CDM fails (and ends session) before remote > rendering is started. Instead, I was talking about: Ecrypted content was > remotely rendering, and now is shutting down. > > In both cases, we want to be using the RemotingRendererImpl because it will be > used to show interstitials to the user to notify them to reload the page to > continue with local rendering. Yes, I agree. > > > Just to make sure we're on the same page, here are the transitions I think we > need to consider in this logic: > > 1. The non-EME case: If the session can start (or is already running), all codec > dependencies are met, and is_fullscreen_ == true; we want to be using the > remoting renderer. If any of these changes, we return false to go back to using > the local renderer. > > 2. EME: We were never before remotely rendering, and we are in the CAN_START or > STARTING state: Return false to keep the local renderer because the CDM has not > started remoting yet. > > 3. EME: We were never before remotely rendering, and CDM has shut down the > session: Always return true so we'll be using the RemotingRendererImpl to show > the "error" interstitial. > > 4. EME: We were never before remotely rendering, and the CDM has started running > (the session is now in the STARTED state to indicate this): Return true since we > are now able to remotely render. > > 5. EME: We were successfully remotely rendering, and the session is stopping or > has shut down: Return true so we'll be using the RemotingRendererImpl to show > the "reload to continue" interstitial. > > > I was trying to think of how to describe suggested changes to the code, but I > ended up having to work it all out in an editor. So, taking all of the above > into account, here's what I came up with: > > bool RemotingRendererController::ShouldBeRemoting() { > DCHECK(thread_checker_.CalledOnValidThread()); > > if (!switch_renderer_cb_.is_null()) > return false; // No way to switch to a remoting Renderer impl. > > switch (remoting_source_->state()) { > case SESSION_UNAVAILABLE: > return false; // Cannot remote media without a remote sink. > case SESSION_CAN_START: > case SESSION_STARTING: > if (is_encrypted_) > return false; // Prerequisite: Remoting CDM not running yet. > break; // Non-encrypted media remoting is possible. > case SESSION_STARTED: > break; // Both encrypted and non-encrypted media remoting are possible. > case SESSION_STOPPING: > case SESSION_PERMANENTLY_STOPPED: > // If content is encrypted, and the session is being stopped (or has > // stopped), return true to indicate that RemotingRendererImpl should be > // used to display an interstitial since local rendering cannot be > // resumed. Otherwise, return false to use local media rendering. > return is_encrypted_; > } > > // TODO(xjz): Remoting CDM should not start until supported codecs are > detected. > if ((!has_audio_ && !has_video_) || > (has_video_ && !IsVideoCodecSupported()) || > (has_audio_ && !IsAudioCodecSupported())) { > return false; > } > > // Normally, entering fullscreen is the signal that starts remote rendering. > // However, current technical limitations require encrypted content be remoted > // without waiting for a user signal. > return is_fullscreen_ || is_encrytped_; > } Modified the logic as we discussed offline. PTAL. https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:214: DCHECK(!is_encrypted_); On 2016/10/29 02:22:16, miu wrote: > Please add comment to explain this DCHECK (something like: "For encrypted > content, it's only valid to switch to the remoting renderer, and never back to > the local renderer."). Done. https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... File media/remoting/remoting_renderer_controller_unittest.cc (right): https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... media/remoting/remoting_renderer_controller_unittest.cc:161: bool is_mirroring_ = false; On 2016/10/29 02:22:16, miu wrote: > naming: The name you had before was much better: It was very accurate, and > "mirroring" is not accurate at all, IMHO. Done. https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... File media/remoting/remoting_source_impl.h (right): https://codereview.chromium.org/2457563002/diff/60002/media/remoting/remoting... media/remoting/remoting_source_impl.h:84: void ShutDown(); On 2016/10/29 02:22:17, miu wrote: > naming nit: Shutdown() > ^^^ Done.
The CQ bit was checked by xjz@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...
xhwang: PTAL the changes in media/base/cdm_context.*. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm % code comment considerations: https://codereview.chromium.org/2457563002/diff/170001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/170001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:155: if (!has_audio_ && !has_video_) Does the pipeline ever give us "null" config change at the end of an encrypted stream? If so, maybe this should be after the switch() statement? https://codereview.chromium.org/2457563002/diff/170001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:164: RemotingSessionState state = remoting_source_->state(); nit: const https://codereview.chromium.org/2457563002/diff/170001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:166: return state == RemotingSessionState::SESSION_STARTED || Let's make sure and have a comment explaining this logic (so we don't forget prior discussions/conclusions). Something like: "Due to technical limitations when playing encrypted content, once a remoting session has been started, always return true here to indicate that the RemotingRendererImpl should be used. In the stopped states, RemotingRendererImpl will display an interstitial to notify the user that local rendering cannot be resumed." Also, in the last round of review, I had code comments explaining reasons for returning true/false, and I think it makes sense to include those (again, for documentative reasons). So, consider adding code comments here: if (switch_renderer_cb_.is_null()) return false; // No way to switch to a remoting Renderer impl. and here: switch (state) { case SESSION_UNAVAILABLE: return false; // Cannot remote media without a remote sink. case SESSION_CAN_START: case SESSION_STARTING: case SESSION_STARTED: break; // Media remoting is possible, assuming other req's are met. case SESSION_STOPPING: case SESSION_PERMANENTLY_STOPPED: return false; // Use local rendering after stopping remoting. } and here: // Normally, entering fullscreen is the signal that starts remote rendering. // However, current technical limitations require encrypted content be remoted // without waiting for a user signal. return is_fullscreen_; https://codereview.chromium.org/2457563002/diff/170001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:210: // For encrypted contents, it's only valid to switch to remoting renderer, s/contents/content/ Oh, and I just noticed we call StopRemoting(this) a few lines down. So, this code comment should probably also explain (for other developers' benefit): "The RemotingCdmController will force-stop the session when remoting has ended; so no need to call StopRemoting() from here."
Sorry for the delay. I'll take a look today or tomorrow.
Patchset #6 (id:210001) has been deleted
I accidentally reviewed all the files (except test) :) Here are my comments. I am mostly concerned that we could enable this in prod without a run-time flag which could cause a lot of unnecessary noise/stress for you. Since you want a flag (base::Feature) for future experiments anyways, should we add it earlier? https://codereview.chromium.org/2457563002/diff/190001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2457563002/diff/190001/content/renderer/rende... content/renderer/render_frame_impl.cc:6412: GetRemoterFactory())); On desktop, where we have both ENABLE_PEPPER_CDMS and ENABLE_MEDIA_REMOTING, we'll create a RendererCdmFactory, destroy it, and create another one. Can we streamline the logic to avoid this? https://codereview.chromium.org/2457563002/diff/190001/media/base/cdm_context.h File media/base/cdm_context.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/base/cdm_context... media/base/cdm_context.h:38: // method to provide safe down-casting to their type. If I add a new CdmContext, how should I implement this method? https://codereview.chromium.org/2457563002/diff/190001/media/base/cdm_context... media/base/cdm_context.h:39: virtual void* GetClassIdentifier() const; Please see my comment in remoting_cdm.cc about the class ID. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.cc:13: void* const kClassIdentifier = const_cast<void**>(&kClassIdentifier); This will work, but I wonder how is this better than less fancy approaches like having a fixed unique id (e.g. name-like string, or GUID)? It seems as long as the ID is unique it'll work. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:15: // TODO(xjz): Merge this with Eric's implementation. nit: use ldap instead of first name for easier tracking https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:30: static RemotingCdm* From(CdmContext* cdm); We don't want to get the CDM from the CdmContext :) Please see comment below (on OnSetCdm()) for reasons. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:32: RemotingSourceImpl* GetRemotingSource(); Who owns the returned value? If it's owned by this CDM, how can we make sure the returned value isn't accessed after this CDM is destroyed? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:32: RemotingSourceImpl* GetRemotingSource(); By looking at code below, it seems RSI is ref-counted. We should not pass around raw-pointers to a ref-counted object... https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:62: const std::unique_ptr<RemotingCdmController> remoting_controller_; nit: will |remoting_cdm_controller_| be more explicit since we have other types of remoting controller as well? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_controller.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.cc:31: base::ResetAndReturn(&cdm_check_cb_).Run(success); Is it possible that |cdm_check_cb_| is null, e.g. ShouldCRC() never called? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.cc:39: remoting_source_->Shutdown(); It's a bit confusing to me on the ownership of |remoting_source_|. It seems we can pass a remoting_source out by remoting_source(). Then who should call Shutdown()? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_controller.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.h:14: } This won't work and you are actually getting the declaration of ThreadChecker from remoting_source_impl.h. Please remove this forward declaration and add include directly. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.h:34: using CdmCheckCallback = base::Callback<void(bool is_remoting)>; nit: Returns whether we should create remoting CDM via |cb|, which could be run synchronously ..... https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.h:39: return remoting_source_.get(); See comment above... why are we passing raw pointers to a ref-counted object around? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.h:48: // Indicates if remoting is started. what does "remoting is started" means exactly? e.g. remoting CDM created? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.h:52: // debug builds. nit: This comment seems unnecessary :) But it's up to you. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:30: NOTIMPLEMENTED(); If we land this CL now, when we are "remoting" (not sure about the exact meaning), protected content playback will fail. Is this correct? Could this actually happen in prod? Can we land a CL to add a command line flag first before we enable remoting by default? This will help avoid unnecessary noise/stress to you, other devs, and users. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:31: } else { nit: we like to return early: if (foo) { Bar(); return; } Qux(); https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:66: // TODO: Replace the callbacks with an interface. http://crbug.com/657940. nit: Usually you have a name/ladp for the TODO :) https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:78: RemotingCdmController* remoting_controller_ptr = remoting_controller.get(); nit: It's worth a comment why you are doing this. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:83: default_cdm_factory_.get())); Is it possible that the callback is fired after |this| is destroyed? If so, default_cdm_factory will be destroyed as well when the callback fires. It's probably easier if CreateCdm() is a member function and we use a weakptr to make sure the callback won't fire after |this| is destroyed. If you do that, you can also store a pending_remoting_controller_ instead of passing of ownership of the |remoting_controller| to the callback, which IMHO is cleaner, but it's totally up to you. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:14: RemotingRendererController::RemotingRendererController( For future CL, please consider do renames in a separate CL. Rename CLs will be super trivial to review, then this CL will be much less noisy. If you have to do renames in the same CL, please consider upload a rename-only patchset first, then upload real changes in a different patchset. This way, it'll be much easier to see the real changes by looking at patchset differences. Now for this file, it's pretty hard to know what has been changed. I can get a better view by looking at the diff against PS1, but PS1 may already contain other changes (since it's PS4 in the previous CL). https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:66: auto* cdm = RemotingCdm::From(cdm_context); Is GetRemotingSource() the only thing you need from the CDM? The purpose that we have CdmContext is to isolate the media player (e.g. renderer) from accessing the CDM directly. Therefore, it's more desirable that we cast CdmContext to a RemotingCdmContext here, and get GetRemotingSource() there, instead of getting the CDM from the context, which defies the purpose of CdmContext. You can see MediaDrmBridgeCdmContext for an example. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_renderer_controller.h:55: RemotingSourceImpl* remoting_source() const { ditto about passing raw pointer around... https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_renderer_controller_unittest.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_renderer_controller_unittest.cc:30: data.video_decoder_config = VideoDecoderConfig( You can use TestVideoConfig::Normal(), etc: https://cs.chromium.org/chromium/src/media/base/test_helpers.h?rcl=0&l=81 Unfortunately we don't have an audio one yet :( https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_renderer_controller_unittest.cc:48: EmptyExtraData(), AesCtrEncryptionScheme()); ditto, you can use TestVideoConfig::NormalEncrypted https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_source_impl.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:62: client->OnStarted(true); Actually you notify all clients, so the comment is a bit misleading. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:121: client->OnStarted(false); hmm, now I see why you have |client|... Does it make sense to make sure clients call StartRemoting() only in the right state (CAN_START)? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:143: return; If this happens, it'll be a programming error... DCHECK? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:153: return; ditto, DCHECK? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_source_impl.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.h:22: SESSION_STARTING, if "starting a remoting session" failed, what state will we end up with? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.h:26: SESSION_STOPPING, Can we get back to STARTED after STOPPING? More generally, can you describe a state machine about possible transitions? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.h:88: void AddClient(Client* client); It's odd that a Client can be added here, but is also specified in StartRemoting() and StopRemoting(). It's more confusing that for StartRemoting(), only |client| will be notified, even though all clients implemented OnStarted(). Then for StopRemoting(), actually all clients will get notified. Does it make sense to simply this so that we only add client through AddClient(), then for StartRemoting() and StopRemoting(), all clients will get notified? If this doesn't work, you can add a callback for StartRemoting(): StartRemoting(const OnStartedCB& cb); Stopremoting(); AddClient(Client* client); // Remove OnStarted() from the Client interface. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.h:88: void AddClient(Client* client); Who owns the |client|? Maybe worth commenting that clients are supposed to call RemoveClient() before it's gone.
Addressed miu's comments in PS# 5. Thanks! https://codereview.chromium.org/2457563002/diff/170001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/170001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:155: if (!has_audio_ && !has_video_) On 2016/10/31 20:10:31, miu wrote: > Does the pipeline ever give us "null" config change at the end of an encrypted > stream? If so, maybe this should be after the switch() statement? It sounds to me checking this either before or after switch() statement is correct. But |is_encrypted_| could be set to true only when this check returns false, so there is no need to check this again for encrypted content. Now moved this check after switch() statement. https://codereview.chromium.org/2457563002/diff/170001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:164: RemotingSessionState state = remoting_source_->state(); On 2016/10/31 20:10:31, miu wrote: > nit: const Done. https://codereview.chromium.org/2457563002/diff/170001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:166: return state == RemotingSessionState::SESSION_STARTED || On 2016/10/31 20:10:31, miu wrote: > Let's make sure and have a comment explaining this logic (so we don't forget > prior discussions/conclusions). Something like: > > "Due to technical limitations when playing encrypted content, once a remoting > session has been started, always return true here to indicate that the > RemotingRendererImpl should be used. In the stopped states, RemotingRendererImpl > will display an interstitial to notify the user that local rendering cannot be > resumed." > > Also, in the last round of review, I had code comments explaining reasons for > returning true/false, and I think it makes sense to include those (again, for > documentative reasons). So, consider adding code comments here: > > if (switch_renderer_cb_.is_null()) > return false; // No way to switch to a remoting Renderer impl. > > and here: > > switch (state) { > case SESSION_UNAVAILABLE: > return false; // Cannot remote media without a remote sink. > case SESSION_CAN_START: > case SESSION_STARTING: > case SESSION_STARTED: > break; // Media remoting is possible, assuming other req's are met. > case SESSION_STOPPING: > case SESSION_PERMANENTLY_STOPPED: > return false; // Use local rendering after stopping remoting. > } > > and here: > > // Normally, entering fullscreen is the signal that starts remote rendering. > // However, current technical limitations require encrypted content be remoted > // without waiting for a user signal. > return is_fullscreen_; Done. https://codereview.chromium.org/2457563002/diff/170001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:210: // For encrypted contents, it's only valid to switch to remoting renderer, On 2016/10/31 20:10:31, miu wrote: > s/contents/content/ > > Oh, and I just noticed we call StopRemoting(this) a few lines down. So, this > code comment should probably also explain (for other developers' benefit): "The > RemotingCdmController will force-stop the session when remoting has ended; so no > need to call StopRemoting() from here." Done.
https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_controller.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.cc:56: remoting_source_->StartRemoting(this); For the majority (say >99%) of playback sessions where user is not casting the tab, |is_remoting_| will always be false, so we'll always end up calling StartRemoting() which is a mojo IPC to the browser and wait for the response back. This could increase the start-to-play latency. I think remoting should not introduce new latency for normal playbacks. To achieve this, is it possible to cache some values about whether the tab is being casted in the render process, and if it's false, do not try remoting at all? There's a slight chance of race here that user just clicked to cast, and then started a playback, so the render process side cache is not updated, but I think the chance for this to happen is very small, and even if it happens, we can always fall back to mirroring.
Patchset #7 (id:250001) has been deleted
Patchset #7 (id:270001) has been deleted
Addressed xhwang's comments. PTAL. https://codereview.chromium.org/2457563002/diff/190001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2457563002/diff/190001/content/renderer/rende... content/renderer/render_frame_impl.cc:6412: GetRemoterFactory())); On 2016/11/01 08:21:28, xhwang wrote: > On desktop, where we have both ENABLE_PEPPER_CDMS and ENABLE_MEDIA_REMOTING, > we'll create a RendererCdmFactory, destroy it, and create another one. Can we > streamline the logic to avoid this? Done. Nice catch. Thanks. https://codereview.chromium.org/2457563002/diff/190001/media/base/cdm_context.h File media/base/cdm_context.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/base/cdm_context... media/base/cdm_context.h:38: // method to provide safe down-casting to their type. On 2016/11/01 08:21:28, xhwang wrote: > If I add a new CdmContext, how should I implement this method? The implementation of this is up to each implementation of CdmContext. If not implemented, this will return null ptr by default. https://codereview.chromium.org/2457563002/diff/190001/media/base/cdm_context... media/base/cdm_context.h:39: virtual void* GetClassIdentifier() const; On 2016/11/01 08:21:28, xhwang wrote: > Please see my comment in remoting_cdm.cc about the class ID. Replied to that comment. :) https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.cc:13: void* const kClassIdentifier = const_cast<void**>(&kClassIdentifier); On 2016/11/01 08:21:28, xhwang wrote: > This will work, but I wonder how is this better than less fancy approaches like > having a fixed unique id (e.g. name-like string, or GUID)? It seems as long as > the ID is unique it'll work. I used the same way as here: https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/processed_... I do think this is a very smart way to get a unique id. Though it is not very straight forward when I first saw this. :) https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:15: // TODO(xjz): Merge this with Eric's implementation. On 2016/11/01 08:21:28, xhwang wrote: > nit: use ldap instead of first name for easier tracking Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:30: static RemotingCdm* From(CdmContext* cdm); On 2016/11/01 08:21:28, xhwang wrote: > We don't want to get the CDM from the CdmContext :) Please see comment below (on > OnSetCdm()) for reasons. Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:32: RemotingSourceImpl* GetRemotingSource(); On 2016/11/01 08:21:28, xhwang wrote: > By looking at code below, it seems RSI is ref-counted. We should not pass around > raw-pointers to a ref-counted object... The below was commented by miu: General scoped_refptr usage note: The return type of a direct accessor should be the raw pointer (RemotingSourceImpl*) instead. This lets the caller decide whether to increment the ref-count by storing the raw pointer into a scoped_refptr. +miu https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:32: RemotingSourceImpl* GetRemotingSource(); On 2016/11/01 08:21:28, xhwang wrote: > Who owns the returned value? If it's owned by this CDM, how can we make sure the > returned value isn't accessed after this CDM is destroyed? The returned value is stored to a scoped_refptr. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:62: const std::unique_ptr<RemotingCdmController> remoting_controller_; On 2016/11/01 08:21:28, xhwang wrote: > nit: will |remoting_cdm_controller_| be more explicit since we have other types > of remoting controller as well? Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_controller.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.cc:31: base::ResetAndReturn(&cdm_check_cb_).Run(success); On 2016/11/01 08:21:28, xhwang wrote: > Is it possible that |cdm_check_cb_| is null, e.g. ShouldCRC() never called? No. The RemotingCdmController will only call StartRemoting and get is OnStarted() call when ShouldCRC() is called and |cdm_check_cb_| is not null. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.cc:39: remoting_source_->Shutdown(); On 2016/11/01 08:21:28, xhwang wrote: > It's a bit confusing to me on the ownership of |remoting_source_|. It seems we > can pass a remoting_source out by remoting_source(). Then who should call > Shutdown()? After SetCdm() is called, |remoting_source_| is owned by both RemotingCdmController and RemotingRendererController. Shutdown() will only be called from RemotingCdmController though. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.cc:56: remoting_source_->StartRemoting(this); On 2016/11/01 16:52:37, xhwang wrote: > For the majority (say >99%) of playback sessions where user is not casting the > tab, |is_remoting_| will always be false, so we'll always end up calling > StartRemoting() which is a mojo IPC to the browser and wait for the response > back. This could increase the start-to-play latency. > > I think remoting should not introduce new latency for normal playbacks. To > achieve this, is it possible to cache some values about whether the tab is being > casted in the render process, and if it's false, do not try remoting at all? > There's a slight chance of race here that user just clicked to cast, and then > started a playback, so the render process side cache is not updated, but I think > the chance for this to happen is very small, and even if it happens, we can > always fall back to mirroring. IIUC, this StartRemoting() call does not always end up a mojo IPC call. If remote sink is not available (e.g., not in mirroring), we will not start remoting, and will run the callback and return false immediately. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_controller.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.h:14: } On 2016/11/01 08:21:29, xhwang wrote: > This won't work and you are actually getting the declaration of ThreadChecker > from remoting_source_impl.h. Please remove this forward declaration and add > include directly. Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.h:34: using CdmCheckCallback = base::Callback<void(bool is_remoting)>; On 2016/11/01 08:21:29, xhwang wrote: > nit: Returns whether we should create remoting CDM via |cb|, which could be run > synchronously ..... Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.h:39: return remoting_source_.get(); On 2016/11/01 08:21:29, xhwang wrote: > See comment above... why are we passing raw pointers to a ref-counted object > around? See reply above. :) https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.h:48: // Indicates if remoting is started. On 2016/11/01 08:21:29, xhwang wrote: > what does "remoting is started" means exactly? e.g. remoting CDM created? This variable indicates if in remoting. i.e., remoting session is started. Modified the comment. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.h:52: // debug builds. On 2016/11/01 08:21:28, xhwang wrote: > nit: This comment seems unnecessary :) But it's up to you. Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:30: NOTIMPLEMENTED(); On 2016/11/01 08:21:29, xhwang wrote: > If we land this CL now, when we are "remoting" (not sure about the exact > meaning), protected content playback will fail. Is this correct? Could this > actually happen in prod? > > Can we land a CL to add a command line flag first before we enable remoting by > default? This will help avoid unnecessary noise/stress to you, other devs, and > users. > No, we will never enable "remoting" if we land this CL now. And I will add the command line flag in a separate CL soon. :) https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:31: } else { On 2016/11/01 08:21:29, xhwang wrote: > nit: we like to return early: > > if (foo) { > Bar(); > return; > } > > Qux(); This seems fine to me, as we will merge with the RemotingCdm implementation very soon. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:66: // TODO: Replace the callbacks with an interface. http://crbug.com/657940. On 2016/11/01 08:21:29, xhwang wrote: > nit: Usually you have a name/ladp for the TODO :) Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:78: RemotingCdmController* remoting_controller_ptr = remoting_controller.get(); On 2016/11/01 08:21:29, xhwang wrote: > nit: It's worth a comment why you are doing this. Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:83: default_cdm_factory_.get())); On 2016/11/01 08:21:29, xhwang wrote: > Is it possible that the callback is fired after |this| is destroyed? If so, > default_cdm_factory will be destroyed as well when the callback fires. It's > probably easier if CreateCdm() is a member function and we use a weakptr to make > sure the callback won't fire after |this| is destroyed. > > If you do that, you can also store a pending_remoting_controller_ instead of > passing of ownership of the |remoting_controller| to the callback, which IMHO is > cleaner, but it's totally up to you. Changed CreateCdm() as a member function of RemotingCdmFactory. But the |remoting_controller| and other parameters other than |default_cdm_factor_| still need to be passed, as the RemotingCdmFactory is shared by the whole RenderFrameImpl. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:14: RemotingRendererController::RemotingRendererController( On 2016/11/01 08:21:29, xhwang wrote: > For future CL, please consider do renames in a separate CL. Rename CLs will be > super trivial to review, then this CL will be much less noisy. > > If you have to do renames in the same CL, please consider upload a rename-only > patchset first, then upload real changes in a different patchset. This way, > it'll be much easier to see the real changes by looking at patchset differences. > > Now for this file, it's pretty hard to know what has been changed. I can get a > better view by looking at the diff against PS1, but PS1 may already contain > other changes (since it's PS4 in the previous CL). Acknowledged. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:66: auto* cdm = RemotingCdm::From(cdm_context); On 2016/11/01 08:21:29, xhwang wrote: > Is GetRemotingSource() the only thing you need from the CDM? The purpose that we > have CdmContext is to isolate the media player (e.g. renderer) from accessing > the CDM directly. Therefore, it's more desirable that we cast CdmContext to a > RemotingCdmContext here, and get GetRemotingSource() there, instead of getting > the CDM from the context, which defies the purpose of CdmContext. > > You can see MediaDrmBridgeCdmContext for an example. Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_renderer_controller.h:55: RemotingSourceImpl* remoting_source() const { On 2016/11/01 08:21:29, xhwang wrote: > ditto about passing raw pointer around... Please see my reply to the first comment on this. :) https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_renderer_controller_unittest.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_renderer_controller_unittest.cc:30: data.video_decoder_config = VideoDecoderConfig( On 2016/11/01 08:21:29, xhwang wrote: > You can use TestVideoConfig::Normal(), etc: > > https://cs.chromium.org/chromium/src/media/base/test_helpers.h?rcl=0&l=81 > > Unfortunately we don't have an audio one yet :( Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_renderer_controller_unittest.cc:48: EmptyExtraData(), AesCtrEncryptionScheme()); On 2016/11/01 08:21:29, xhwang wrote: > ditto, you can use TestVideoConfig::NormalEncrypted Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_source_impl.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:62: client->OnStarted(true); On 2016/11/01 08:21:29, xhwang wrote: > Actually you notify all clients, so the comment is a bit misleading. In general, no. Only when there are requests from multiple clients to StartRemoting() at almost same time while the session is not started yet, then all these clients will get notified when the session is started. If the session is already started, only the client that call StartRemoting() will get notified. Note: The same session is shared by multiple (might be 2 in our case) clients. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:121: client->OnStarted(false); On 2016/11/01 08:21:29, xhwang wrote: > hmm, now I see why you have |client|... > > Does it make sense to make sure clients call StartRemoting() only in the right > state (CAN_START)? That may make the switching logic more complicate. Both CdmController and RendererController share same session, and the session might be already started (SESSION_STARTED) when create remoting cdm, the current logic allow RendererController call StartRemoting() to switch to remoting renderer, just same as that for non-EME case. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:143: return; On 2016/11/01 08:21:29, xhwang wrote: > If this happens, it'll be a programming error... DCHECK? Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:153: return; On 2016/11/01 08:21:29, xhwang wrote: > ditto, DCHECK? Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_source_impl.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.h:22: SESSION_STARTING, On 2016/11/01 08:21:29, xhwang wrote: > if "starting a remoting session" failed, what state will we end up with? It could be either SESSION_UNAVAILABLE, or SESSION_PERMANENTLY_STOPPED. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.h:26: SESSION_STOPPING, On 2016/11/01 08:21:30, xhwang wrote: > Can we get back to STARTED after STOPPING? > > More generally, can you describe a state machine about possible transitions? Done. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.h:88: void AddClient(Client* client); On 2016/11/01 08:21:29, xhwang wrote: > It's odd that a Client can be added here, but is also specified in > StartRemoting() and StopRemoting(). It's more confusing that for > StartRemoting(), only |client| will be notified, even though all clients > implemented OnStarted(). Then for StopRemoting(), actually all clients will get > notified. > > Does it make sense to simply this so that we only add client through > AddClient(), then for StartRemoting() and StopRemoting(), all clients will get > notified? If this doesn't work, you can add a callback for StartRemoting(): > > StartRemoting(const OnStartedCB& cb); > Stopremoting(); > AddClient(Client* client); > // Remove OnStarted() from the Client interface. Clients can only be added/removed through the Add/RemoveClient() functions. StartRemoting() or StopRemoting() will not add/remove client. The parameter |client| is just the client that requires to start/stop remoting. e.g., If one client call StartRemoting() and the remoting is already started (might by another client), that client will simply get notified by calling OnStarted(true). Other clients will not get notified in this case. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.h:88: void AddClient(Client* client); On 2016/11/01 08:21:29, xhwang wrote: > Who owns the |client|? Maybe worth commenting that clients are supposed to call > RemoveClient() before it's gone. This class doesn't own the clients. Added comment.
In the future please address comments first, then rebase in a separate PS. Now it's hard to see the diff and comments without the rebase noise ...
Thanks! I just have a few followup questions for discussion. Some comment could be in an old PS. https://codereview.chromium.org/2457563002/diff/190001/media/base/cdm_context.h File media/base/cdm_context.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/base/cdm_context... media/base/cdm_context.h:38: // method to provide safe down-casting to their type. On 2016/11/01 21:55:52, xjz wrote: > On 2016/11/01 08:21:28, xhwang wrote: > > If I add a new CdmContext, how should I implement this method? > > The implementation of this is up to each implementation of CdmContext. If not > implemented, this will return null ptr by default. +miu But how "unique" is unique? The current impl only guarantees that the "id" will be unique in the current process. Two instances of the same CDM running in two processes would could get two different ID, and two different CDMs could get the same ID when running in two processes (though the chance is low)... I guess a normal developer have to look at how RemotingCdm implements this to be able to implement this in another CDM, which means this interface isn't complete form an API's perspective. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.cc:13: void* const kClassIdentifier = const_cast<void**>(&kClassIdentifier); On 2016/11/01 21:55:52, xjz wrote: > On 2016/11/01 08:21:28, xhwang wrote: > > This will work, but I wonder how is this better than less fancy approaches > like > > having a fixed unique id (e.g. name-like string, or GUID)? It seems as long as > > the ID is unique it'll work. > > I used the same way as here: > https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/processed_... > I do think this is a very smart way to get a unique id. Though it is not very > straight forward when I first saw this. :) +miu yeah, I did code search and found this pattern. As commented above, this works. But I wonder whether we have to be this "smart" given we only need a unique ID. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:32: RemotingSourceImpl* GetRemotingSource(); On 2016/11/01 21:55:52, xjz wrote: > On 2016/11/01 08:21:28, xhwang wrote: > > By looking at code below, it seems RSI is ref-counted. We should not pass > around > > raw-pointers to a ref-counted object... > > The below was commented by miu: > General scoped_refptr usage note: The return type of a direct accessor should be > the raw pointer (RemotingSourceImpl*) instead. This lets the caller decide > whether to increment the ref-count by storing the raw pointer into a > scoped_refptr. > > +miu hmm, this seems a pattern that is not recommended now: https://www.chromium.org/developers/smart-pointer-guidelines If a function takes or returns a raw pointer, it may mean no ownership is transferred, or it may not. Much of Chromium was written before std::unique_ptr<> existed, or by people unfamiliar with using it to indicate ownership transfers, and thus takes or returns raw pointers but transfers ownership in the process. Because the compiler can't enforce correct behavior here, this is less safe. Consider cleaning up such code, so that functions which take or return raw pointers never transfer ownership. This is more for unique_ptr, but I feel the idea also applies to scoped_refptr... https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_controller.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.cc:56: remoting_source_->StartRemoting(this); On 2016/11/01 21:55:52, xjz wrote: > On 2016/11/01 16:52:37, xhwang wrote: > > For the majority (say >99%) of playback sessions where user is not casting the > > tab, |is_remoting_| will always be false, so we'll always end up calling > > StartRemoting() which is a mojo IPC to the browser and wait for the response > > back. This could increase the start-to-play latency. > > > > I think remoting should not introduce new latency for normal playbacks. To > > achieve this, is it possible to cache some values about whether the tab is > being > > casted in the render process, and if it's false, do not try remoting at all? > > There's a slight chance of race here that user just clicked to cast, and then > > started a playback, so the render process side cache is not updated, but I > think > > the chance for this to happen is very small, and even if it happens, we can > > always fall back to mirroring. > > IIUC, this StartRemoting() call does not always end up a mojo IPC call. If > remote sink is not available (e.g., not in mirroring), we will not start > remoting, and will run the callback and return false immediately. I see. Thanks for pointing this out. So now it seems we can have this in one big callstack: RemotingCdmFactory::Create() CreateRemotingCdmController(); remoter_factory_->Create(std::move(remoting_source),.... new RemotingSourceImpl... remoting_cdm_controller_ptr->ShouldCreateRemotingCdm()... remoting_source_->StartRemoting(this); case SESSION_UNAVAILABLE: // this is the default state, and we are not getting sink availability info from browser yet client->OnStarted(false); CreateCdm(..., false); // so we'll always use the default CDM If this is true, then we'll never create the RemotingCdm. We should probably break this big callstack to wait for sink availability update, but then CDM create will be delayed since we are waiting for browser to notify us. I feel the root problem is that we only create RemotingSourceImpl when we are about to create the CDM. Someone in the render process should probably try to get info about whether the tab is being casted as soon as it's available at the RenderProcess level... FWIW, I have no idea about what kind of latency I am talking about. If we have data to prove it's negligible then we are probably still fine. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_source_impl.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.h:88: void AddClient(Client* client); On 2016/11/01 21:55:53, xjz wrote: > On 2016/11/01 08:21:29, xhwang wrote: > > It's odd that a Client can be added here, but is also specified in > > StartRemoting() and StopRemoting(). It's more confusing that for > > StartRemoting(), only |client| will be notified, even though all clients > > implemented OnStarted(). Then for StopRemoting(), actually all clients will > get > > notified. > > > > Does it make sense to simply this so that we only add client through > > AddClient(), then for StartRemoting() and StopRemoting(), all clients will get > > notified? If this doesn't work, you can add a callback for StartRemoting(): > > > > StartRemoting(const OnStartedCB& cb); > > Stopremoting(); > > AddClient(Client* client); > > // Remove OnStarted() from the Client interface. > > Clients can only be added/removed through the Add/RemoveClient() functions. > StartRemoting() or StopRemoting() will not add/remove client. The parameter > |client| is just the client that requires to start/stop remoting. e.g., If one > client call StartRemoting() and the remoting is already started (might by > another client), that client will simply get notified by calling > OnStarted(true). Other clients will not get notified in this case. This is not documented (and is a bit confusing)... Also, at least in the StopRemoting() case, |client| isn't used at all... https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... File media/remoting/remoting_cdm_context.h (right): https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... media/remoting/remoting_cdm_context.h:18: RemotingCdmContext(RemotingCdm* remoting_cdm); explicit https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... media/remoting/remoting_cdm_context.h:33: RemotingCdm* const remoting_cdm_; Please add a comment about lifetime. https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.cc (right): https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:79: VLOG(1) << "Create local CDM."; The CDM could run in ppapi or other process so it's not really local. s/local/default/? https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:66: auto* cdm = RemotingCdmContext::From(cdm_context); s/cdm/remoting_cdm_context, strictly speaking it's now a CDM
On 2016/11/02 05:22:37, xhwang wrote: > In the future please address comments first, then rebase in a separate PS. Now > it's hard to see the diff and comments without the rebase noise ... Sorry, I did rebase before receiving your comments, and this rebase is not trivial. I will try address comments first next time anyway.
Addressed xhwang's comments. PTAL. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:32: RemotingSourceImpl* GetRemotingSource(); On 2016/11/02 06:47:05, xhwang wrote: > On 2016/11/01 21:55:52, xjz wrote: > > On 2016/11/01 08:21:28, xhwang wrote: > > > By looking at code below, it seems RSI is ref-counted. We should not pass > > around > > > raw-pointers to a ref-counted object... > > > > The below was commented by miu: > > General scoped_refptr usage note: The return type of a direct accessor should > be > > the raw pointer (RemotingSourceImpl*) instead. This lets the caller decide > > whether to increment the ref-count by storing the raw pointer into a > > scoped_refptr. > > > > +miu > > hmm, this seems a pattern that is not recommended now: > > https://www.chromium.org/developers/smart-pointer-guidelines > > If a function takes or returns a raw pointer, it may mean no ownership is > transferred, or it may not. Much of Chromium was written before > std::unique_ptr<> existed, or by people unfamiliar with using it to indicate > ownership transfers, and thus takes or returns raw pointers but transfers > ownership in the process. Because the compiler can't enforce correct behavior > here, this is less safe. Consider cleaning up such code, so that functions which > take or return raw pointers never transfer ownership. > > This is more for unique_ptr, but I feel the idea also applies to > scoped_refptr... +miu Here is the coding style: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... It seems that we don't need to return raw pointer after c++11. WDYT? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_controller.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.cc:56: remoting_source_->StartRemoting(this); On 2016/11/02 06:47:05, xhwang wrote: > On 2016/11/01 21:55:52, xjz wrote: > > On 2016/11/01 16:52:37, xhwang wrote: > > > For the majority (say >99%) of playback sessions where user is not casting > the > > > tab, |is_remoting_| will always be false, so we'll always end up calling > > > StartRemoting() which is a mojo IPC to the browser and wait for the response > > > back. This could increase the start-to-play latency. > > > > > > I think remoting should not introduce new latency for normal playbacks. To > > > achieve this, is it possible to cache some values about whether the tab is > > being > > > casted in the render process, and if it's false, do not try remoting at all? > > > There's a slight chance of race here that user just clicked to cast, and > then > > > started a playback, so the render process side cache is not updated, but I > > think > > > the chance for this to happen is very small, and even if it happens, we can > > > always fall back to mirroring. > > > > IIUC, this StartRemoting() call does not always end up a mojo IPC call. If > > remote sink is not available (e.g., not in mirroring), we will not start > > remoting, and will run the callback and return false immediately. > > I see. Thanks for pointing this out. > > So now it seems we can have this in one big callstack: > > RemotingCdmFactory::Create() > CreateRemotingCdmController(); > remoter_factory_->Create(std::move(remoting_source),.... > new RemotingSourceImpl... > remoting_cdm_controller_ptr->ShouldCreateRemotingCdm()... > remoting_source_->StartRemoting(this); > case SESSION_UNAVAILABLE: // this is the default state, and we are not > getting sink availability info from browser yet > client->OnStarted(false); > CreateCdm(..., false); // so we'll always use the default CDM > > If this is true, then we'll never create the RemotingCdm. We should probably > break this big callstack to wait for sink availability update, but then CDM > create will be delayed since we are waiting for browser to notify us. > > I feel the root problem is that we only create RemotingSourceImpl when we are > about to create the CDM. Someone in the render process should probably try to > get info about whether the tab is being casted as soon as it's available at the > RenderProcess level... > > FWIW, I have no idea about what kind of latency I am talking about. If we have > data to prove it's negligible then we are probably still fine. +miu We currently only create the RemotingSourceImpl when Create() is called is because the CdmFactory is shared by the whole RenderFrameImpl. Does this sound a problem? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_source_impl.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_source_impl.h:88: void AddClient(Client* client); On 2016/11/02 06:47:05, xhwang wrote: > On 2016/11/01 21:55:53, xjz wrote: > > On 2016/11/01 08:21:29, xhwang wrote: > > > It's odd that a Client can be added here, but is also specified in > > > StartRemoting() and StopRemoting(). It's more confusing that for > > > StartRemoting(), only |client| will be notified, even though all clients > > > implemented OnStarted(). Then for StopRemoting(), actually all clients will > > get > > > notified. > > > > > > Does it make sense to simply this so that we only add client through > > > AddClient(), then for StartRemoting() and StopRemoting(), all clients will > get > > > notified? If this doesn't work, you can add a callback for StartRemoting(): > > > > > > StartRemoting(const OnStartedCB& cb); > > > Stopremoting(); > > > AddClient(Client* client); > > > // Remove OnStarted() from the Client interface. > > > > Clients can only be added/removed through the Add/RemoveClient() functions. > > StartRemoting() or StopRemoting() will not add/remove client. The parameter > > |client| is just the client that requires to start/stop remoting. e.g., If one > > client call StartRemoting() and the remoting is already started (might by > > another client), that client will simply get notified by calling > > OnStarted(true). Other clients will not get notified in this case. > > This is not documented (and is a bit confusing)... > > Also, at least in the StopRemoting() case, |client| isn't used at all... Added comments. In StopRemoting() case, |client| is used to confirm that it is a valid client that requires stopping remoting. https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... File media/remoting/remoting_cdm_context.h (right): https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... media/remoting/remoting_cdm_context.h:18: RemotingCdmContext(RemotingCdm* remoting_cdm); On 2016/11/02 06:47:05, xhwang wrote: > explicit Done. https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... media/remoting/remoting_cdm_context.h:33: RemotingCdm* const remoting_cdm_; On 2016/11/02 06:47:05, xhwang wrote: > Please add a comment about lifetime. Done. https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.cc (right): https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:79: VLOG(1) << "Create local CDM."; On 2016/11/02 06:47:05, xhwang wrote: > The CDM could run in ppapi or other process so it's not really local. > s/local/default/? Done. https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/290001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:66: auto* cdm = RemotingCdmContext::From(cdm_context); On 2016/11/02 06:47:05, xhwang wrote: > s/cdm/remoting_cdm_context, strictly speaking it's now a CDM Done.
I'll re-review tomorrow. For now, some comments that'll hopefully help resolve on-going concerns: https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:32: RemotingSourceImpl* GetRemotingSource(); On 2016/11/02 17:38:11, xjz wrote: > On 2016/11/02 06:47:05, xhwang wrote: > > On 2016/11/01 21:55:52, xjz wrote: > > > On 2016/11/01 08:21:28, xhwang wrote: > > > > By looking at code below, it seems RSI is ref-counted. We should not pass > > > around > > > > raw-pointers to a ref-counted object... > > > > > > The below was commented by miu: > > > General scoped_refptr usage note: The return type of a direct accessor > should > > be > > > the raw pointer (RemotingSourceImpl*) instead. This lets the caller decide > > > whether to increment the ref-count by storing the raw pointer into a > > > scoped_refptr. > > > > > > +miu > > > > hmm, this seems a pattern that is not recommended now: > > > > https://www.chromium.org/developers/smart-pointer-guidelines > > > > If a function takes or returns a raw pointer, it may mean no ownership is > > transferred, or it may not. Much of Chromium was written before > > std::unique_ptr<> existed, or by people unfamiliar with using it to indicate > > ownership transfers, and thus takes or returns raw pointers but transfers > > ownership in the process. Because the compiler can't enforce correct behavior > > here, this is less safe. Consider cleaning up such code, so that functions > which > > take or return raw pointers never transfer ownership. > > > > This is more for unique_ptr, but I feel the idea also applies to > > scoped_refptr... > > +miu > > Here is the coding style: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > It seems that we don't need to return raw pointer after c++11. WDYT? This is the text guiding this decision: "Conventions for return values are similar: return raw pointers when the caller does not take ownership, and return smart pointers by value otherwise, potentially in conjunction with std::move()." So, does the caller take ownership? If not, raw pointer. If so, scoped_refptr<>. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_controller.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.cc:56: remoting_source_->StartRemoting(this); On 2016/11/02 17:38:11, xjz wrote: > On 2016/11/02 06:47:05, xhwang wrote: > > On 2016/11/01 21:55:52, xjz wrote: > > > On 2016/11/01 16:52:37, xhwang wrote: > > > > For the majority (say >99%) of playback sessions where user is not casting > > the > > > > tab, |is_remoting_| will always be false, so we'll always end up calling > > > > StartRemoting() which is a mojo IPC to the browser and wait for the > response > > > > back. This could increase the start-to-play latency. > > > > > > > > I think remoting should not introduce new latency for normal playbacks. To > > > > achieve this, is it possible to cache some values about whether the tab is > > > being > > > > casted in the render process, and if it's false, do not try remoting at > all? > > > > There's a slight chance of race here that user just clicked to cast, and > > then > > > > started a playback, so the render process side cache is not updated, but I > > > think > > > > the chance for this to happen is very small, and even if it happens, we > can > > > > always fall back to mirroring. > > > > > > IIUC, this StartRemoting() call does not always end up a mojo IPC call. If > > > remote sink is not available (e.g., not in mirroring), we will not start > > > remoting, and will run the callback and return false immediately. > > > > I see. Thanks for pointing this out. > > > > So now it seems we can have this in one big callstack: > > > > RemotingCdmFactory::Create() > > CreateRemotingCdmController(); > > remoter_factory_->Create(std::move(remoting_source),.... > > new RemotingSourceImpl... > > remoting_cdm_controller_ptr->ShouldCreateRemotingCdm()... > > remoting_source_->StartRemoting(this); > > case SESSION_UNAVAILABLE: // this is the default state, and we are not > > getting sink availability info from browser yet > > client->OnStarted(false); > > CreateCdm(..., false); // so we'll always use the default CDM > > > > If this is true, then we'll never create the RemotingCdm. We should probably > > break this big callstack to wait for sink availability update, but then CDM > > create will be delayed since we are waiting for browser to notify us. > > > > I feel the root problem is that we only create RemotingSourceImpl when we are > > about to create the CDM. Someone in the render process should probably try to > > get info about whether the tab is being casted as soon as it's available at > the > > RenderProcess level... > > > > FWIW, I have no idea about what kind of latency I am talking about. If we have > > data to prove it's negligible then we are probably still fine. > > +miu > > We currently only create the RemotingSourceImpl when Create() is called is > because the CdmFactory is shared by the whole RenderFrameImpl. Does this sound a > problem? xhwang brings up a good point. What if we created the RemotingSourceImpl when the factory is created (which is long before this RemotingCdmController)? That way, it would get the OnSinkAvailable/Gone() notifications from the browser process long before RemotingCdmController is created. Then, in this method, we could simply: if (remoting_source_->state() == STATE_UNAVAILABLE) { // Do local CDM! } else if (not stopped) { // Do remote CDM! } https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:30: NOTIMPLEMENTED(); On 2016/11/01 21:55:53, xjz wrote: > On 2016/11/01 08:21:29, xhwang wrote: > > If we land this CL now, when we are "remoting" (not sure about the exact > > meaning), protected content playback will fail. Is this correct? Could this > > actually happen in prod? > > > > Can we land a CL to add a command line flag first before we enable remoting by > > default? This will help avoid unnecessary noise/stress to you, other devs, and > > users. > > > > No, we will never enable "remoting" if we land this CL now. > And I will add the command line flag in a separate CL soon. :) I talked about this issue here: https://codereview.chromium.org/2470003002/#msg5 tl;dr: Nothing's enabled yet. Let's do the feature flag in browser code (a single point at a higher level where all code paths can be (de)activated).
Addressed comments. PTAL. https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_controller.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_controller.cc:56: remoting_source_->StartRemoting(this); On 2016/11/03 00:03:08, miu wrote: > On 2016/11/02 17:38:11, xjz wrote: > > On 2016/11/02 06:47:05, xhwang wrote: > > > On 2016/11/01 21:55:52, xjz wrote: > > > > On 2016/11/01 16:52:37, xhwang wrote: > > > > > For the majority (say >99%) of playback sessions where user is not > casting > > > the > > > > > tab, |is_remoting_| will always be false, so we'll always end up calling > > > > > StartRemoting() which is a mojo IPC to the browser and wait for the > > response > > > > > back. This could increase the start-to-play latency. > > > > > > > > > > I think remoting should not introduce new latency for normal playbacks. > To > > > > > achieve this, is it possible to cache some values about whether the tab > is > > > > being > > > > > casted in the render process, and if it's false, do not try remoting at > > all? > > > > > There's a slight chance of race here that user just clicked to cast, and > > > then > > > > > started a playback, so the render process side cache is not updated, but > I > > > > think > > > > > the chance for this to happen is very small, and even if it happens, we > > can > > > > > always fall back to mirroring. > > > > > > > > IIUC, this StartRemoting() call does not always end up a mojo IPC call. If > > > > remote sink is not available (e.g., not in mirroring), we will not start > > > > remoting, and will run the callback and return false immediately. > > > > > > I see. Thanks for pointing this out. > > > > > > So now it seems we can have this in one big callstack: > > > > > > RemotingCdmFactory::Create() > > > CreateRemotingCdmController(); > > > remoter_factory_->Create(std::move(remoting_source),.... > > > new RemotingSourceImpl... > > > remoting_cdm_controller_ptr->ShouldCreateRemotingCdm()... > > > remoting_source_->StartRemoting(this); > > > case SESSION_UNAVAILABLE: // this is the default state, and we are > not > > > getting sink availability info from browser yet > > > client->OnStarted(false); > > > CreateCdm(..., false); // so we'll always use the default CDM > > > > > > If this is true, then we'll never create the RemotingCdm. We should probably > > > break this big callstack to wait for sink availability update, but then CDM > > > create will be delayed since we are waiting for browser to notify us. > > > > > > I feel the root problem is that we only create RemotingSourceImpl when we > are > > > about to create the CDM. Someone in the render process should probably try > to > > > get info about whether the tab is being casted as soon as it's available at > > the > > > RenderProcess level... > > > > > > FWIW, I have no idea about what kind of latency I am talking about. If we > have > > > data to prove it's negligible then we are probably still fine. > > > > +miu > > > > We currently only create the RemotingSourceImpl when Create() is called is > > because the CdmFactory is shared by the whole RenderFrameImpl. Does this sound > a > > problem? > > xhwang brings up a good point. What if we created the RemotingSourceImpl when > the factory is created (which is long before this RemotingCdmController)? That > way, it would get the OnSinkAvailable/Gone() notifications from the browser > process long before RemotingCdmController is created. Then, in this method, we > could simply: > > if (remoting_source_->state() == STATE_UNAVAILABLE) { > // Do local CDM! > } else if (not stopped) { > // Do remote CDM! > } Yes, we can do that. So there will be always a spare RemoutingSourceImpl living in RemotingCdmFactory. We don't need these codes though. The condition will be checked in RemotingSourceImpl::StartRemoting().
https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm.h (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm.h:32: RemotingSourceImpl* GetRemotingSource(); On 2016/11/03 00:03:08, miu wrote: > On 2016/11/02 17:38:11, xjz wrote: > > On 2016/11/02 06:47:05, xhwang wrote: > > > On 2016/11/01 21:55:52, xjz wrote: > > > > On 2016/11/01 08:21:28, xhwang wrote: > > > > > By looking at code below, it seems RSI is ref-counted. We should not > pass > > > > around > > > > > raw-pointers to a ref-counted object... > > > > > > > > The below was commented by miu: > > > > General scoped_refptr usage note: The return type of a direct accessor > > should > > > be > > > > the raw pointer (RemotingSourceImpl*) instead. This lets the caller decide > > > > whether to increment the ref-count by storing the raw pointer into a > > > > scoped_refptr. > > > > > > > > +miu > > > > > > hmm, this seems a pattern that is not recommended now: > > > > > > https://www.chromium.org/developers/smart-pointer-guidelines > > > > > > If a function takes or returns a raw pointer, it may mean no ownership is > > > transferred, or it may not. Much of Chromium was written before > > > std::unique_ptr<> existed, or by people unfamiliar with using it to indicate > > > ownership transfers, and thus takes or returns raw pointers but transfers > > > ownership in the process. Because the compiler can't enforce correct > behavior > > > here, this is less safe. Consider cleaning up such code, so that functions > > which > > > take or return raw pointers never transfer ownership. > > > > > > This is more for unique_ptr, but I feel the idea also applies to > > > scoped_refptr... > > > > +miu > > > > Here is the coding style: > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > It seems that we don't need to return raw pointer after c++11. WDYT? > > This is the text guiding this decision: "Conventions for return values are > similar: return raw pointers when the caller does not take ownership, and return > smart pointers by value otherwise, potentially in conjunction with std::move()." > > So, does the caller take ownership? If not, raw pointer. If so, scoped_refptr<>. Agreed. But in the comment above, xjz@ said "the returned value is stored to a scoped_refptr". In this case we should return scoped_refptr<> instead of the raw pointer? https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.cc (right): https://codereview.chromium.org/2457563002/diff/190001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:30: NOTIMPLEMENTED(); On 2016/11/03 00:03:08, miu wrote: > On 2016/11/01 21:55:53, xjz wrote: > > On 2016/11/01 08:21:29, xhwang wrote: > > > If we land this CL now, when we are "remoting" (not sure about the exact > > > meaning), protected content playback will fail. Is this correct? Could this > > > actually happen in prod? > > > > > > Can we land a CL to add a command line flag first before we enable remoting > by > > > default? This will help avoid unnecessary noise/stress to you, other devs, > and > > > users. > > > > > > > No, we will never enable "remoting" if we land this CL now. > > And I will add the command line flag in a separate CL soon. :) > > I talked about this issue here: https://codereview.chromium.org/2470003002/#msg5 > > tl;dr: Nothing's enabled yet. Let's do the feature flag in browser code (a > single point at a higher level where all code paths can be (de)activated). Makes sense. As long as we always pick the default Renderer/CDM for now we are good. When it's possible to pick remoting Renderer/CDM we should have a "feature" for it.
Patchset #10 (id:350001) has been deleted
xhwang and miu: In PS# 10, I add a RemotingSinkObserver to monitor the remoting sink availablity in RenderFrameImpl. This is used to initialize RemotingSourceImpl to avoid the delay of OnSinkAvailable() call from browser. PTAL. Thanks!
Description was changed from ========== Media Remoting: Add romoting control logic for encrypted contents. This CL does the following: 1. Adds a RemotingCdmController to control whether to create CDM remotely for Media Remoting. 2. Renames the RemotingController to RemotingRendererController. 3. Separates the implementation of RemotingSource from RemotingRenderController to RemotingSourceImpl. After OnSetCdm() is called, one RemotingSourceImpl is shared by both RemotingRenderController and RemotingCdmController. BUG=643964 ========== to ========== Media Remoting: Add remoting control logic for encrypted contents. This CL does the following: 1. Adds a RemotingCdmController to control whether to create CDM remotely for Media Remoting. 2. Renames the RemotingController to RemotingRendererController. 3. Separates the implementation of RemotingSource from RemotingRenderController to RemotingSourceImpl. After OnSetCdm() is called, one RemotingSourceImpl is shared by both RemotingRenderController and RemotingCdmController. BUG=643964 ==========
lgtm % nits https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_cdm_context.h (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_cdm_context.h:25: RemotingSourceImpl* GetRemotingSource(); based on the discussion, please update this to return a scoped_refptr, since the caller will take ownership https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.cc (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:62: remoting_cdm_controller_ptr->ShouldCreateRemotingCdm(base::Bind( Can we check sink_observer_->is_sink_available() here and if it's false then call CreateCdm() directly? Or add a comment that if sink is not available we'll call CreateCdm() immediately.
Very nice work! This is a very complex problem to solve! :) Almost there; comments on PS11: https://codereview.chromium.org/2457563002/diff/390001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2457563002/diff/390001/content/renderer/rende... content/renderer/render_frame_impl.cc:2746: if (remoting_sink_observer_->is_sink_available()) Let's not do this for the Renderer: 1. Let the RemotingRendererController wait to get the OnSinkAvailable() notification (and this would also give the RemotingCdmController a chance to start the remoting session). 2. This is extra logic in RenderFrameImpl. We should try to do nothing more than creating/gluing objects together in RenderFrameImpl. https://codereview.chromium.org/2457563002/diff/390001/content/renderer/rende... content/renderer/render_frame_impl.cc:6450: #if BUILDFLAG(ENABLE_MEDIA_REMOTING) nit: Please eliminate duplicate code and simplify: #if BUILDFLAG(ENABLE_PEPPER_CDMS) DCHECK(frame_); cdm_factory_.reset( new RenderCdmFactory(base::Bind(&PepperCdmWrapperImpl::Create, frame_))); #endif // BUILDFLAG(ENABLE_PEPPER_CDMS) #if BUILDFLAG(ENABLE_MEDIA_REMOTING) cdm_factory_.reset(new media::RemotingCdmFactory(std::move(cdm_factory_), GetRemoterFactory(), remoting_sink_observer_.get())); #endif // BUILDFLAG(ENABLE_MEDIA_REMOTING) https://codereview.chromium.org/2457563002/diff/390001/media/remoting/fake_re... File media/remoting/fake_remoting_controller.cc (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/fake_re... media/remoting/fake_remoting_controller.cc:119: base::Bind(&FakeRemoter::StartFailed, weak_factory_.GetWeakPtr())); OOC, why were these changed to use WeakPtr? Can the test code not ensure all tasks have run before tear-down like it did before? https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.cc (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:40: // Initialize the sink availability according to |sink_observer_|. Please add to this comment: Explain that we are purposely copying the "available" state before the RemotingCdmController would naturally get the notification. Also, explain why we are doing this. (Pretend someone unfamiliar with this project will read this code 2+ years from now.) You might even want to start the comment with "HACK": // HACK: Copy-over the sink availability status from |sink_observer_| ...and why... https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:62: remoting_cdm_controller_ptr->ShouldCreateRemotingCdm(base::Bind( On 2016/11/05 02:39:58, xhwang wrote: > Can we check sink_observer_->is_sink_available() here and if it's false then > call CreateCdm() directly? Or add a comment that if sink is not available we'll > call CreateCdm() immediately. +1 https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.h (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_cdm_factory.h:52: RemotingSinkObserver* sink_observer_; // Outlives this class. const https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:89: return remoting_source_->GetRpcBroker(); Note: In a future change, we really should roll the RpcBroker into RemotingSourceImpl (or vice versa). There should just be one class handling the messaging to/from the browser process. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_source_impl.cc (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:19: rpc_broker_.reset(new remoting::RpcBroker( Please move this to the initializer list. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:99: LOG(ERROR) << "corrupted Rpc message"; We should shut down remoting if a message is corrupt. (It might have been a critical message.) https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:220: void RemotingSourceImpl::OnSendMessageToSink( naming nit: Just "SendMessageToSink()" since this isn't a subscribed/observed event handler. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_source_impl.h (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_source_impl.h:125: base::WeakPtr<remoting::RpcBroker> GetRpcBroker() const; WeakPtrs are not necessary. Just return a raw pointer here. See comment below. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_source_impl.h:139: std::unique_ptr<remoting::RpcBroker> rpc_broker_; Don't use a pointer to a heap-allocated object here. You can just compose the RpcBroker as a direct member of this RemotingSourceImpl: remoting::RpcBroker rpc_broker_; ...and this is why I mentioned that, in a future CL the RpcBroker should probably be merged into RemotingSourceImpl.
Thanks for detailed reviewing! xhwang@ and miu@: Addressed your comments in PS#12. PTAL. Thanks! https://codereview.chromium.org/2457563002/diff/390001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2457563002/diff/390001/content/renderer/rende... content/renderer/render_frame_impl.cc:2746: if (remoting_sink_observer_->is_sink_available()) On 2016/11/05 04:05:15, miu wrote: > Let's not do this for the Renderer: > > 1. Let the RemotingRendererController wait to get the OnSinkAvailable() > notification (and this would also give the RemotingCdmController a chance to > start the remoting session). > > 2. This is extra logic in RenderFrameImpl. We should try to do nothing more than > creating/gluing objects together in RenderFrameImpl. Done. And let |cdm_factory_| owns the |remoting_sink_observer_| now, since only |cdm_factor_| needs it. https://codereview.chromium.org/2457563002/diff/390001/content/renderer/rende... content/renderer/render_frame_impl.cc:6450: #if BUILDFLAG(ENABLE_MEDIA_REMOTING) On 2016/11/05 04:05:15, miu wrote: > nit: Please eliminate duplicate code and simplify: > > #if BUILDFLAG(ENABLE_PEPPER_CDMS) > DCHECK(frame_); > cdm_factory_.reset( > new RenderCdmFactory(base::Bind(&PepperCdmWrapperImpl::Create, frame_))); > #endif // BUILDFLAG(ENABLE_PEPPER_CDMS) > > #if BUILDFLAG(ENABLE_MEDIA_REMOTING) > cdm_factory_.reset(new media::RemotingCdmFactory(std::move(cdm_factory_), > GetRemoterFactory(), remoting_sink_observer_.get())); > #endif // BUILDFLAG(ENABLE_MEDIA_REMOTING) Done. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/fake_re... File media/remoting/fake_remoting_controller.cc (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/fake_re... media/remoting/fake_remoting_controller.cc:119: base::Bind(&FakeRemoter::StartFailed, weak_factory_.GetWeakPtr())); On 2016/11/05 04:05:15, miu wrote: > OOC, why were these changed to use WeakPtr? Can the test code not ensure all > tasks have run before tear-down like it did before? WeakPtr is needed because when the RemotingSourceImpl is destroyed, FakeRemoter will be destroyed as well due to strong binding. RemotingSourceImpl might be immediately destroyed after sending stop(), in which case the FakeRemoter is destroyed before calling FakeRemoter::StartFailed, and will crash. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_cdm_context.h (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_cdm_context.h:25: RemotingSourceImpl* GetRemotingSource(); On 2016/11/05 02:39:58, xhwang wrote: > based on the discussion, please update this to return a scoped_refptr, since the > caller will take ownership Based on previous discussion (comments), I decide to keep the raw pointer here, as this method does not mean to transfer the ownership, but get access to the RemotingSourceImpl. Although the return value is stored in another scoped_refptr, this RemotingCdmContext still owns this RemotingSourceImpl. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.cc (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:40: // Initialize the sink availability according to |sink_observer_|. On 2016/11/05 04:05:15, miu wrote: > Please add to this comment: Explain that we are purposely copying the > "available" state before the RemotingCdmController would naturally get the > notification. Also, explain why we are doing this. (Pretend someone unfamiliar > with this project will read this code 2+ years from now.) > > You might even want to start the comment with "HACK": > > // HACK: Copy-over the sink availability status from |sink_observer_| ...and > why... Done. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:62: remoting_cdm_controller_ptr->ShouldCreateRemotingCdm(base::Bind( On 2016/11/05 02:39:58, xhwang_OOO_11_07 wrote: > Can we check sink_observer_->is_sink_available() here and if it's false then > call CreateCdm() directly? Or add a comment that if sink is not available we'll > call CreateCdm() immediately. Done. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:62: remoting_cdm_controller_ptr->ShouldCreateRemotingCdm(base::Bind( On 2016/11/05 04:05:15, miu wrote: > On 2016/11/05 02:39:58, xhwang wrote: > > Can we check sink_observer_->is_sink_available() here and if it's false then > > call CreateCdm() directly? Or add a comment that if sink is not available > we'll > > call CreateCdm() immediately. > > +1 Done. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.h (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_cdm_factory.h:52: RemotingSinkObserver* sink_observer_; // Outlives this class. On 2016/11/05 04:05:15, miu wrote: > const Not applicable. Changed it to std::unique_ptr and let RemotingCdmFactory own it now. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:89: return remoting_source_->GetRpcBroker(); On 2016/11/05 04:05:15, miu wrote: > Note: In a future change, we really should roll the RpcBroker into > RemotingSourceImpl (or vice versa). There should just be one class handling the > messaging to/from the browser process. Ack. Added a TODO comment in RemotingSourceImpl.h. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_source_impl.cc (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:19: rpc_broker_.reset(new remoting::RpcBroker( On 2016/11/05 04:05:16, miu wrote: > Please move this to the initializer list. Done. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:99: LOG(ERROR) << "corrupted Rpc message"; On 2016/11/05 04:05:16, miu wrote: > We should shut down remoting if a message is corrupt. (It might have been a > critical message.) Done. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:220: void RemotingSourceImpl::OnSendMessageToSink( On 2016/11/05 04:05:16, miu wrote: > naming nit: Just "SendMessageToSink()" since this isn't a subscribed/observed > event handler. Done. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_source_impl.h (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_source_impl.h:125: base::WeakPtr<remoting::RpcBroker> GetRpcBroker() const; On 2016/11/05 04:05:16, miu wrote: > WeakPtrs are not necessary. Just return a raw pointer here. See comment below. Done. https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_source_impl.h:139: std::unique_ptr<remoting::RpcBroker> rpc_broker_; On 2016/11/05 04:05:16, miu wrote: > Don't use a pointer to a heap-allocated object here. You can just compose the > RpcBroker as a direct member of this RemotingSourceImpl: > > remoting::RpcBroker rpc_broker_; > > ...and this is why I mentioned that, in a future CL the RpcBroker should > probably be merged into RemotingSourceImpl. Done.
The CQ bit was checked by xjz@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...
PS12 lgtm % nits: https://codereview.chromium.org/2457563002/diff/410001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.cc (right): https://codereview.chromium.org/2457563002/diff/410001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:42: // avoid the possible delay on OnSinkAbailable() call from browser. Please fix typo: s/Abailable/Available/ https://codereview.chromium.org/2457563002/diff/410001/media/remoting/remotin... File media/remoting/remoting_source_impl.cc (right): https://codereview.chromium.org/2457563002/diff/410001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:218: return const_cast<remoting::RpcBroker*>(&rpc_broker_); nit: Please add a TODO comment for fixing const-correctness of RpcBroker. (This can be done in the future CL where RpcBroker is merged into this class.)
https://codereview.chromium.org/2457563002/diff/430001/media/remoting/remotin... File media/remoting/remoting_cdm_context.h (right): https://codereview.chromium.org/2457563002/diff/430001/media/remoting/remotin... media/remoting/remoting_cdm_context.h:16: class RemotingCdmContext : public CdmContext { I found it un-necessary to create separated class implementation for CdmContext. In fact this can be part of RemotingCdm and since this is defined as member variable in RemotingCdm(not using any pointer...smart poninter) it will be deleted only if calling RemotingCdm dtor. This is just a thought, because we still have to create additional interface to pass those information such cdm_id_ from RemotingCdm rpc from remote. and this is a light weight implementation. It makes me think we can combine both into single one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Addressed comments. Thanks for reviewing! https://codereview.chromium.org/2457563002/diff/410001/media/remoting/remotin... File media/remoting/remoting_cdm_factory.cc (right): https://codereview.chromium.org/2457563002/diff/410001/media/remoting/remotin... media/remoting/remoting_cdm_factory.cc:42: // avoid the possible delay on OnSinkAbailable() call from browser. On 2016/11/07 21:54:02, miu wrote: > Please fix typo: s/Abailable/Available/ Done. https://codereview.chromium.org/2457563002/diff/410001/media/remoting/remotin... File media/remoting/remoting_source_impl.cc (right): https://codereview.chromium.org/2457563002/diff/410001/media/remoting/remotin... media/remoting/remoting_source_impl.cc:218: return const_cast<remoting::RpcBroker*>(&rpc_broker_); On 2016/11/07 21:54:02, miu wrote: > nit: Please add a TODO comment for fixing const-correctness of RpcBroker. (This > can be done in the future CL where RpcBroker is merged into this class.) Done. https://codereview.chromium.org/2457563002/diff/430001/media/remoting/remotin... File media/remoting/remoting_cdm_context.h (right): https://codereview.chromium.org/2457563002/diff/430001/media/remoting/remotin... media/remoting/remoting_cdm_context.h:16: class RemotingCdmContext : public CdmContext { On 2016/11/07 22:03:15, erickung1 wrote: > I found it un-necessary to create separated class implementation for CdmContext. > In fact this can be part of RemotingCdm > > and since this is defined as member variable in RemotingCdm(not using any > pointer...smart poninter) > it will be deleted only if calling RemotingCdm dtor. > > This is just a thought, because we still have to create additional interface to > pass those information such cdm_id_ from RemotingCdm rpc from remote. and this > is a light weight implementation. It makes me think we can combine both into > single one. This is explicitly required by xhwang@. As he commented, the purpose to have a CdmContext is to not get Cdm from a CdmContext. :)
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org, xhwang@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2457563002/#ps450001 (title: "Addressed comments from PS#12. Fixed LSAN.")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:89: return remoting_source_->GetRpcBroker(); On 2016/11/07 19:03:55, xjz wrote: > On 2016/11/05 04:05:15, miu wrote: > > Note: In a future change, we really should roll the RpcBroker into > > RemotingSourceImpl (or vice versa). There should just be one class handling > the > > messaging to/from the browser process. > > Ack. Added a TODO comment in RemotingSourceImpl.h. Note: the RpcBroker is the utility class that was designed to be used on both sender and receiver. Therefore, I think we need to think about the benefit of merging this into RemotingSourceImpl as that is not ruiqured on the receiver side
https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... File media/remoting/remoting_cdm_context.h (right): https://codereview.chromium.org/2457563002/diff/390001/media/remoting/remotin... media/remoting/remoting_cdm_context.h:25: RemotingSourceImpl* GetRemotingSource(); On 2016/11/07 19:03:54, xjz wrote: > On 2016/11/05 02:39:58, xhwang wrote: > > based on the discussion, please update this to return a scoped_refptr, since > the > > caller will take ownership > > Based on previous discussion (comments), I decide to keep the raw pointer here, > as this method does not mean to transfer the ownership, but get access to the > RemotingSourceImpl. Although the return value is stored in another > scoped_refptr, this RemotingCdmContext still owns this RemotingSourceImpl. When the returned value is stored in another scoped_refptr, the caller is a shared owner. If the caller doesn't need to take the shared ownership, it should just keep using the raw pointer. In other words, you should pretty much never wrap a raw pointer of a ref-counted object into a scoped_refptr after creation time.
The CQ bit was checked by xjz@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: Add remoting control logic for encrypted contents. This CL does the following: 1. Adds a RemotingCdmController to control whether to create CDM remotely for Media Remoting. 2. Renames the RemotingController to RemotingRendererController. 3. Separates the implementation of RemotingSource from RemotingRenderController to RemotingSourceImpl. After OnSetCdm() is called, one RemotingSourceImpl is shared by both RemotingRenderController and RemotingCdmController. BUG=643964 ========== to ========== Media Remoting: Add remoting control logic for encrypted contents. This CL does the following: 1. Adds a RemotingCdmController to control whether to create CDM remotely for Media Remoting. 2. Renames the RemotingController to RemotingRendererController. 3. Separates the implementation of RemotingSource from RemotingRenderController to RemotingSourceImpl. After OnSetCdm() is called, one RemotingSourceImpl is shared by both RemotingRenderController and RemotingCdmController. BUG=643964 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:450001)
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Add remoting control logic for encrypted contents. This CL does the following: 1. Adds a RemotingCdmController to control whether to create CDM remotely for Media Remoting. 2. Renames the RemotingController to RemotingRendererController. 3. Separates the implementation of RemotingSource from RemotingRenderController to RemotingSourceImpl. After OnSetCdm() is called, one RemotingSourceImpl is shared by both RemotingRenderController and RemotingCdmController. BUG=643964 ========== to ========== Media Remoting: Add remoting control logic for encrypted contents. This CL does the following: 1. Adds a RemotingCdmController to control whether to create CDM remotely for Media Remoting. 2. Renames the RemotingController to RemotingRendererController. 3. Separates the implementation of RemotingSource from RemotingRenderController to RemotingSourceImpl. After OnSetCdm() is called, one RemotingSourceImpl is shared by both RemotingRenderController and RemotingCdmController. BUG=643964 Committed: https://crrev.com/df8ea429178910ac6293e8e014fe307438fa099a Cr-Commit-Position: refs/heads/master@{#430730} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/df8ea429178910ac6293e8e014fe307438fa099a Cr-Commit-Position: refs/heads/master@{#430730} |
