Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1567123002:
Support CAST+WMPI on android (Closed)
|
Created:
4 years, 11 months ago by hubbe Modified:
4 years, 11 months ago Reviewers:
sandersd (OOO until July 31), watk, liberato (no reviews please), DaleCurtis, nasko, Tima Vaisburd CC:
avayvod+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri (slow - plz ping), nasko+codewatch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport CAST+WMPI on android
This is a intermediate implementation of cast support for WebMediaPlayerImpl.
A better long-term solution would be to implement the same functionality on
top of the presentation API. As a bonus, that would work on both mobile and
desktop. However, that solution may take some time to implement, and in the
meantime, this CL will unblock the spitzer project.
BUG=575276
Committed: https://crrev.com/d5f3688818eaca5f93587f4e2753d4bef4c0ab5c
Cr-Commit-Position: refs/heads/master@{#369869}
Patch Set 1 #
Total comments: 18
Patch Set 2 : comments addressed #
Total comments: 12
Patch Set 3 : broke out (most) android code into a separate file #Patch Set 4 : suspend/resume cleanup #
Total comments: 16
Patch Set 5 : comments addressed + build fix #
Total comments: 12
Patch Set 6 : compile fix #
Total comments: 10
Patch Set 7 : comments addressed #
Total comments: 4
Patch Set 8 : some comments addressed... #Patch Set 9 : comments addressed... #Patch Set 10 : comments addressed #
Total comments: 8
Patch Set 11 : merged #Patch Set 12 : comments addressed #
Total comments: 2
Patch Set 13 : dcheck removed #
Total comments: 27
Patch Set 14 : comments addressed #Patch Set 15 : build fix #Patch Set 16 : cast tests are now passing locally #
Total comments: 12
Patch Set 17 : comments addressed + merged (sorry) #
Total comments: 2
Patch Set 18 : build fixes + comments addressed #Messages
Total messages: 51 (9 generated)
hubbe@chromium.org changed reviewers: + dalecurtis@google.com
Pre-review. It compiles and works, but needs more testing.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org, liberato@chromium.org, sandersd@chromium.org, watk@chromium.org
+watk, sandersd, liberato since this touches pieces that they're all working on. I think this also means we don't need my MediaSession changes for M49 since you get that behavior for free here, right? https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:42: #define WIMPI_CAST Can we avoid setting this here. #define in header files are frowned upon. https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:207: #if defined(WIMPI_CAST) Can you stick this all in another class instead that WMPI can inherit conditionally based on defined(OS_ANDROID)? This would simply #define. You can add IsRemote() and other helper methods that WMPI can call as necessary for instances where a clean separation isn't possible.
https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:42: #define WIMPI_CAST On 2016/01/07 19:59:32, DaleCurtis wrote: > Can we avoid setting this here. #define in header files are frowned upon. Hmm, so where should it go then? I see two possible options: 1) add the defines in the build files (But this seems quite dangerous, what happens if different modules get compiled with different defines??) 2) just use OS_ANDROID https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:207: #if defined(WIMPI_CAST) On 2016/01/07 19:59:32, DaleCurtis wrote: > Can you stick this all in another class instead that WMPI can inherit > conditionally based on defined(OS_ANDROID)? This would simply #define. > > You can add IsRemote() and other helper methods that WMPI can call as necessary > for instances where a clean separation isn't possible. It's not a simple thing to do. Some of these methods manipulate WIMI variables, also a couple of the methods from RendererMediaPlayerInterface are not actually inside this #if, since they were already available in WebMediaPlayerImpl
https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1379: void WebMediaPlayerImpl::DrawRemotePlaybackText( Can you just abstract the existing code in WebMediaPlayerAndroid into MediaCodecUtil or something similar? https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:42: #define WIMPI_CAST On 2016/01/07 20:31:03, hubbe wrote: > On 2016/01/07 19:59:32, DaleCurtis wrote: > > Can we avoid setting this here. #define in header files are frowned upon. > > Hmm, so where should it go then? > > I see two possible options: > > 1) add the defines in the build files > (But this seems quite dangerous, what happens if different modules get > compiled with different defines??) > > 2) just use OS_ANDROID I'd just use OS_ANDROID with a // WMPI_CAST comment for deletability later. https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:207: #if defined(WIMPI_CAST) On 2016/01/07 20:31:03, hubbe wrote: > On 2016/01/07 19:59:32, DaleCurtis wrote: > > Can you stick this all in another class instead that WMPI can inherit > > conditionally based on defined(OS_ANDROID)? This would simply #define. > > > > You can add IsRemote() and other helper methods that WMPI can call as > necessary > > for instances where a clean separation isn't possible. > > It's not a simple thing to do. > Some of these methods manipulate WIMI variables, also a couple of the methods > from RendererMediaPlayerInterface are not actually inside this #if, since they > were already available in WebMediaPlayerImpl Please give it some more thought. You're essentially gluing 900+ lines into WMPI many of which are not using internal variables or functions; even for a temporary change is ugly.
https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:207: #if defined(WIMPI_CAST) On 2016/01/07 21:28:40, DaleCurtis wrote: > On 2016/01/07 20:31:03, hubbe wrote: > > On 2016/01/07 19:59:32, DaleCurtis wrote: > > > Can you stick this all in another class instead that WMPI can inherit > > > conditionally based on defined(OS_ANDROID)? This would simply #define. > > > > > > You can add IsRemote() and other helper methods that WMPI can call as > > necessary > > > for instances where a clean separation isn't possible. > > > > It's not a simple thing to do. > > Some of these methods manipulate WIMI variables, also a couple of the methods > > from RendererMediaPlayerInterface are not actually inside this #if, since > they > > were already available in WebMediaPlayerImpl > > Please give it some more thought. You're essentially gluing 900+ lines into WMPI > many of which are not using internal variables or functions; even for a > temporary change is ugly. s/900/400/
it's really cool that this is working! thanks -fl https://chromiumcodereview.appspot.com/1567123002/diff/1/content/renderer/ren... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/1567123002/diff/1/content/renderer/ren... content/renderer/render_frame_impl.cc:2393: #if defined(WIMPI_CAST) nit: s/WI/W/ https://chromiumcodereview.appspot.com/1567123002/diff/1/media/blink/renderer... File media/blink/renderer_media_player_interface.h (right): https://chromiumcodereview.appspot.com/1567123002/diff/1/media/blink/renderer... media/blink/renderer_media_player_interface.h:121: // Registers and unregisters a WebMediaPlayerAndroid object. s/WebMediaPlayerAndroid/Renderer.../ https://chromiumcodereview.appspot.com/1567123002/diff/1/media/blink/webmedia... File media/blink/webmediaplayer_impl.cc (right): https://chromiumcodereview.appspot.com/1567123002/diff/1/media/blink/webmedia... media/blink/webmediaplayer_impl.cc:1688: if (!is_remote_) the comment makes me expect that it should check for |is_remote_|==true rather than ==false. https://chromiumcodereview.appspot.com/1567123002/diff/1/media/blink/webmedia... File media/blink/webmediaplayer_impl.h (right): https://chromiumcodereview.appspot.com/1567123002/diff/1/media/blink/webmedia... media/blink/webmediaplayer_impl.h:207: #if defined(WIMPI_CAST) On 2016/01/07 21:29:09, DaleCurtis wrote: > On 2016/01/07 21:28:40, DaleCurtis wrote: > > On 2016/01/07 20:31:03, hubbe wrote: > > > On 2016/01/07 19:59:32, DaleCurtis wrote: > > > > Can you stick this all in another class instead that WMPI can inherit > > > > conditionally based on defined(OS_ANDROID)? This would simply #define. > > > > > > > > You can add IsRemote() and other helper methods that WMPI can call as > > > necessary > > > > for instances where a clean separation isn't possible. > > > > > > It's not a simple thing to do. > > > Some of these methods manipulate WIMI variables, also a couple of the > methods > > > from RendererMediaPlayerInterface are not actually inside this #if, since > > they > > > were already available in WebMediaPlayerImpl > > > > Please give it some more thought. You're essentially gluing 900+ lines into > WMPI > > many of which are not using internal variables or functions; even for a > > temporary change is ugly. > > s/900/400/ many of the operations these things perform are on |client_|, or on member variables that are specific to remote playback. i think that one might be able to create a class that inherits from the RendererMediaPlayerInterface, is given |client_|, and just notifies WMPI when exiting or entering remote playback. calls to WebMediaPlayer (e.g., play) can be delegated.
Description was changed from ========== Support CAST+WIMPI on android This is a intermediate implementation of cast support for WebMediaPlayerImpl. A better long-term solution would be to implement the same functionality on top of the presentation API. As a bonus, that would work on both mobile and desktop. However, that solution may take some time to implement, and in the meantime, this CL will unblock the spitzer project. BUG=575276 ========== to ========== Support CAST+WMPI on android This is a intermediate implementation of cast support for WebMediaPlayerImpl. A better long-term solution would be to implement the same functionality on top of the presentation API. As a bonus, that would work on both mobile and desktop. However, that solution may take some time to implement, and in the meantime, this CL will unblock the spitzer project. BUG=575276 ==========
https://codereview.chromium.org/1567123002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1567123002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2393: #if defined(WIMPI_CAST) On 2016/01/08 16:37:58, liberato wrote: > nit: s/WI/W/ Done. https://codereview.chromium.org/1567123002/diff/1/media/blink/renderer_media_... File media/blink/renderer_media_player_interface.h (right): https://codereview.chromium.org/1567123002/diff/1/media/blink/renderer_media_... media/blink/renderer_media_player_interface.h:121: // Registers and unregisters a WebMediaPlayerAndroid object. On 2016/01/08 16:37:58, liberato wrote: > s/WebMediaPlayerAndroid/Renderer.../ Done. https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1379: void WebMediaPlayerImpl::DrawRemotePlaybackText( On 2016/01/07 21:28:39, DaleCurtis wrote: > Can you just abstract the existing code in WebMediaPlayerAndroid into > MediaCodecUtil or something similar? I think it can, but it will not make it easier to delete this code later. Do you still want me to break it out? https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1688: if (!is_remote_) On 2016/01/08 16:37:58, liberato wrote: > the comment makes me expect that it should check for |is_remote_|==true rather > than ==false. Good catch, fixed. (Testing...) https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:42: #define WIMPI_CAST On 2016/01/07 21:28:39, DaleCurtis wrote: > On 2016/01/07 20:31:03, hubbe wrote: > > On 2016/01/07 19:59:32, DaleCurtis wrote: > > > Can we avoid setting this here. #define in header files are frowned upon. > > > > Hmm, so where should it go then? > > > > I see two possible options: > > > > 1) add the defines in the build files > > (But this seems quite dangerous, what happens if different modules get > > compiled with different defines??) > > > > 2) just use OS_ANDROID > > I'd just use OS_ANDROID with a // WMPI_CAST comment for deletability later. Done. https://codereview.chromium.org/1567123002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:207: #if defined(WIMPI_CAST) On 2016/01/08 16:37:58, liberato wrote: > On 2016/01/07 21:29:09, DaleCurtis wrote: > > On 2016/01/07 21:28:40, DaleCurtis wrote: > > > On 2016/01/07 20:31:03, hubbe wrote: > > > > On 2016/01/07 19:59:32, DaleCurtis wrote: > > > > > Can you stick this all in another class instead that WMPI can inherit > > > > > conditionally based on defined(OS_ANDROID)? This would simply #define. > > > > > > > > > > You can add IsRemote() and other helper methods that WMPI can call as > > > > necessary > > > > > for instances where a clean separation isn't possible. > > > > > > > > It's not a simple thing to do. > > > > Some of these methods manipulate WIMI variables, also a couple of the > > methods > > > > from RendererMediaPlayerInterface are not actually inside this #if, since > > > they > > > > were already available in WebMediaPlayerImpl > > > > > > Please give it some more thought. You're essentially gluing 900+ lines into > > WMPI > > > many of which are not using internal variables or functions; even for a > > > temporary change is ugly. > > > > s/900/400/ > > many of the operations these things perform are on |client_|, or on member > variables that are specific to remote playback. > > i think that one might be able to create a class that inherits from the > RendererMediaPlayerInterface, is given |client_|, and just notifies WMPI when > exiting or entering remote playback. calls to WebMediaPlayer (e.g., play) can > be delegated. I think it might turn out messy, but I'm not sure. I'll make a copy of this CL and try it.
https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1142: if (suspended_ || suspending_) What's the logic for this change? As currently written, there are some bugs in this implementation. Example: * Playing * OnHidden -> Suspending * OnShown -> Suspending + Pending Resume * OnHidden -> (no action due to new code) * Suspended -> Resuming (due to pending resume) * Resumed https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1198: if (!suspended_ || resuming_) Same question here. Why is this the correct change?
https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1142: if (suspended_ || suspending_) On 2016/01/11 23:04:37, sandersd wrote: > What's the logic for this change? As currently written, there are some bugs in > this implementation. Example: The logic was DCHECK(!suspending_) triggered. > > * Playing > * OnHidden -> Suspending > * OnShown -> Suspending + Pending Resume > * OnHidden -> (no action due to new code) > * Suspended -> Resuming (due to pending resume) > * Resumed I'm not sure I grok this example. https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1198: if (!suspended_ || resuming_) On 2016/01/11 23:04:37, sandersd wrote: > Same question here. Why is this the correct change? Frankly, I'm not entirely sure this is the correct change. It seemed reasonable at the time, but you're the export.
https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1142: if (suspended_ || suspending_) On 2016/01/11 23:39:11, hubbe wrote: > On 2016/01/11 23:04:37, sandersd wrote: > > What's the logic for this change? As currently written, there are some bugs in > > this implementation. Example: > > > The logic was DCHECK(!suspending_) triggered. Where was this DCHECK? No such DCHECK exists in the code currently. If it was added here, perhaps it's not indicative of a problem? > > * Playing > > * OnHidden -> Suspending > > * OnShown -> Suspending + Pending Resume > > * OnHidden -> (no action due to new code) > > * Suspended -> Resuming (due to pending resume) > > * Resumed > > I'm not sure I grok this example. It's a list of events and state transitions where we imagine that the frame is being hidden twice within the span it takes to suspend once. In this example the new code skips later code that would have remove the pending resume, and so when the suspend finishes it resumes immediately.
https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1142: if (suspended_ || suspending_) On 2016/01/12 00:05:33, sandersd wrote: > On 2016/01/11 23:39:11, hubbe wrote: > > On 2016/01/11 23:04:37, sandersd wrote: > > > What's the logic for this change? As currently written, there are some bugs > in > > > this implementation. Example: > > > > > > The logic was DCHECK(!suspending_) triggered. > > Where was this DCHECK? No such DCHECK exists in the code currently. If it was > added here, perhaps it's not indicative of a problem? > > > > * Playing > > > * OnHidden -> Suspending > > > * OnShown -> Suspending + Pending Resume > > > * OnHidden -> (no action due to new code) > > > * Suspended -> Resuming (due to pending resume) > > > * Resumed > > > > I'm not sure I grok this example. > > It's a list of events and state transitions where we imagine that the frame is > being hidden twice within the span it takes to suspend once. In this example the > new code skips later code that would have remove the pending resume, and so when > the suspend finishes it resumes immediately. My confusion, there is a DCHECK(!resuming_) but no dcheck for !suspsending, I think I assumed symmetry when I added this. Will update CL shortly...
Broke out most of the WMPI changes into a separate file as requested and cleaned up some of the suspend/resume logic. Time for serious reviewing!
dalecurtis@chromium.org changed reviewers: + timav@chromium.org - dalecurtis@google.com
Glanced over, will review more in an hour or so. +timav for android changes. https://codereview.chromium.org/1567123002/diff/60001/content/renderer/media/... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/1567123002/diff/60001/content/renderer/media/... content/renderer/media/android/renderer_media_player_manager.cc:76: static_cast<MediaPlayerHostMsg_Initialize_Type>(type); Seems unnecessary? Did something complain?
https://codereview.chromium.org/1567123002/diff/60001/content/renderer/media/... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/1567123002/diff/60001/content/renderer/media/... content/renderer/media/android/renderer_media_player_manager.cc:76: static_cast<MediaPlayerHostMsg_Initialize_Type>(type); On 2016/01/12 20:24:09, DaleCurtis wrote: > Seems unnecessary? Did something complain? ../../content/renderer/media/android/renderer_media_player_manager.cc: In member function 'virtual void content::RendererMediaPlay\ erManager::Initialize(media::MediaPlayerHostMsg_Initialize_Type, int, const GURL&, const GURL&, int, const GURL&, bool)': ../../content/renderer/media/android/renderer_media_player_manager.cc:75:28: error: cannot convert 'media::MediaPlayerHostMsg_Init\ ialize_Type' to 'MediaPlayerHostMsg_Initialize_Type' in assignment media_player_params.type = type;
overall i like the refactoring. just a few minor questions. thanks -fl https://chromiumcodereview.appspot.com/1567123002/diff/20001/media/blink/webm... File media/blink/webmediaplayer_impl.cc (right): https://chromiumcodereview.appspot.com/1567123002/diff/20001/media/blink/webm... media/blink/webmediaplayer_impl.cc:116: // File-static function is to allow it to run even after WMPA is deleted. /WMPA/WMPI/ https://chromiumcodereview.appspot.com/1567123002/diff/20001/media/blink/webm... media/blink/webmediaplayer_impl.cc:130: // Flush to ensure that the stream texture gets deleted in a timely fashion. /stream// https://chromiumcodereview.appspot.com/1567123002/diff/20001/media/blink/webm... media/blink/webmediaplayer_impl.cc:1497: if (!suspended_ && !suspending_) why is this here? https://chromiumcodereview.appspot.com/1567123002/diff/60001/media/blink/webm... File media/blink/webmediaplayer_cast_android.cc (right): https://chromiumcodereview.appspot.com/1567123002/diff/60001/media/blink/webm... media/blink/webmediaplayer_cast_android.cc:29: // File-static function is to allow it to run even after WMPA is deleted. /WMPA/WMPI/ https://chromiumcodereview.appspot.com/1567123002/diff/60001/media/blink/webm... media/blink/webmediaplayer_cast_android.cc:43: // Flush to ensure that the stream texture gets deleted in a timely fashion. stale comment -- /stream // https://chromiumcodereview.appspot.com/1567123002/diff/60001/media/blink/webm... media/blink/webmediaplayer_cast_android.cc:54: WebMediaPlayerCast::~WebMediaPlayerCast() { \n https://chromiumcodereview.appspot.com/1567123002/diff/60001/media/blink/webm... File media/blink/webmediaplayer_impl.cc (right): https://chromiumcodereview.appspot.com/1567123002/diff/60001/media/blink/webm... media/blink/webmediaplayer_impl.cc:314: #if defined(OS_ANDROID) // WMPI_CAST are these ifdefs needed? cast_impl_.isRemote() will return false unless on android. here and elsewhere. https://chromiumcodereview.appspot.com/1567123002/diff/60001/media/blink/webm... media/blink/webmediaplayer_impl.cc:316: cast_impl_.play(); if it's remote and already playing, why do we fall through to the non-remote case? this is asymmetric with the paused case, where we don't. https://chromiumcodereview.appspot.com/1567123002/diff/60001/media/blink/webm... media/blink/webmediaplayer_impl.cc:1242: void WebMediaPlayerImpl::DisplayCastFrame( since this only works as a suspend callback, maybe DisplayCastFrameAfterSuspend or something.
https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:116: // File-static function is to allow it to run even after WMPA is deleted. On 2016/01/12 22:14:57, liberato wrote: > /WMPA/WMPI/ Done. https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:130: // Flush to ensure that the stream texture gets deleted in a timely fashion. On 2016/01/12 22:14:58, liberato wrote: > /stream// Done. https://codereview.chromium.org/1567123002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1497: if (!suspended_ && !suspending_) On 2016/01/12 22:14:57, liberato wrote: > why is this here? A very existential question. Check the latest version and see if makes more sense? https://codereview.chromium.org/1567123002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_cast_android.cc (right): https://codereview.chromium.org/1567123002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_cast_android.cc:29: // File-static function is to allow it to run even after WMPA is deleted. On 2016/01/12 22:14:58, liberato wrote: > /WMPA/WMPI/ Done. https://codereview.chromium.org/1567123002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_cast_android.cc:43: // Flush to ensure that the stream texture gets deleted in a timely fashion. On 2016/01/12 22:14:58, liberato wrote: > stale comment -- /stream // Done. https://codereview.chromium.org/1567123002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_cast_android.cc:54: WebMediaPlayerCast::~WebMediaPlayerCast() { On 2016/01/12 22:14:58, liberato wrote: > \n Done. https://codereview.chromium.org/1567123002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:314: #if defined(OS_ANDROID) // WMPI_CAST On 2016/01/12 22:14:58, liberato wrote: > are these ifdefs needed? cast_impl_.isRemote() will return false unless on > android. here and elsewhere. cast_impl_ doesn't exist on non-android. I could make it exist I suppose, but that seems weird. https://codereview.chromium.org/1567123002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:316: cast_impl_.play(); On 2016/01/12 22:14:58, liberato wrote: > if it's remote and already playing, why do we fall through to the non-remote > case? this is asymmetric with the paused case, where we don't. Fixed. https://codereview.chromium.org/1567123002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1242: void WebMediaPlayerImpl::DisplayCastFrame( On 2016/01/12 22:14:58, liberato wrote: > since this only works as a suspend callback, maybe DisplayCastFrameAfterSuspend > or something. Done.
Just some nits, overall lg to me. Does this pass the instrumentation tests for media session and cast as discussed via chat when spitzer is defaulted on? https://codereview.chromium.org/1567123002/diff/60001/content/renderer/media/... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/1567123002/diff/60001/content/renderer/media/... content/renderer/media/android/renderer_media_player_manager.cc:76: static_cast<MediaPlayerHostMsg_Initialize_Type>(type); On 2016/01/12 21:23:18, hubbe wrote: > On 2016/01/12 20:24:09, DaleCurtis wrote: > > Seems unnecessary? Did something complain? > > ../../content/renderer/media/android/renderer_media_player_manager.cc: In member > function 'virtual void content::RendererMediaPlay\ > erManager::Initialize(media::MediaPlayerHostMsg_Initialize_Type, int, const > GURL&, const GURL&, int, const GURL&, bool)': > ../../content/renderer/media/android/renderer_media_player_manager.cc:75:28: > error: cannot convert 'media::MediaPlayerHostMsg_Init\ > ialize_Type' to 'MediaPlayerHostMsg_Initialize_Type' in assignment > media_player_params.type = type; Oh I see, you have a different type of the same name. Lets not do that, please give it a different name to confusing aliasing. https://codereview.chromium.org/1567123002/diff/80001/media/blink/BUILD.gn File media/blink/BUILD.gn (right): https://codereview.chromium.org/1567123002/diff/80001/media/blink/BUILD.gn#ne... media/blink/BUILD.gn:82: "webmediasource_cast_android.cc", wmp? https://codereview.chromium.org/1567123002/diff/80001/media/blink/renderer_me... File media/blink/renderer_media_player_interface.h (right): https://codereview.chromium.org/1567123002/diff/80001/media/blink/renderer_me... media/blink/renderer_media_player_interface.h:73: enum MediaPlayerHostMsg_Initialize_Type { Why not include this file in content/common/media/media_player_messages_enums_android.h so they use the same enum? https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_cast_android.cc (right): https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_cast_android.cc:211: bool WebMediaPlayerCast::paused() const { Clang-format this file? there should be lines between the function definitions. https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_cast_android.cc:224: void WebMediaPlayerCast::DrawRemotePlaybackText( Can this just use the WMPA code instead of copy pasting its own? https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_cast_android.h (right): https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_cast_android.h:25: class WebMediaPlayerCast : public RendererMediaPlayerInterface { Top level comment.
https://codereview.chromium.org/1567123002/diff/60001/content/renderer/media/... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/1567123002/diff/60001/content/renderer/media/... content/renderer/media/android/renderer_media_player_manager.cc:76: static_cast<MediaPlayerHostMsg_Initialize_Type>(type); On 2016/01/12 23:21:53, DaleCurtis wrote: > On 2016/01/12 21:23:18, hubbe wrote: > > On 2016/01/12 20:24:09, DaleCurtis wrote: > > > Seems unnecessary? Did something complain? > > > > ../../content/renderer/media/android/renderer_media_player_manager.cc: In > member > > function 'virtual void content::RendererMediaPlay\ > > erManager::Initialize(media::MediaPlayerHostMsg_Initialize_Type, int, const > > GURL&, const GURL&, int, const GURL&, bool)': > > ../../content/renderer/media/android/renderer_media_player_manager.cc:75:28: > > error: cannot convert 'media::MediaPlayerHostMsg_Init\ > > ialize_Type' to 'MediaPlayerHostMsg_Initialize_Type' in assignment > > media_player_params.type = type; > > Oh I see, you have a different type of the same name. Lets not do that, please > give it a different name to confusing aliasing. Done. https://codereview.chromium.org/1567123002/diff/80001/media/blink/BUILD.gn File media/blink/BUILD.gn (right): https://codereview.chromium.org/1567123002/diff/80001/media/blink/BUILD.gn#ne... media/blink/BUILD.gn:82: "webmediasource_cast_android.cc", On 2016/01/12 23:21:53, DaleCurtis wrote: > wmp? Oops, fixed. https://codereview.chromium.org/1567123002/diff/80001/media/blink/renderer_me... File media/blink/renderer_media_player_interface.h (right): https://codereview.chromium.org/1567123002/diff/80001/media/blink/renderer_me... media/blink/renderer_media_player_interface.h:73: enum MediaPlayerHostMsg_Initialize_Type { On 2016/01/12 23:21:53, DaleCurtis wrote: > Why not include this file in > content/common/media/media_player_messages_enums_android.h so they use the same > enum? Thought that would end up causing dependency problems, but it seems to work fine. Done. https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_cast_android.cc (right): https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_cast_android.cc:211: bool WebMediaPlayerCast::paused() const { On 2016/01/12 23:21:53, DaleCurtis wrote: > Clang-format this file? there should be lines between the function definitions. Already read "git cl format media" which did not add it. Added missing newlines manually. https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_cast_android.cc:224: void WebMediaPlayerCast::DrawRemotePlaybackText( On 2016/01/12 23:21:53, DaleCurtis wrote: > Can this just use the WMPA code instead of copy pasting its own? I can break out the common parts, but it's not possible to invoke the code as-is. https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_cast_android.h (right): https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_cast_android.h:25: class WebMediaPlayerCast : public RendererMediaPlayerInterface { On 2016/01/12 23:21:53, DaleCurtis wrote: > Top level comment. Done.
https://codereview.chromium.org/1567123002/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1095: if (suspended_) This is not the right place because while suspended there may be a pending resume. Perhaps there should be a separate branch for remote playback, since I imagine we don't really want to handle OnHidden/OnShown at all during remote playback. This check is somewhat similar to the !suspended_ check in OnShown(), in that I can imagine it happening if we miss an OnShown event, but in fact the underlying bug and reason for that comment have been fixed. https://codereview.chromium.org/1567123002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1151: if (!suspended_ || resuming_) Without cast changes, it's not possible to be resuming_ here, but the good news is that means this won't break anything. I'd suggest inverting the whole condition and calling Resume() inside it, and updating the comment to say that this can happen when resuming from remote playback. https://codereview.chromium.org/1567123002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1213: pipeline_.SetPlaybackRate(0.0); Here we want to make sure that Blink finds out that we've forced a paused state. It's possible that playbackStateChanged() does this (if it actually gets called), but it may be necessary to call DidPause() on the delegate as well. I'll leave it to you to investigate the effects of those things. https://codereview.chromium.org/1567123002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1218: if (suspended_ && !resuming_) Here again what we really want is to factor out the logic in OnShown() and call that. https://codereview.chromium.org/1567123002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1234: if (!suspended_ && !suspending_) { |suspending_| is a subset of |suspended_|. In general this condition is not sufficient, since there could be a pending suspend or an active seek. A cleaner solution is to factor the main logic out of OnHidden(), and handle the PaintFrame call in OnPipelineSuspended. Then you won't have to worry about all the possible transitions here or juggle suspend callbacks.
https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_cast_android.cc (right): https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_cast_android.cc:224: void WebMediaPlayerCast::DrawRemotePlaybackText( On 2016/01/13 00:11:56, hubbe wrote: > On 2016/01/12 23:21:53, DaleCurtis wrote: > > Can this just use the WMPA code instead of copy pasting its own? > > I can break out the common parts, but it's not possible to invoke the code > as-is. Seems like a util method which returns a videoframe and is passed in the context cb would be enough? https://codereview.chromium.org/1567123002/diff/120001/content/common/media/m... File content/common/media/media_player_messages_enums_android.h (right): https://codereview.chromium.org/1567123002/diff/120001/content/common/media/m... content/common/media/media_player_messages_enums_android.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. Delete this file and just include the other one directly in relevant spots? https://codereview.chromium.org/1567123002/diff/120001/content/renderer/media... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/1567123002/diff/120001/content/renderer/media... content/renderer/media/android/renderer_media_player_manager.cc:76: static_cast<MediaPlayerHostMsg_Initialize_Type>(type); All these casts can be removed now.
Some comments addressed, still need to work on cleaning up suspend/resume. https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_cast_android.cc (right): https://codereview.chromium.org/1567123002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_cast_android.cc:224: void WebMediaPlayerCast::DrawRemotePlaybackText( On 2016/01/13 01:22:33, DaleCurtis wrote: > On 2016/01/13 00:11:56, hubbe wrote: > > On 2016/01/12 23:21:53, DaleCurtis wrote: > > > Can this just use the WMPA code instead of copy pasting its own? > > > > I can break out the common parts, but it's not possible to invoke the code > > as-is. > > Seems like a util method which returns a videoframe and is passed in the context > cb would be enough? Needed a little creative interfacing, but it's now done. I looked at putting it MediaCodecUtils, but I would have needed to add some more dependencies and includes that just didn't seem right. https://codereview.chromium.org/1567123002/diff/120001/content/common/media/m... File content/common/media/media_player_messages_enums_android.h (right): https://codereview.chromium.org/1567123002/diff/120001/content/common/media/m... content/common/media/media_player_messages_enums_android.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2016/01/13 01:22:33, DaleCurtis wrote: > Delete this file and just include the other one directly in relevant spots? Done. https://codereview.chromium.org/1567123002/diff/120001/content/renderer/media... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/1567123002/diff/120001/content/renderer/media... content/renderer/media/android/renderer_media_player_manager.cc:76: static_cast<MediaPlayerHostMsg_Initialize_Type>(type); On 2016/01/13 01:22:33, DaleCurtis wrote: > All these casts can be removed now. Done.
https://codereview.chromium.org/1567123002/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1095: if (suspended_) On 2016/01/13 00:23:08, sandersd wrote: > This is not the right place because while suspended there may be a pending > resume. > > Perhaps there should be a separate branch for remote playback, since I imagine > we don't really want to handle OnHidden/OnShown at all during remote playback. > > This check is somewhat similar to the !suspended_ check in OnShown(), in that I > can imagine it happening if we miss an OnShown event, but in fact the underlying > bug and reason for that comment have been fixed. Better? https://codereview.chromium.org/1567123002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1151: if (!suspended_ || resuming_) On 2016/01/13 00:23:08, sandersd wrote: > Without cast changes, it's not possible to be resuming_ here, but the good news > is that means this won't break anything. > > I'd suggest inverting the whole condition and calling Resume() inside it, and > updating the comment to say that this can happen when resuming from remote > playback. Done. https://codereview.chromium.org/1567123002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1213: pipeline_.SetPlaybackRate(0.0); On 2016/01/13 00:23:08, sandersd wrote: > Here we want to make sure that Blink finds out that we've forced a paused state. > It's possible that playbackStateChanged() does this (if it actually gets > called), but it may be necessary to call DidPause() on the delegate as well. > I'll leave it to you to investigate the effects of those things. We don't want to call DidPause(), and WebMediaPlayer should already be in a paused state, so all we need to to is to let blink know. https://codereview.chromium.org/1567123002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1218: if (suspended_ && !resuming_) On 2016/01/13 00:23:08, sandersd wrote: > Here again what we really want is to factor out the logic in OnShown() and call > that. Done. https://codereview.chromium.org/1567123002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1234: if (!suspended_ && !suspending_) { On 2016/01/13 00:23:08, sandersd wrote: > |suspending_| is a subset of |suspended_|. > > In general this condition is not sufficient, since there could be a pending > suspend or an active seek. A cleaner solution is to factor the main logic out of > OnHidden(), and handle the PaintFrame call in OnPipelineSuspended. Then you > won't have to worry about all the possible transitions here or juggle suspend > callbacks. Done.
https://codereview.chromium.org/1567123002/diff/180001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1095: if (cast_impl_.isRemote()) Nit: This would be better as a separate condition, to keep the command line handling together. https://codereview.chromium.org/1567123002/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1144: if (cast_impl_.isRemote()) Nit: Same here about grouping command line handling. https://codereview.chromium.org/1567123002/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1233: pending_seek_time_ = paused_time_; One thing to consider is that there may have been a pending seek when the pipeline suspended (suspend overrides pending seek), and so you'll want to be sure that you start remote playback at that time. You should be able to assert that there is no pending seek in this method. https://codereview.chromium.org/1567123002/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1251: return; Early return here could be in a bad state if there is a current or pending resume.
https://codereview.chromium.org/1567123002/diff/180001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1095: if (cast_impl_.isRemote()) On 2016/01/13 22:11:10, sandersd wrote: > Nit: This would be better as a separate condition, to keep the command line > handling together. Like this? https://codereview.chromium.org/1567123002/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1144: if (cast_impl_.isRemote()) On 2016/01/13 22:11:10, sandersd wrote: > Nit: Same here about grouping command line handling. Done. https://codereview.chromium.org/1567123002/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1233: pending_seek_time_ = paused_time_; On 2016/01/13 22:11:10, sandersd wrote: > One thing to consider is that there may have been a pending seek when the > pipeline suspended (suspend overrides pending seek), and so you'll want to be > sure that you start remote playback at that time. You should be able to assert > that there is no pending seek in this method. Wouldn't currentTime() return the right time if a seek is pending? (Because that's where I start remote playback.) I added a DCHECK for !pending_seek https://codereview.chromium.org/1567123002/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1251: return; On 2016/01/13 22:11:10, sandersd wrote: > Early return here could be in a bad state if there is a current or pending > resume. Done.
lgtm % outcome of the current thread of comments :)
LGTM for suspend/resume logic (/w nit). https://codereview.chromium.org/1567123002/diff/220001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/220001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1246: DCHECK(!pending_seek_); Looks like you're correct that the currentTime() respects a pending seek. However, there is no step that actually clears the pending seek and so this DCHECK will trigger in that case. I suppose just remove this DCHECK for now, having a pending seek the whole time isn't really a problem.
https://chromiumcodereview.appspot.com/1567123002/diff/220001/media/blink/web... File media/blink/webmediaplayer_impl.cc (right): https://chromiumcodereview.appspot.com/1567123002/diff/220001/media/blink/web... media/blink/webmediaplayer_impl.cc:1246: DCHECK(!pending_seek_); On 2016/01/13 23:05:43, sandersd wrote: > Looks like you're correct that the currentTime() respects a pending seek. > However, there is no step that actually clears the pending seek and so this > DCHECK will trigger in that case. > > I suppose just remove this DCHECK for now, having a pending seek the whole time > isn't really a problem. Done.
As discussed there are still many failing media session and cast tests. I'll give approval once those are passing locally. https://codereview.chromium.org/1567123002/diff/240001/content/renderer/media... File content/renderer/media/android/renderer_media_player_manager.h (right): https://codereview.chromium.org/1567123002/diff/240001/content/renderer/media... content/renderer/media/android/renderer_media_player_manager.h:30: using media::RendererMediaPlayerInterface; Either keep as class or convert the rest to using for consistency. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_cast_android.cc (right): https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:24: #if defined(OS_ANDROID) Unnecessary? https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:176: void WebMediaPlayerCast::set_media_player_manager( SetMediaPlayerManager() no hacker_style() for non-inline methods. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:192: // RendererMediaPlayerInterface implementation Remove. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:209: // DCHECK(main_thread_checker_.CalledOnValidThread()); ? https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:245: webmediaplayer_->pause(); Do we want the UI to reflect this pause? https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:268: void WebMediaPlayerCast::pause() { insert line above. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:339: // DCHECK(main_thread_checker_.CalledOnValidThread()); ? https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1463: #if defined(OS_ANDROID) // WMPI_CAST You should still send this information the browser side will make that determination based on whether isRemote() is true. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1480: if (cast_impl_.isRemote()) Ditto.
https://codereview.chromium.org/1567123002/diff/240001/media/blink/renderer_m... File media/blink/renderer_media_player_interface.h (right): https://codereview.chromium.org/1567123002/diff/240001/media/blink/renderer_m... media/blink/renderer_media_player_interface.h:24: MEDIA_PLAYER_TYPE_LAST = MEDIA_PLAYER_TYPE_MEDIA_SOURCE Shall the last type be MEDIA_PLAYER_TYPE_REMOTE_ONLY ?
https://codereview.chromium.org/1567123002/diff/240001/content/renderer/media... File content/renderer/media/android/renderer_media_player_manager.h (right): https://codereview.chromium.org/1567123002/diff/240001/content/renderer/media... content/renderer/media/android/renderer_media_player_manager.h:30: using media::RendererMediaPlayerInterface; On 2016/01/14 02:01:41, DaleCurtis wrote: > Either keep as class or convert the rest to using for consistency. Done. https://codereview.chromium.org/1567123002/diff/240001/media/blink/renderer_m... File media/blink/renderer_media_player_interface.h (right): https://codereview.chromium.org/1567123002/diff/240001/media/blink/renderer_m... media/blink/renderer_media_player_interface.h:24: MEDIA_PLAYER_TYPE_LAST = MEDIA_PLAYER_TYPE_MEDIA_SOURCE On 2016/01/14 02:32:44, Tima Vaisburd wrote: > Shall the last type be MEDIA_PLAYER_TYPE_REMOTE_ONLY ? Done. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_cast_android.cc (right): https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:24: #if defined(OS_ANDROID) On 2016/01/14 02:01:41, DaleCurtis wrote: > Unnecessary? I thought so too, but apparently other platforms will try to compile this without these ifdefs. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:176: void WebMediaPlayerCast::set_media_player_manager( On 2016/01/14 02:01:41, DaleCurtis wrote: > SetMediaPlayerManager() no hacker_style() for non-inline methods. Done. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:192: // RendererMediaPlayerInterface implementation On 2016/01/14 02:01:41, DaleCurtis wrote: > Remove. Done. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:209: // DCHECK(main_thread_checker_.CalledOnValidThread()); On 2016/01/14 02:01:41, DaleCurtis wrote: > ? Removed https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:245: webmediaplayer_->pause(); On 2016/01/14 02:01:41, DaleCurtis wrote: > Do we want the UI to reflect this pause? I think so, it seems to make sense that the local player signals a "pause" while the remote is loading. Once the playback starts it changes to play again. However, I think it could make sense either way. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:268: void WebMediaPlayerCast::pause() { On 2016/01/14 02:01:41, DaleCurtis wrote: > insert line above. Done. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:339: // DCHECK(main_thread_checker_.CalledOnValidThread()); On 2016/01/14 02:01:41, DaleCurtis wrote: > ? Removed. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1463: #if defined(OS_ANDROID) // WMPI_CAST On 2016/01/14 02:01:41, DaleCurtis wrote: > You should still send this information the browser side will make that > determination based on whether isRemote() is true. Good point. Added delegate reporting from WebMediaPlayerCast. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1480: if (cast_impl_.isRemote()) On 2016/01/14 02:01:41, DaleCurtis wrote: > Ditto. Done.
https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_cast_android.cc (right): https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:24: #if defined(OS_ANDROID) On 2016/01/14 18:05:00, hubbe wrote: > On 2016/01/14 02:01:41, DaleCurtis wrote: > > Unnecessary? > > I thought so too, but apparently other platforms will try to compile this > without these ifdefs. Probably you need to add the wmpcast_android to a sources! section in .gyp and sources -= in GN then for non-android builds (or only add it in android builds, depending on what the existing pattern is in the build files). https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:245: webmediaplayer_->pause(); On 2016/01/14 18:05:00, hubbe wrote: > On 2016/01/14 02:01:41, DaleCurtis wrote: > > Do we want the UI to reflect this pause? > > I think so, it seems to make sense that the local player signals a "pause" while > the remote is loading. Once the playback starts it changes to play again. > However, I think it could make sense either way. You'll need to call playbackStateChanged() if so then -- otherwise the UI will still show as playing.
hubbe@chromium.org changed reviewers: + nasko@chromium.org
Cast tests are now passing locally, PTAL. +nasko for the following files: content/common/media/media_player_messages_android.h content/renderer/render_frame_impl.cc Fast turnaround appreciated.
https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_cast_android.cc (right): https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:24: #if defined(OS_ANDROID) On 2016/01/14 18:25:40, DaleCurtis wrote: > On 2016/01/14 18:05:00, hubbe wrote: > > On 2016/01/14 02:01:41, DaleCurtis wrote: > > > Unnecessary? > > > > I thought so too, but apparently other platforms will try to compile this > > without these ifdefs. > > Probably you need to add the wmpcast_android to a sources! section in .gyp and > sources -= in GN then for non-android builds (or only add it in android builds, > depending on what the existing pattern is in the build files). Can you resolve this? Add a if (is_android) section to the .gn and .gyp files.
https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_cast_android.cc (right): https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:24: #if defined(OS_ANDROID) Delete after build file fixes. https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:250: delegate_->DidPlay(webmediaplayer_); All the delegate calls in this file happen with is_remote_=true right? So they should be noops. https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_cast_android.h (right): https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.h:15: #if defined(OS_ANDROID) Delete. https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.h:144: }; DISALLOW_COPY_AND_ASSIGN
LGTM on: content/common/media/media_player_messages_android.h content/renderer/render_frame_impl.cc https://codereview.chromium.org/1567123002/diff/300001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1567123002/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:2415: #if defined(OS_ANDROID) // WMPI_CAST Please leave an empty line before and after the #ifdef'd code, since it makes it more readable.
All comments addressed (I hope) Running trybots now. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_cast_android.cc (right): https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:24: #if defined(OS_ANDROID) On 2016/01/15 19:55:44, DaleCurtis wrote: > On 2016/01/14 18:25:40, DaleCurtis wrote: > > On 2016/01/14 18:05:00, hubbe wrote: > > > On 2016/01/14 02:01:41, DaleCurtis wrote: > > > > Unnecessary? > > > > > > I thought so too, but apparently other platforms will try to compile this > > > without these ifdefs. > > > > Probably you need to add the wmpcast_android to a sources! section in .gyp and > > sources -= in GN then for non-android builds (or only add it in android > builds, > > depending on what the existing pattern is in the build files). > > Can you resolve this? Add a if (is_android) section to the .gn and .gyp files. Done. https://codereview.chromium.org/1567123002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:245: webmediaplayer_->pause(); On 2016/01/14 18:25:40, DaleCurtis wrote: > On 2016/01/14 18:05:00, hubbe wrote: > > On 2016/01/14 02:01:41, DaleCurtis wrote: > > > Do we want the UI to reflect this pause? > > > > I think so, it seems to make sense that the local player signals a "pause" > while > > the remote is loading. Once the playback starts it changes to play again. > > However, I think it could make sense either way. > > You'll need to call playbackStateChanged() if so then -- otherwise the UI will > still show as playing. Actually, it seems the tests prefer it to be in "playing" state... https://codereview.chromium.org/1567123002/diff/300001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1567123002/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:2415: #if defined(OS_ANDROID) // WMPI_CAST On 2016/01/15 20:08:37, nasko wrote: > Please leave an empty line before and after the #ifdef'd code, since it makes it > more readable. Done. https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_cast_android.cc (right): https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:24: #if defined(OS_ANDROID) On 2016/01/15 20:07:45, DaleCurtis wrote: > Delete after build file fixes. Done. https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.cc:250: delegate_->DidPlay(webmediaplayer_); On 2016/01/15 20:07:45, DaleCurtis wrote: > All the delegate calls in this file happen with is_remote_=true right? So they > should be noops. Yes, but you wanted the delegate to make that determination, right? https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_cast_android.h (right): https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.h:15: #if defined(OS_ANDROID) On 2016/01/15 20:07:45, DaleCurtis wrote: > Delete. Still need this one since it will be included in places regardless of what the build file says. (Better than having #if defined(OS_ANDROID) where this is included, right?) https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.h:144: }; On 2016/01/15 20:07:45, DaleCurtis wrote: > DISALLOW_COPY_AND_ASSIGN Done.
https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_cast_android.h (right): https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.h:15: #if defined(OS_ANDROID) On 2016/01/15 20:35:47, hubbe wrote: > On 2016/01/15 20:07:45, DaleCurtis wrote: > > Delete. > > Still need this one since it will be included in places regardless of what the > build file says. > (Better than having #if defined(OS_ANDROID) where this is included, right?) > No, the latter is preferred and is already done. I can't think of any other places in media (or elsewhere) where we do it the way you have here. -- That said, I don't see any place in this CL where you're including this file in a non-android place. What fails if you delete this?
this lgtm % removing the OS_ANDROID #if checks from the header files. Local android test running seems to be busted as the moment :( https://code.google.com/p/chromium/issues/detail?id=577695 https://codereview.chromium.org/1567123002/diff/320001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1567123002/diff/320001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:12: Add build/build_config.h for OS Macros.
Almost there..... (a few buildbots away...) https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_cast_android.h (right): https://codereview.chromium.org/1567123002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_cast_android.h:15: #if defined(OS_ANDROID) On 2016/01/15 21:03:38, DaleCurtis wrote: > On 2016/01/15 20:35:47, hubbe wrote: > > On 2016/01/15 20:07:45, DaleCurtis wrote: > > > Delete. > > > > Still need this one since it will be included in places regardless of what the > > build file says. > > (Better than having #if defined(OS_ANDROID) where this is included, right?) > > > > No, the latter is preferred and is already done. I can't think of any other > places in media (or elsewhere) where we do it the way you have here. -- That > said, I don't see any place in this CL where you're including this file in a > non-android place. What fails if you delete this? Gone. https://codereview.chromium.org/1567123002/diff/320001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1567123002/diff/320001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:12: On 2016/01/15 21:11:12, DaleCurtis wrote: > Add build/build_config.h for OS Macros. Done.
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org, sandersd@chromium.org, nasko@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1567123002/#ps340001 (title: "build fixes + comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567123002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567123002/340001
Message was sent while issue was closed.
Description was changed from ========== Support CAST+WMPI on android This is a intermediate implementation of cast support for WebMediaPlayerImpl. A better long-term solution would be to implement the same functionality on top of the presentation API. As a bonus, that would work on both mobile and desktop. However, that solution may take some time to implement, and in the meantime, this CL will unblock the spitzer project. BUG=575276 ========== to ========== Support CAST+WMPI on android This is a intermediate implementation of cast support for WebMediaPlayerImpl. A better long-term solution would be to implement the same functionality on top of the presentation API. As a bonus, that would work on both mobile and desktop. However, that solution may take some time to implement, and in the meantime, this CL will unblock the spitzer project. BUG=575276 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Support CAST+WMPI on android This is a intermediate implementation of cast support for WebMediaPlayerImpl. A better long-term solution would be to implement the same functionality on top of the presentation API. As a bonus, that would work on both mobile and desktop. However, that solution may take some time to implement, and in the meantime, this CL will unblock the spitzer project. BUG=575276 ========== to ========== Support CAST+WMPI on android This is a intermediate implementation of cast support for WebMediaPlayerImpl. A better long-term solution would be to implement the same functionality on top of the presentation API. As a bonus, that would work on both mobile and desktop. However, that solution may take some time to implement, and in the meantime, this CL will unblock the spitzer project. BUG=575276 Committed: https://crrev.com/d5f3688818eaca5f93587f4e2753d4bef4c0ab5c Cr-Commit-Position: refs/heads/master@{#369869} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/d5f3688818eaca5f93587f4e2753d4bef4c0ab5c Cr-Commit-Position: refs/heads/master@{#369869} |
