Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1076013002:
Added stub MediaSourcePlayer for developing behind the flag (Closed)
|
Created:
5 years, 8 months ago by Tima Vaisburd Modified:
5 years, 7 months ago CC:
chromium-reviews, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[RELAND] Added stub MediaSourcePlayer for developing behind the flag
Added a new experimental flag and stub implementation for
the player. Created Media thread and redirected demuxer
messages to it.
BUG=none
Committed: https://crrev.com/8ad68b12db7db320a37e39e94be8d45205fee626
Cr-Commit-Position: refs/heads/master@{#329059}
Committed: https://crrev.com/2a8c9fea422b7d5a56d40635f85cace48339b06e
Cr-Commit-Position: refs/heads/master@{#329068}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fixed binding for SetVideoSurface #
Total comments: 8
Patch Set 3 : Got rid of extra namespace, renamed players and the flag #
Total comments: 7
Patch Set 4 : Fixed WeakPtr usage, disabled thread-safe checks for demuxer's IDMap #
Total comments: 4
Patch Set 5 : Used OverrideTaskRunnerForMessage as suggested by Xiaohan #
Total comments: 3
Patch Set 6 : DemuxerAndroid is called from MediaThread only, removed the lock #Patch Set 7 : Removed unnecessary changes from non_thread_safe.h #
Total comments: 8
Patch Set 8 : Renamed old and new players, addressed some Xiaohan comments #
Total comments: 8
Patch Set 9 : Eliminated proxy player class as suggested by Xiaohan #
Total comments: 33
Patch Set 10 : TaskRunner as a member in demuxer, better macro in player #Patch Set 11 : Added missed license header. #Patch Set 12 : Fix GN build #
Total comments: 2
Patch Set 13 : A better GN compilation fix: added missed MEDIA_EXPORT #Patch Set 14 : Fixed clang build #Messages
Total messages: 73 (22 generated)
timav@google.com changed reviewers: + qinmin@chromium.org, timav@google.com
Min, please take a look. This CL just creates the place for the future development. https://codereview.chromium.org/1076013002/diff/1/content/browser/media/andro... File content/browser/media/android/browser_demuxer_android.cc (right): https://codereview.chromium.org/1076013002/diff/1/content/browser/media/andro... content/browser/media/android/browser_demuxer_android.cc:92: base::IgnoreResult(&BrowserDemuxerAndroid::OnMessageReceived), Posting the same method to another thread. Shall we have another method for another thread instead? Shall we ask to make Media thread a well known one? https://codereview.chromium.org/1076013002/diff/1/media/base/android/mt/player.h File media/base/android/mt/player.h (right): https://codereview.chromium.org/1076013002/diff/1/media/base/android/mt/playe... media/base/android/mt/player.h:17: namespace mt { Nested namespace and nested mt/ directory. "mt" stands for "multithreaded". Does it look good to you? https://codereview.chromium.org/1076013002/diff/1/media/base/media_switches.h File media/base/media_switches.h (right): https://codereview.chromium.org/1076013002/diff/1/media/base/media_switches.h... media/base/media_switches.h:24: MEDIA_EXPORT extern const char kEnableMediaPlayerV2[]; Maybe we need a better name. V2 is not very descriptive. I could not come up though.
https://codereview.chromium.org/1076013002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1076013002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:726: #if defined(OS_ANDROID) do we really need about flags for now? I think the command-line switch is good enough for development purpose. https://codereview.chromium.org/1076013002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:728: "enable-media-player-v2", Be more clear about this, rather than "enable-media-player-v2", what about "enable-media-thread-for-media-playback" https://codereview.chromium.org/1076013002/diff/20001/media/base/android/mt/p... File media/base/android/mt/player.h (right): https://codereview.chromium.org/1076013002/diff/20001/media/base/android/mt/p... media/base/android/mt/player.h:17: namespace mt { I don't think an extra namespace is needed, since the player has a different name from MediaSoucePlayer https://codereview.chromium.org/1076013002/diff/20001/media/base/android/mt/p... media/base/android/mt/player.h:23: class MEDIA_EXPORT Player : public MediaPlayerAndroid, We could use LegacyMediaSourcePlayer and MediaSourcePlayer for old and new MediaSourcePlayers.
Min, please take another look. If you like this change, I can add the actual work on top of it. https://codereview.chromium.org/1076013002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1076013002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:726: #if defined(OS_ANDROID) On 2015/04/15 18:12:47, qinmin wrote: > do we really need about flags for now? I think the command-line switch is good > enough for development purpose. Removed completely from this file https://codereview.chromium.org/1076013002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:728: "enable-media-player-v2", On 2015/04/15 18:12:47, qinmin wrote: > Be more clear about this, rather than "enable-media-player-v2", what about > "enable-media-thread-for-media-playback" I changed the name of the switch, but here it does not appear anymore. https://codereview.chromium.org/1076013002/diff/20001/media/base/android/mt/p... File media/base/android/mt/player.h (right): https://codereview.chromium.org/1076013002/diff/20001/media/base/android/mt/p... media/base/android/mt/player.h:17: namespace mt { On 2015/04/15 18:12:47, qinmin wrote: > I don't think an extra namespace is needed, since the player has a different > name from MediaSoucePlayer Extra |mt| namespace removed. https://codereview.chromium.org/1076013002/diff/20001/media/base/android/mt/p... media/base/android/mt/player.h:23: class MEDIA_EXPORT Player : public MediaPlayerAndroid, On 2015/04/15 18:12:47, qinmin wrote: > We could use LegacyMediaSourcePlayer and MediaSourcePlayer for old and new > MediaSourcePlayers. Done.
qinmin@chromium.org changed reviewers: + xhwang@chromium.org
+xhwang for media.gyp OWNER.
https://codereview.chromium.org/1076013002/diff/40001/content/browser/media/a... File content/browser/media/android/browser_demuxer_android.cc (right): https://codereview.chromium.org/1076013002/diff/40001/content/browser/media/a... content/browser/media/android/browser_demuxer_android.cc:100: handled = true; nit: no need for this line https://codereview.chromium.org/1076013002/diff/40001/media/base/android/medi... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/1076013002/diff/40001/media/base/android/medi... media/base/android/media_source_player.cc:48: weak_this_ = weak_factory_.GetWeakPtr(); weak_ptr is bind to the UI thread here. https://codereview.chromium.org/1076013002/diff/40001/media/base/android/medi... media/base/android/media_source_player.cc:61: weak_this_, base::Passed(&surface))); and the weak ptr is now being used on the media thread, which is wrong
https://codereview.chromium.org/1076013002/diff/40001/content/browser/media/a... File content/browser/media/android/browser_demuxer_android.cc (right): https://codereview.chromium.org/1076013002/diff/40001/content/browser/media/a... content/browser/media/android/browser_demuxer_android.cc:100: handled = true; On 2015/04/23 18:48:38, qinmin wrote: > nit: no need for this line Removed this line https://codereview.chromium.org/1076013002/diff/40001/media/base/android/medi... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/1076013002/diff/40001/media/base/android/medi... media/base/android/media_source_player.cc:61: weak_this_, base::Passed(&surface))); On 2015/04/23 18:48:38, qinmin wrote: > and the weak ptr is now being used on the media thread, which is wrong Yes, I learned this later, while running real implementation. Replaced with base::Unretained(this) here. https://codereview.chromium.org/1076013002/diff/60001/base/threading/non_thre... File base/threading/non_thread_safe.h (right): https://codereview.chromium.org/1076013002/diff/60001/base/threading/non_thre... base/threading/non_thread_safe.h:62: typedef NonThreadSafeDoNothing NonThreadSafe; Another problem discovered later with the real implementation is that IDMap in android demuxer complains about the wrong threads. I did not want to modify this file, but could not find the way around.
I didn't review the whole CL. Just some initial comments regarding the demuxer. https://codereview.chromium.org/1076013002/diff/40001/content/browser/media/a... File content/browser/media/android/browser_demuxer_android.h (right): https://codereview.chromium.org/1076013002/diff/40001/content/browser/media/a... content/browser/media/android/browser_demuxer_android.h:57: media::DemuxerAndroidClient* Lookup(int demuxer_client_id); nit: Something like GetDemuxerClient() to be consistent with other functions? https://codereview.chromium.org/1076013002/diff/40001/content/browser/media/a... content/browser/media/android/browser_demuxer_android.h:61: base::Lock lock_; Add a comment why we need this and what needs to be protected by this lock. https://codereview.chromium.org/1076013002/diff/60001/content/browser/media/a... File content/browser/media/android/browser_demuxer_android.cc (right): https://codereview.chromium.org/1076013002/diff/60001/content/browser/media/a... content/browser/media/android/browser_demuxer_android.cc:93: this, message)); Can you use OverrideTaskRunnerForMessage() return the media thread task runner directly? Then all messages will be redirected to the media thread. Actually in that case, just use OverrideTaskRunnerForMessage() for both cases. In the new code, return media thread task runner, in the old code, return UI thread task runner. https://codereview.chromium.org/1076013002/diff/60001/content/browser/media/a... content/browser/media/android/browser_demuxer_android.cc:134: base::AutoLock lock(lock_); This whole class should still live on one single thread, either the media thread, or the UI thread. I don't quite follow why we need a lock here....
https://codereview.chromium.org/1076013002/diff/60001/content/browser/media/a... File content/browser/media/android/browser_demuxer_android.cc (right): https://codereview.chromium.org/1076013002/diff/60001/content/browser/media/a... content/browser/media/android/browser_demuxer_android.cc:134: base::AutoLock lock(lock_); On 2015/04/24 05:04:13, xhwang wrote: > This whole class should still live on one single thread, either the media > thread, or the UI thread. I don't quite follow why we need a lock here.... Xiaohan, thank you for looking into this! In this stub (and admittedly not very interesting) implementation the methods AddDemuxerClient() and RemoveDemuxerClient() are called on the UI thread, while others - on the Media thread. That's why I added the lock. In the real implementation we destroy the player (which is the demuxer client) on the Media thread, and thus call RemoveDemuxerClient on the Media thread. But construction and the call to Add still happens on the UI thread. Can we call AddDemuxerClient() later without a danger to skip demuxer messages?
I talked with Min and he pointed out that even now, with the lock, the code has a problem that the player creation in UI thread might happen later than OnDemuxerReady comes to the Media thread. I think we can address the issue such that the lock can be removed.
https://codereview.chromium.org/1076013002/diff/80001/base/threading/non_thre... File base/threading/non_thread_safe.h (right): https://codereview.chromium.org/1076013002/diff/80001/base/threading/non_thre... base/threading/non_thread_safe.h:61: #if defined(DISABLE_NON_THREAD_SAFE) why we need this here? https://codereview.chromium.org/1076013002/diff/80001/content/browser/media/a... File content/browser/media/android/browser_demuxer_android.h (right): https://codereview.chromium.org/1076013002/diff/80001/content/browser/media/a... content/browser/media/android/browser_demuxer_android.h:8: #define DISABLE_NON_THREAD_SAFE why cannot we just define ENABLE_THREAD_SAFE 0? And do we have to stick to id_map?
Please take a look at the version that calls demuxer on one thread (which is the Media thread with a new player). I added a new MediaSourcePlayerImpl file which is a part of implementation to explain how we call AddDemuxerClient and RemoveDemuxerClient(). https://codereview.chromium.org/1076013002/diff/80001/content/browser/media/a... File content/browser/media/android/browser_demuxer_android.h (right): https://codereview.chromium.org/1076013002/diff/80001/content/browser/media/a... content/browser/media/android/browser_demuxer_android.h:8: #define DISABLE_NON_THREAD_SAFE On 2015/04/27 22:16:20, qinmin wrote: > why cannot we just define ENABLE_THREAD_SAFE 0? And do we have to stick to > id_map? I did it here because ENABLE_THREAD_SAFE seemed to be redefined within id_map.h, in non_thread_safe.h which is included inside. But this all should be obsolete now, with Patch Set 7 we do not need these hacks.
lgtm on media/base/android. For BrowserDemuxerAndroid, it's unfortunate that OnDemuxerReady could arrive before Initialize().
On 2015/04/28 17:53:06, qinmin wrote: > lgtm on media/base/android. > For BrowserDemuxerAndroid, it's unfortunate that OnDemuxerReady could arrive > before Initialize(). Thank you. Does the change for BrowserDemuxerAndroid seems reasonable to you though? One question with respect to media/base: I feel a little bit uneasy to rename the current player to "Legacy" before the new player starts working for real. Can we keep the old name and have something like MediaThreadSourcePlayer for a new one for now?
On 2015/04/28 18:30:52, timav wrote: > On 2015/04/28 17:53:06, qinmin wrote: > > lgtm on media/base/android. > > For BrowserDemuxerAndroid, it's unfortunate that OnDemuxerReady could arrive > > before Initialize(). > > Thank you. Does the change for BrowserDemuxerAndroid seems reasonable to you > though? > > One question with respect to media/base: I feel a little bit uneasy to rename > the current player to "Legacy" before the new player starts working for real. > Can we keep the old name and have something like MediaThreadSourcePlayer for a > new one for now? I am ok with your BrowserDemuxerAndroid change, but will let xhwang to take another look. For now, we can use MediaSourcePlayerWithDedicatedThread for the naming.
On 2015/04/28 18:34:53, qinmin wrote: > On 2015/04/28 18:30:52, timav wrote: > > On 2015/04/28 17:53:06, qinmin wrote: > > > lgtm on media/base/android. > > > For BrowserDemuxerAndroid, it's unfortunate that OnDemuxerReady could arrive > > > before Initialize(). > > > > Thank you. Does the change for BrowserDemuxerAndroid seems reasonable to you > > though? > > > > One question with respect to media/base: I feel a little bit uneasy to rename > > the current player to "Legacy" before the new player starts working for real. > > Can we keep the old name and have something like MediaThreadSourcePlayer for a > > new one for now? > > I am ok with your BrowserDemuxerAndroid change, but will let xhwang to take > another look. > For now, we can use MediaSourcePlayerWithDedicatedThread for the naming. I don't have bandwidth to review this today. I'll try to do it tonight/tomorrow.
https://codereview.chromium.org/1076013002/diff/120001/content/browser/media/... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/1076013002/diff/120001/content/browser/media/... content/browser/media/android/browser_media_player_manager.cc:134: media_player_params.frame_url); nit: Use braces for complicated if/else statements. https://codereview.chromium.org/1076013002/diff/120001/media/base/android/med... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/1076013002/diff/120001/media/base/android/med... media/base/android/media_source_player.cc:19: MediaThread() : base::Thread("MediaThread") { We also have a media thread on desktop in the render process. To make it clear, can we call this "BrowserMediaThread". https://codereview.chromium.org/1076013002/diff/120001/media/base/android/med... media/base/android/media_source_player.cc:72: base::Unretained(impl_), base::Passed(&surface))); Now the messages from the render process are first dispatched on the IO thread, then sent to the UI thread, then posted to the media thread. This sounds bad. Previously I planned to move MediaWebContentsObserver to use BrowserMessageFilter (see https://code.google.com/p/chromium/issues/detail?id=388930). But I never had a chance to finish that. With BrowserMessageFilter, you can dispatch messages to the media thread directly, same as what you did for BrowserDemuxerAndroid. Then you have better performance, and you don't need MSPI. Will that be an option for you?
https://codereview.chromium.org/1076013002/diff/120001/content/browser/media/... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/1076013002/diff/120001/content/browser/media/... content/browser/media/android/browser_media_player_manager.cc:134: media_player_params.frame_url); On 2015/04/29 23:50:51, xhwang wrote: > nit: Use braces for complicated if/else statements. Done. https://codereview.chromium.org/1076013002/diff/120001/media/base/android/med... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/1076013002/diff/120001/media/base/android/med... media/base/android/media_source_player.cc:19: MediaThread() : base::Thread("MediaThread") { On 2015/04/29 23:50:51, xhwang wrote: > We also have a media thread on desktop in the render process. To make it clear, > can we call this "BrowserMediaThread". I did this. I have a small question though: I though the name here is only for the crash report, and isn't it rather obvious what process did crash? https://codereview.chromium.org/1076013002/diff/120001/media/base/android/med... media/base/android/media_source_player.cc:72: base::Unretained(impl_), base::Passed(&surface))); On 2015/04/29 23:50:51, xhwang wrote: > Now the messages from the render process are first dispatched on the IO thread, > then sent to the UI thread, then posted to the media thread. This sounds bad. Except for SetVideoSurface, which comes directly from UI thread, yes. > With BrowserMessageFilter, you can dispatch messages to the media thread > directly, same as what you did for BrowserDemuxerAndroid. Then you have better > performance, and you don't need MSPI. > > Will that be an option for you? I talked to Min. He agreed it would be better, but suggested we limit the scope if this refactoring to the MediaSourcePlayer only and moving the BrowserMediaPlayerManager should be a separate CL. There are two kinds of players, MediaSourcePlayer and native (MediaPlayerBridge). Moving the native might cause problems. Would you agree with this staged approach? As far as I understand the performance penalty for IO -> UI -> Media threads hopping will only happen to the user's commands that happen rarely (with the exception of SeekTo).
Sorry for the delay. Some more discussions... https://codereview.chromium.org/1076013002/diff/120001/media/base/android/med... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/1076013002/diff/120001/media/base/android/med... media/base/android/media_source_player.cc:19: MediaThread() : base::Thread("MediaThread") { On 2015/04/30 21:18:50, timav wrote: > On 2015/04/29 23:50:51, xhwang wrote: > > We also have a media thread on desktop in the render process. To make it > clear, > > can we call this "BrowserMediaThread". > > I did this. I have a small question though: I though the name here is only for > the crash report, and isn't it rather obvious what process did crash? Some time I print thread names in my local build to monitor threads. I agree this won't affect prod :) https://codereview.chromium.org/1076013002/diff/120001/media/base/android/med... media/base/android/media_source_player.cc:72: base::Unretained(impl_), base::Passed(&surface))); On 2015/04/30 21:18:50, timav wrote: > On 2015/04/29 23:50:51, xhwang wrote: > > Now the messages from the render process are first dispatched on the IO > thread, > > then sent to the UI thread, then posted to the media thread. This sounds bad. > > Except for SetVideoSurface, which comes directly from UI thread, yes. > > > With BrowserMessageFilter, you can dispatch messages to the media thread > > directly, same as what you did for BrowserDemuxerAndroid. Then you have better > > performance, and you don't need MSPI. > > > > Will that be an option for you? > > I talked to Min. He agreed it would be better, but suggested we limit the scope > if this refactoring to the MediaSourcePlayer only and moving the > BrowserMediaPlayerManager should be a separate CL. There are two kinds of > players, MediaSourcePlayer and native (MediaPlayerBridge). Moving the native > might cause problems. Agreed that should be in a separate CL and we can investigate that later. > Would you agree with this staged approach? As far as I understand the > performance penalty for IO -> UI -> Media threads hopping will only happen to > the user's commands that happen rarely (with the exception of SeekTo). https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... File media/base/android/media_source_player_with_dedicated_thread.cc (right): https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... media/base/android/media_source_player_with_dedicated_thread.cc:74: } Can you just do something like: void MediaSourcePlayer::SetVideoSurface() { if (NotOnMediaTaskRunner()) { GetMediaTaskRunner()->PostTask( FROM_HERE, base::Bind(&MediaSourcePlayerImpl::SetVideoSurface, weak_factory_.GetWeakPtr(), base::Passed(&surface))); return; } // Do work. } Then you don't need MediaSourcePlayerWithDedicatedThread. https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... media/base/android/media_source_player_with_dedicated_thread.cc:76: void MediaSourcePlayerWithDedicatedThread::Start() { I am not a fan of the name MediaSourcePlayerImpl and MediaSourcePlayerWithDedicatedThread. It's really not an implementation of MediaSourcePlayer :) Actually if you think of it, it's not limited to MediaSource (MSE spec). If I use a normal demuxer (instead of the ChunkDemuxer) at the render side and send demuxed buffers to the browser, this player will still work. With that, maybe we can just start with a new name. This is really just a MediaCodecBridge based MediaPlayerAndroid implementation. How about MediaCodecPlayer? Or if you have better names go for it!
https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... File media/base/android/media_source_player_with_dedicated_thread.cc (right): https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... media/base/android/media_source_player_with_dedicated_thread.cc:74: } On 2015/05/01 06:55:05, xhwang wrote: > Can you just do something like: > > void MediaSourcePlayer::SetVideoSurface() { > if (NotOnMediaTaskRunner()) { > GetMediaTaskRunner()->PostTask( > FROM_HERE, base::Bind(&MediaSourcePlayerImpl::SetVideoSurface, > weak_factory_.GetWeakPtr(), > base::Passed(&surface))); > return; > } > > // Do work. > } > > Then you don't need MediaSourcePlayerWithDedicatedThread. Sorry, I did not understand your point here. The main reason to have two classes (MSPWDT and MSPI, I agree with what you said about the naming, they are actually PlayerFacade and Player) is the desire to properly stop and destroy the Player on the Media thread. In the destructor of the PlayerFacade I do not delete the |impl_|, the Player destruction is postponed and it happens on Media thread (decoder threads stopped, then MediaCodecs released, surface released, then we delete the Player). Initially we wanted to use a scoped_ptr with Deleter, but first, we did not want to modify BrowserMediaPlayerManager which uses std::vector<raw pointer>, and second, I did not find a way to push a scoped_ptr into std::vector (no copy constructor). Another thing is that the current MediaSourcePlayer should remain, Grace advised on gradually switching between them for some % of user base. Did you mean to have the current MediaSourcePlayer redirect some calls to MSPI when the command line flag is set? https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... media/base/android/media_source_player_with_dedicated_thread.cc:76: void MediaSourcePlayerWithDedicatedThread::Start() { On 2015/05/01 06:55:05, xhwang wrote: > I am not a fan of the name MediaSourcePlayerImpl and > MediaSourcePlayerWithDedicatedThread. It's really not an implementation of > MediaSourcePlayer :) > > Actually if you think of it, it's not limited to MediaSource (MSE spec). If I > use a normal demuxer (instead of the ChunkDemuxer) at the render side and send > demuxed buffers to the browser, this player will still work. With that, maybe we > can just start with a new name. This is really just a MediaCodecBridge based > MediaPlayerAndroid implementation. How about MediaCodecPlayer? Or if you have > better names go for it! I completely agree :-) I will get in agreement with Min about the naming and rename.
https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... File media/base/android/media_source_player_with_dedicated_thread.cc (right): https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... media/base/android/media_source_player_with_dedicated_thread.cc:76: void MediaSourcePlayerWithDedicatedThread::Start() { On 2015/05/01 18:14:10, timav wrote: > On 2015/05/01 06:55:05, xhwang wrote: > > I am not a fan of the name MediaSourcePlayerImpl and > > MediaSourcePlayerWithDedicatedThread. It's really not an implementation of > > MediaSourcePlayer :) > > > > Actually if you think of it, it's not limited to MediaSource (MSE spec). If I > > use a normal demuxer (instead of the ChunkDemuxer) at the render side and send > > demuxed buffers to the browser, this player will still work. With that, maybe > we > > can just start with a new name. This is really just a MediaCodecBridge based > > MediaPlayerAndroid implementation. How about MediaCodecPlayer? Or if you have > > better names go for it! > > I completely agree :-) > I will get in agreement with Min about the naming and rename. MediaCodecPlayer sgtm
https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... File media/base/android/media_source_player_with_dedicated_thread.cc (right): https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... media/base/android/media_source_player_with_dedicated_thread.cc:74: } Sorry for the confusion. I was just proposing to merge MediaSourcePlayerWithDedicatedThread and MediaSourcePlayerImpl. Your current CL works but I wonder whether that's necessary. See comments inline: On 2015/05/01 18:14:10, timav wrote: > On 2015/05/01 06:55:05, xhwang wrote: > > Can you just do something like: > > > > void MediaSourcePlayer::SetVideoSurface() { > > if (NotOnMediaTaskRunner()) { > > GetMediaTaskRunner()->PostTask( > > FROM_HERE, base::Bind(&MediaSourcePlayerImpl::SetVideoSurface, > > weak_factory_.GetWeakPtr(), > > base::Passed(&surface))); > > return; > > } > > > > // Do work. > > } > > > > Then you don't need MediaSourcePlayerWithDedicatedThread. > > Sorry, I did not understand your point here. > > The main reason to have two classes (MSPWDT and MSPI, I agree with what you said > about the naming, they are actually PlayerFacade and Player) is the desire to > properly stop and destroy the Player on the Media thread. In the destructor of > the PlayerFacade I do not delete the |impl_|, the Player destruction is > postponed and it happens on Media thread (decoder threads stopped, then > MediaCodecs released, surface released, then we delete the Player). Can you use DeleteSoon for delayed deletion on the media thread? > Initially we wanted to use a scoped_ptr with Deleter, but first, we did not want > to modify BrowserMediaPlayerManager which uses std::vector<raw pointer>, and > second, I did not find a way to push a scoped_ptr into std::vector (no copy > constructor). I am not following. But you can use ScopedVector if you want to store scoped_ptr in a vector. > Another thing is that the current MediaSourcePlayer should remain, Grace advised > on gradually switching between them for some % of user base. Totally agreed. We should keep MediaSourcePlayer, and have MediaCodecPlayer at the same time (behind the flag). > Did you mean to have the current MediaSourcePlayer redirect some calls to MSPI > when the command line flag is set? No. I suggested to have MediaCodecPlayer posting tasks from UI to Media thread by itself, instead having another MediaSourcePlayerWithDedicatedThread class. You can see examples at: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
On 2015/05/01 20:26:35, xhwang wrote: > No. I suggested to have MediaCodecPlayer posting tasks from UI to Media thread > by itself, instead having another MediaSourcePlayerWithDedicatedThread class. > You can see examples at: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Ah, I see. But deletion? BrowserMediaPlayerManager deletes the player on UI thread like this: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... destructor is called on UI thread, I need to stop decoder threads but cannot call join() on UI thread. > DeleteSoon I think it is more suited to the next CL mentioned above when we modify or get rid of BrowserMediaPlayerManager?
On 2015/05/01 22:38:59, timav wrote: > On 2015/05/01 20:26:35, xhwang wrote: > > No. I suggested to have MediaCodecPlayer posting tasks from UI to Media thread > > by itself, instead having another MediaSourcePlayerWithDedicatedThread class. > > You can see examples at: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > Ah, I see. But deletion? > > BrowserMediaPlayerManager deletes the player on UI thread like this: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... > > destructor is called on UI thread, I need to stop decoder threads but cannot > call join() on UI thread. > > > > DeleteSoon > > I think it is more suited to the next CL mentioned above when we modify or get > rid of BrowserMediaPlayerManager? It seems we can do this easily. In BrowserMediaPlayerManager, instead of calling erase(), you can do a weak_erase(). Then instead of "delete player", do something like player->DeleteOnCorrectThread(). Here's an example: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ho... Since this isn't too hard, I'd prefer we don't introduce redundant file/code and then delete it very soon. Will that work for you?
On 2015/05/01 23:06:21, xhwang wrote: > On 2015/05/01 22:38:59, timav wrote: > > On 2015/05/01 20:26:35, xhwang wrote: > > > No. I suggested to have MediaCodecPlayer posting tasks from UI to Media > thread > > > by itself, instead having another MediaSourcePlayerWithDedicatedThread > class. > > > You can see examples at: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > Ah, I see. But deletion? > > > > BrowserMediaPlayerManager deletes the player on UI thread like this: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... > > > > destructor is called on UI thread, I need to stop decoder threads but cannot > > call join() on UI thread. > > > > > > > DeleteSoon > > > > I think it is more suited to the next CL mentioned above when we modify or get > > rid of BrowserMediaPlayerManager? > > It seems we can do this easily. In BrowserMediaPlayerManager, instead of calling > erase(), you can do a weak_erase(). Then instead of "delete player", do > something like player->DeleteOnCorrectThread(). Here's an example: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ho... > > Since this isn't too hard, I'd prefer we don't introduce redundant file/code and > then delete it very soon. Will that work for you? Xiaohan, thank you for your suggestion, I missed weak_erase(). I will give it a try.
Min and Xiaohan, I combined two player classes into one by using DeleteOnCorrectThread() and re-posting some methods to the media thread. The testing revealed that the MediaPlayerListener has to be released on the same thread that created it (i.e. UI) ( see https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...) Please take another look. https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... File media/base/android/media_source_player_with_dedicated_thread.cc (right): https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... media/base/android/media_source_player_with_dedicated_thread.cc:74: } On 2015/05/01 20:26:35, xhwang wrote: > Sorry for the confusion. I was just proposing to merge > MediaSourcePlayerWithDedicatedThread and MediaSourcePlayerImpl. Your current CL > works but I wonder whether that's necessary. See comments inline: > > On 2015/05/01 18:14:10, timav wrote: > > On 2015/05/01 06:55:05, xhwang wrote: > > > Can you just do something like: > > > > > > void MediaSourcePlayer::SetVideoSurface() { > > > if (NotOnMediaTaskRunner()) { > > > GetMediaTaskRunner()->PostTask( > > > FROM_HERE, base::Bind(&MediaSourcePlayerImpl::SetVideoSurface, > > > weak_factory_.GetWeakPtr(), > > > base::Passed(&surface))); > > > return; > > > } > > > > > > // Do work. > > > } > > > > > > Then you don't need MediaSourcePlayerWithDedicatedThread. > > > > Sorry, I did not understand your point here. > > > > The main reason to have two classes (MSPWDT and MSPI, I agree with what you > said > > about the naming, they are actually PlayerFacade and Player) is the desire to > > properly stop and destroy the Player on the Media thread. In the destructor of > > the PlayerFacade I do not delete the |impl_|, the Player destruction is > > postponed and it happens on Media thread (decoder threads stopped, then > > MediaCodecs released, surface released, then we delete the Player). > > Can you use DeleteSoon for delayed deletion on the media thread? > > > Initially we wanted to use a scoped_ptr with Deleter, but first, we did not > want > > to modify BrowserMediaPlayerManager which uses std::vector<raw pointer>, and > > second, I did not find a way to push a scoped_ptr into std::vector (no copy > > constructor). > > I am not following. But you can use ScopedVector if you want to store scoped_ptr > in a vector. > > > Another thing is that the current MediaSourcePlayer should remain, Grace > advised > > on gradually switching between them for some % of user base. > > Totally agreed. We should keep MediaSourcePlayer, and have MediaCodecPlayer at > the same time (behind the flag). > > > Did you mean to have the current MediaSourcePlayer redirect some calls to MSPI > > when the command line flag is set? > > No. I suggested to have MediaCodecPlayer posting tasks from UI to Media thread > by itself, instead having another MediaSourcePlayerWithDedicatedThread class. > You can see examples at: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Done. https://codereview.chromium.org/1076013002/diff/140001/media/base/android/med... media/base/android/media_source_player_with_dedicated_thread.cc:76: void MediaSourcePlayerWithDedicatedThread::Start() { On 2015/05/01 19:12:14, qinmin wrote: > On 2015/05/01 18:14:10, timav wrote: > > On 2015/05/01 06:55:05, xhwang wrote: > > > I am not a fan of the name MediaSourcePlayerImpl and > > > MediaSourcePlayerWithDedicatedThread. It's really not an implementation of > > > MediaSourcePlayer :) > > > > > > Actually if you think of it, it's not limited to MediaSource (MSE spec). If > I > > > use a normal demuxer (instead of the ChunkDemuxer) at the render side and > send > > > demuxed buffers to the browser, this player will still work. With that, > maybe > > we > > > can just start with a new name. This is really just a MediaCodecBridge based > > > MediaPlayerAndroid implementation. How about MediaCodecPlayer? Or if you > have > > > better names go for it! > > > > I completely agree :-) > > I will get in agreement with Min about the naming and rename. > > MediaCodecPlayer sgtm Done.
Looking pretty good! Just a few more comments. https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... File content/browser/media/android/browser_demuxer_android.cc (right): https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:9: #include "media/base/android/media_codec_player.h" hmm, wondering why we need this... https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:61: HasSwitch(switches::kEnableMediaThreadForMediaPlayback); Actually, how about this. Instead of having a enable_media_thread_for_media_playback_ member, just have a task_runner_ member, which will be initialized in the ctor. Then in OverrideTaskRunnerForMessage(), you can just return the task_runner_. With that, you can DCHECK we are on the correct thread in each member functions, which will make the threading model more clear. https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:80: return nullptr; When nullptr is returned, a message will be dispatched on the IO thread, which is unexpected. Add NOTREACHED() before return? https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:83: nit: remove extra line. https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:105: void BrowserDemuxerAndroid::AddDemuxerClient( In what case will this happen after OnDemuxerReady? If we can avoid this, we shouldn't need the |pending_configs_|? https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... File content/browser/media/android/browser_demuxer_android.h (right): https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.h:62: ConfigsPerClient pending_configs_; Please add a comment why we need this. https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.h:64: bool enable_media_thread_for_media_playback_; nit: Make this a const? https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_media_player_manager.cc:37: using media::MediaCodecPlayer; order https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:7: #define POST_TO_MEDIA_THREAD(closure) \ This doesn't always "post". How about POST_TO_MEDIA_THREAD_IF_NEEDED? Actually I am not sure whether this improves readability. Please see comments below. https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:88: void MediaCodecPlayer::SetVideoSurface(gfx::ScopedJavaSurface surface) { Use NOTIMPLEMENTED() in these functions. https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:89: POST_TO_MEDIA_THREAD( I don't find this macro very readable, maybe because it's missing the "return", especially when you have this in the middle of a function, e.g. on l.101... https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:99: AttachListener(NULL); Seems to worth a comment. https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:140: // UI thread Here and below, DCHECK it's called on the correct thread so you don't need these comments. https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_codec_player.h:25: // This class implements the media player using Android's MediaCodec. nit: Add a comment about the diff b/w this and the MediaSourcePlayer, just to avoid confusion. https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... File media/base/android/media_player_android.h (right): https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_player_android.h:109: void DestroyListener(); When do we need to call this?
https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... File content/browser/media/android/browser_demuxer_android.cc (right): https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:9: #include "media/base/android/media_codec_player.h" On 2015/05/06 17:42:44, xhwang wrote: > hmm, wondering why we need this... media::GetMediaTaskRunner() is defined there https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:61: HasSwitch(switches::kEnableMediaThreadForMediaPlayback); On 2015/05/06 17:42:44, xhwang wrote: > Actually, how about this. > > Instead of having a enable_media_thread_for_media_playback_ member, just have a > task_runner_ member, which will be initialized in the ctor. Then in > OverrideTaskRunnerForMessage(), you can just return the task_runner_. > > With that, you can DCHECK we are on the correct thread in each member functions, > which will make the threading model more clear. Done. https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:80: return nullptr; On 2015/05/06 17:42:44, xhwang wrote: > When nullptr is returned, a message will be dispatched on the IO thread, which > is unexpected. > > Add NOTREACHED() before return? Actually, I saw some messages there. I did not identify them by name, but I believe they were other media player messages that were later dispatched on UI thread. Can it be that the message filters form a chain? https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:83: On 2015/05/06 17:42:44, xhwang wrote: > nit: remove extra line. Done. https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:105: void BrowserDemuxerAndroid::AddDemuxerClient( On 2015/05/06 17:42:44, xhwang wrote: > In what case will this happen after OnDemuxerReady? If we can avoid this, we > shouldn't need the |pending_configs_|? As far as I understand, Initialize message arrives on UI thread and we construct an object, and the next DemuxerReady message arrives on Media thread. Min pointed out that already at this point we do not know which will be processed at an earlier time. The player ctor posts the message on the Media thread, and it is that handler of this message that calls AddDemuxerClient(). This delays AddDemuxerClient() even further. Therefore, it seems, |pending_configs_| is necessary. I added some comments in the code, although shorter than this. https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... File content/browser/media/android/browser_demuxer_android.h (right): https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.h:62: ConfigsPerClient pending_configs_; On 2015/05/06 17:42:44, xhwang wrote: > Please add a comment why we need this. Done. https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.h:64: bool enable_media_thread_for_media_playback_; On 2015/05/06 17:42:44, xhwang wrote: > nit: Make this a const? Eliminated, replaced with task_runner_ https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_media_player_manager.cc:37: using media::MediaCodecPlayer; On 2015/05/06 17:42:44, xhwang wrote: > order Done. https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:7: #define POST_TO_MEDIA_THREAD(closure) \ On 2015/05/06 17:42:44, xhwang wrote: > This doesn't always "post". How about POST_TO_MEDIA_THREAD_IF_NEEDED? > > Actually I am not sure whether this improves readability. Please see comments > below. I tried to make it better. Please see the comments below and the code. https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:88: void MediaCodecPlayer::SetVideoSurface(gfx::ScopedJavaSurface surface) { On 2015/05/06 17:42:44, xhwang wrote: > Use NOTIMPLEMENTED() in these functions. Done. https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:89: POST_TO_MEDIA_THREAD( On 2015/05/06 17:42:44, xhwang wrote: > I don't find this macro very readable, maybe because it's missing the "return", > especially when you have this in the middle of a function, e.g. on l.101... Yes. I pulled the base::Bind into the macro, and use it only if it's at the beginning of the method. Changed the name to RUN_ON_MEDIA_THREAD. Please take another look. https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:99: AttachListener(NULL); On 2015/05/06 17:42:44, xhwang wrote: > Seems to worth a comment. A closer look at the AttachListener/DetachListener revealed that I use them incorrectly here. They should be called from different places when we actually start and stop playing. I removed them completely for now, it also helps to use the macro uniformly. https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:140: // UI thread On 2015/05/06 17:42:44, xhwang wrote: > Here and below, DCHECK it's called on the correct thread so you don't need these > comments. Done. https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_codec_player.h:25: // This class implements the media player using Android's MediaCodec. On 2015/05/06 17:42:44, xhwang wrote: > nit: Add a comment about the diff b/w this and the MediaSourcePlayer, just to > avoid confusion. Added some words, please take another look https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... File media/base/android/media_player_android.h (right): https://codereview.chromium.org/1076013002/diff/160001/media/base/android/med... media/base/android/media_player_android.h:109: void DestroyListener(); On 2015/05/06 17:42:44, xhwang wrote: > When do we need to call this? Changed the name to DestroyListenerOnUIThread and added a comment
Thanks for the clarifications and thanks for the patience! LGTM https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... File content/browser/media/android/browser_demuxer_android.cc (right): https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:9: #include "media/base/android/media_codec_player.h" On 2015/05/08 07:30:53, Tima Vaisburd wrote: > On 2015/05/06 17:42:44, xhwang wrote: > > hmm, wondering why we need this... > > media::GetMediaTaskRunner() is defined there It's a bit odd since BDA can also be used by MediaSourcePlayer. It's not a big issue though. https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:80: return nullptr; On 2015/05/08 07:30:53, Tima Vaisburd wrote: > On 2015/05/06 17:42:44, xhwang wrote: > > When nullptr is returned, a message will be dispatched on the IO thread, which > > is unexpected. > > > > Add NOTREACHED() before return? > > Actually, I saw some messages there. I did not identify them by name, but I > believe they were other media player messages that were later dispatched on UI > thread. Can it be that the message filters form a chain? Acknowledged. You are right. I believe the dispatcher tries each MessageFilter to see whether the message can be handled by this MF. https://codereview.chromium.org/1076013002/diff/160001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:105: void BrowserDemuxerAndroid::AddDemuxerClient( On 2015/05/08 07:30:53, Tima Vaisburd wrote: > On 2015/05/06 17:42:44, xhwang wrote: > > In what case will this happen after OnDemuxerReady? If we can avoid this, we > > shouldn't need the |pending_configs_|? > > As far as I understand, Initialize message arrives on UI thread and we construct > an object, and the next DemuxerReady message arrives on Media thread. Min > pointed out that already at this point we do not know which > will be processed at an earlier time. The player ctor posts the message on the > Media thread, and it is that handler of this message that calls > AddDemuxerClient(). This delays AddDemuxerClient() even further. Therefore, it > seems, |pending_configs_| is necessary. > I added some comments in the code, although shorter than this. Acknowledged.
On 2015/05/08 17:12:36, xhwang wrote: > Thanks for the clarifications and thanks for the patience! LGTM I wouldn't call this patience, your comments were/are right to the point and the work was required! Thank you.
The CQ bit was checked by timav@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org Link to the patchset: https://codereview.chromium.org/1076013002/#ps180001 (title: "TaskRunner as a member in demuxer, better macro in player")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1076013002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by timav@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/1076013002/#ps200001 (title: "Added missed license header.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1076013002/200001
The CQ bit was unchecked by timav@google.com
The CQ bit was checked by timav@google.com
The CQ bit was checked by timav@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/1076013002/#ps220001 (title: "Fix GN build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1076013002/220001
sievers@chromium.org changed reviewers: + sievers@chromium.org
https://codereview.chromium.org/1076013002/diff/220001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/1076013002/diff/220001/content/browser/BUILD.... content/browser/BUILD.gn:348: "//media/base/android:android", Why doesn't content_browser depend on this in gyp? The gyp dependencies for player_android are weird.
https://codereview.chromium.org/1076013002/diff/220001/content/browser/media/... File content/browser/media/android/browser_demuxer_android.cc (right): https://codereview.chromium.org/1076013002/diff/220001/content/browser/media/... content/browser/media/android/browser_demuxer_android.cc:65: media::GetMediaTaskRunner().get() : Without the BUILD.gn change in content the linker could not find the method media::GetMediaTaskRunner(). Maybe dependencies work differently in gyp and gn?
On 2015/05/08 22:32:01, timav wrote: > https://codereview.chromium.org/1076013002/diff/220001/content/browser/media/... > File content/browser/media/android/browser_demuxer_android.cc (right): > > https://codereview.chromium.org/1076013002/diff/220001/content/browser/media/... > content/browser/media/android/browser_demuxer_android.cc:65: > media::GetMediaTaskRunner().get() : > Without the BUILD.gn change in content the linker could not find the method > media::GetMediaTaskRunner(). > > Maybe dependencies work differently in gyp and gn? Something made you decide to add the dependency in BUILD.gn for this target instead of matching the .gyp logic. Can you explain? I'm just wondering why this is different. I'm not saying that necessarily the way we depend on it in various places from .gyp is right. But maybe we can make it match somewhat?
On 2015/05/08 22:34:10, sievers wrote: > On 2015/05/08 22:32:01, timav wrote: > > > https://codereview.chromium.org/1076013002/diff/220001/content/browser/media/... > > File content/browser/media/android/browser_demuxer_android.cc (right): > > > > > https://codereview.chromium.org/1076013002/diff/220001/content/browser/media/... > > content/browser/media/android/browser_demuxer_android.cc:65: > > media::GetMediaTaskRunner().get() : > > Without the BUILD.gn change in content the linker could not find the method > > media::GetMediaTaskRunner(). > > > > Maybe dependencies work differently in gyp and gn? > > Something made you decide to add the dependency in BUILD.gn for this target > instead of matching the .gyp logic. > Can you explain? > > I'm just wondering why this is different. I'm not saying that necessarily the > way we depend on it in various places from .gyp is right. But maybe we can make > it match somewhat? Ok based on our chat, suggest looking at why in GYP we successfully depend content_common -> media -> media/base/android (aka player_android), while this doesn't work in GN (although it looks like it should; maybe to do with order of linking or so?).
On 2015/05/08 23:00:26, sievers wrote: > On 2015/05/08 22:34:10, sievers wrote: > > On 2015/05/08 22:32:01, timav wrote: > > > > > > https://codereview.chromium.org/1076013002/diff/220001/content/browser/media/... > > > File content/browser/media/android/browser_demuxer_android.cc (right): > > > > > > > > > https://codereview.chromium.org/1076013002/diff/220001/content/browser/media/... > > > content/browser/media/android/browser_demuxer_android.cc:65: > > > media::GetMediaTaskRunner().get() : > > > Without the BUILD.gn change in content the linker could not find the method > > > media::GetMediaTaskRunner(). > > > > > > Maybe dependencies work differently in gyp and gn? > > > > Something made you decide to add the dependency in BUILD.gn for this target > > instead of matching the .gyp logic. > > Can you explain? > > > > I'm just wondering why this is different. I'm not saying that necessarily the > > way we depend on it in various places from .gyp is right. But maybe we can > make > > it match somewhat? > > Ok based on our chat, suggest looking at why in GYP we successfully depend > content_common -> media -> media/base/android (aka player_android), while this > doesn't work in GN (although it looks like it should; maybe to do with order of > linking or so?). Daniel, you were absolutely right, it was a missing MEDIA_EXPORT in the code. Sorry for the trouble.
The CQ bit was checked by timav@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/1076013002/#ps240001 (title: "A better GN compilation fix: added missed MEDIA_EXPORT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1076013002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by timav@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1076013002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by timav@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1076013002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/8ad68b12db7db320a37e39e94be8d45205fee626 Cr-Commit-Position: refs/heads/master@{#329059}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/1132973002/ by miu@chromium.org. The reason for reverting is: Broke Android Clang Builder (dbg): http://build.chromium.org/p/chromium.linux/buildstatus?builder=Android%20Clan....
The CQ bit was checked by timav@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/1076013002/#ps260001 (title: "Fixed clang build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1076013002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by timav@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1076013002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/2a8c9fea422b7d5a56d40635f85cace48339b06e Cr-Commit-Position: refs/heads/master@{#329068} |
