Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1128383003:
Implementation of MediaCodecPlayer stage 1 (Closed)
|
Created:
5 years, 7 months ago by Tima Vaisburd Modified:
5 years, 6 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. |
DescriptionImplementation of MediaCodecPlayer stage 1
The CL implements the basic playback with pause/resume till
completion only (no seek, dynamic configuration, surface change,
encryption).
This player uses one long-lived Media thread for the IPC
with demuxer and for the player's internal operations.
The player starts and stops one audio and one video decoder
thread as necessary.
The MediaCodec input and output buffers operations
(dequeue/enqueue/release) as well as the rendering of the
output buffers all happen on the same decoder thread.
Closing since this CL as been splitted into 3 smaller ones:
https://codereview.chromium.org/1162203009/
https://codereview.chromium.org/1176993005/
https://codereview.chromium.org/1184373005/
BUG=407577
Patch Set 1 #
Total comments: 3
Patch Set 2 : Changed stop procedure, fixed compilation #
Total comments: 30
Patch Set 3 : Addressed Min comments, added DCHECKs for current threads #
Total comments: 27
Patch Set 4 : Removed unneeded changed to MediaPlayerAndroid, style guide compliance #
Total comments: 36
Patch Set 5 : Comments, better Listener callbacks, removed unused includes #
Total comments: 13
Patch Set 6 : Ensure EOS is released last; cleanup #Patch Set 7 : Moved state machine into MediaCodecPlayer and eliminated the State pattern. #Patch Set 8 : Removed unused media_codec_player_state.h and .cc #
Total comments: 93
Patch Set 9 : Accept configuration change from decoder, media metadata cache #
Total comments: 8
Patch Set 10 : Addressed some comments #
Total comments: 21
Patch Set 11 : AccessUnitQueue changed to list of chunks, threading cleanup for decoders #
Total comments: 2
Patch Set 12 : Improved state machine diagrams #
Total comments: 15
Patch Set 13 : Added AccessUnitQueue unittests, cleanup per Min comments. #Patch Set 14 : Added initial unit tests for MediaCodecPlayer #Patch Set 15 : Restored media_source_player_unittest.cc #
Total comments: 52
Patch Set 16 : Addressed watk@ comments #Patch Set 17 : AccessUnitQueue::GetInfo() returns result by value #
Total comments: 4
Messages
Total messages: 55 (4 generated)
timav@google.com changed reviewers: + qinmin@chromium.org, timav@google.com
Min, I pulled it my existing implementation here and change the names: Player -> MediaCodecPlayer (already established) PlayerState -> MediaCodecPlayerState Decoder -> MediaCodecDecoder AudioDecoder -> MediaCodecAudioDecoder VideoDecoder -> MediaCodecAudioDecoder AUQueue -> not changed After porting I removed the stuff related to seek to get this CL more manageable, but frankly it did not take out much. Please take a look. https://codereview.chromium.org/1128383003/diff/1/content/browser/media/andro... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/1128383003/diff/1/content/browser/media/andro... content/browser/media/android/browser_media_player_manager.cc:82: MediaPlayerManager* manager, It seems we always pass |this| for |manager|, can we remove this parameter? https://codereview.chromium.org/1128383003/diff/1/media/base/android/media_co... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/1/media/base/android/media_co... media/base/android/media_codec_player.cc:55: base::WeakPtr<MediaPlayerManager> manager, Since we post notifications from Media to UI thread we seem to need a weak pointer to Manager (or use weak_ptr_for_ui_thread() that is defined in the base class and call a base class method). https://codereview.chromium.org/1128383003/diff/1/media/base/android/media_pl... File media/base/android/media_player_android.h (right): https://codereview.chromium.org/1128383003/diff/1/media/base/android/media_pl... media/base/android/media_player_android.h:118: base::WeakPtr<MediaPlayerAndroid> weak_ptr_for_ui_thread() { I'm not sure about this. I think we need this method if we want to post AttachListener() and DetachListener() to the UI thread as I do right now, but maybe we can call them from the Media thread.
Min, this maybe better, I tried to simplify the stop.
Some initial comments https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... File media/base/android/au_queue.cc (right): https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.cc:10: //#include "base/tvlog.h" remove unused includes https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.cc:15: : has_eos_(false) the ctor can squeeze a single line https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.cc:19: {} move {} to the previous line https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.cc:22: base::AutoLock lock(lock_); the lock is pretty expensive here as data will be copied. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.cc:64: for (it = access_units_.begin(); it!= access_units_.end(); ++it) { doesn't this skip all the access units that haven't been played? We should not skip access units that has not yet played. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... File media/base/android/au_queue.h (right): https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.h:16: class AUQueue { I prefer this to be called AccessUnitQueue, rather than AUQueue. Which is not straightforward. Same to the file name. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.h:36: // removes prior frames an returns true. If it does not nit: s/an/and/ https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.h:46: DISALLOW_COPY_AND_ASSIGN(AUQueue); this should stay at the end of the class declaration. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.h:53: nit: extra line https://codereview.chromium.org/1128383003/diff/20001/media/base/android/medi... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1128383003/diff/20001/media/base/android/medi... media/base/android/media_codec_decoder.h:23: extra line https://codereview.chromium.org/1128383003/diff/20001/media/base/android/medi... media/base/android/media_codec_decoder.h:42: base::Callback<void(base::TimeDelta, base::TimeDelta)> SetTimeCallback; nit: spacing https://codereview.chromium.org/1128383003/diff/20001/media/base/android/medi... media/base/android/media_codec_decoder.h:55: virtual const char * class_name() const { return "Decoder"; } nit: extra space after char https://codereview.chromium.org/1128383003/diff/20001/media/base/android/medi... media/base/android/media_codec_decoder.h:89: extra lines https://codereview.chromium.org/1128383003/diff/20001/media/base/android/medi... media/base/android/media_codec_decoder.h:142: // Flag is set when the EOS is received in MediaCodec output. why all the members are protected here? if we don't use the variable in child class, shouldn't them be private?
https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... File media/base/android/au_queue.cc (right): https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.cc:10: //#include "base/tvlog.h" On 2015/05/12 23:58:22, qinmin wrote: > remove unused includes I need them for testing. I will remove them last thing before the commit. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.cc:22: base::AutoLock lock(lock_); On 2015/05/12 23:58:22, qinmin wrote: > the lock is pretty expensive here as data will be copied. I used std::deque for simplicity as a temporary solution. I think the ring buffer that we have in MediaSourcePlayer is better and I should do it here too. That said, if we cannot lock this queue, my only idea would be to go for some lock-free schemes which I haven't used myself. Your other comments are acknowledged I'll submit another version, but please do stop me if you see a major flaw and we need to change the player drastically. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.cc:64: for (it = access_units_.begin(); it!= access_units_.end(); ++it) { On 2015/05/12 23:58:22, qinmin wrote: > doesn't this skip all the access units that haven't been played? We should not > skip access units that has not yet played. I think I misunderstood what SetCurrentFrameToPreviouslyCachedKeyFrame() does. I though it jumps into the future, but it seems to go into the past. That means I cannot remove the access units that went to the decoder right away.
Updated the CL description. Added DCHECKs for what thread the method is executing. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... File media/base/android/au_queue.cc (right): https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.cc:15: : has_eos_(false) On 2015/05/12 23:58:22, qinmin wrote: > the ctor can squeeze a single line Done. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.cc:19: {} On 2015/05/12 23:58:22, qinmin wrote: > move {} to the previous line Done. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.cc:22: base::AutoLock lock(lock_); On 2015/05/12 23:58:22, qinmin wrote: > the lock is pretty expensive here as data will be copied. Added TODO item https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... File media/base/android/au_queue.h (right): https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.h:16: class AUQueue { On 2015/05/12 23:58:23, qinmin wrote: > I prefer this to be called AccessUnitQueue, rather than AUQueue. Which is not > straightforward. Same to the file name. Renamed. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.h:36: // removes prior frames an returns true. If it does not On 2015/05/12 23:58:23, qinmin wrote: > nit: s/an/and/ Done, although this method does the wrong thing, as you pointed out. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.h:46: DISALLOW_COPY_AND_ASSIGN(AUQueue); On 2015/05/12 23:58:23, qinmin wrote: > this should stay at the end of the class declaration. Done. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.h:53: On 2015/05/12 23:58:23, qinmin wrote: > nit: extra line Removed. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/medi... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1128383003/diff/20001/media/base/android/medi... media/base/android/media_codec_decoder.h:23: On 2015/05/12 23:58:23, qinmin wrote: > extra line Removed. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/medi... media/base/android/media_codec_decoder.h:42: base::Callback<void(base::TimeDelta, base::TimeDelta)> SetTimeCallback; On 2015/05/12 23:58:23, qinmin wrote: > nit: spacing Done. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/medi... media/base/android/media_codec_decoder.h:55: virtual const char * class_name() const { return "Decoder"; } On 2015/05/12 23:58:23, qinmin wrote: > nit: extra space after char Done. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/medi... media/base/android/media_codec_decoder.h:89: On 2015/05/12 23:58:23, qinmin wrote: > extra lines Removed https://codereview.chromium.org/1128383003/diff/20001/media/base/android/medi... media/base/android/media_codec_decoder.h:142: // Flag is set when the EOS is received in MediaCodec output. On 2015/05/12 23:58:23, qinmin wrote: > why all the members are protected here? if we don't use the variable in child > class, shouldn't them be private? Sorry I missed that. Separated protected and private methods and data.
some initial comments https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_audio_decoder.cc:44: {} { placed incorrectly https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_audio_decoder.cc:47: { ditto https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_decoder.cc:60: { { belongs to the previous line https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:354: &MediaPlayerAndroid::DetachListener, weak_ptr_for_ui_thread())); binding the ui thread weakptr on media thread? https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:391: DCHECK(new_state != nullptr); nit: simply DCHECK(new_state); https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:542: &MediaPlayerAndroid::AttachListener, weak_ptr_for_ui_thread(), ditto https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_codec_player_state.cc (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player_state.cc:8: nit:extra line https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player_state.cc:30: ditto https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player_state.cc:38: ditto https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player_state.cc:74: { { has to go to the previous line https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player_state.cc:99: ditto https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player_state.cc:123: ditto https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:104: if (! au_queue_.SkipToKeyFrame()) { extra space after ! https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_player_android.h (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_player_android.h:119: return weak_ptr_for_ui_thread_; cannot we just return weak_factory_.GetWeakPtr() ?
https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_audio_decoder.cc:44: {} On 2015/05/13 23:17:31, qinmin wrote: > { placed incorrectly Fixed https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_audio_decoder.cc:47: { On 2015/05/13 23:17:31, qinmin wrote: > ditto Fixed https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_decoder.cc:60: { On 2015/05/13 23:17:31, qinmin wrote: > { belongs to the previous line Done. https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:354: &MediaPlayerAndroid::DetachListener, weak_ptr_for_ui_thread())); On 2015/05/13 23:17:32, qinmin wrote: > binding the ui thread weakptr on media thread? Yes. What I wanted to do here is to have the weak pointer that will be used (dereferenced) and invalidated on the UI thread. As far as I understood, weak pointers can be passed around between threads as long as they are used and invalidated on the dedicated thread. https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:391: DCHECK(new_state != nullptr); On 2015/05/13 23:17:32, qinmin wrote: > nit: simply DCHECK(new_state); Done. https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:542: &MediaPlayerAndroid::AttachListener, weak_ptr_for_ui_thread(), On 2015/05/13 23:17:31, qinmin wrote: > ditto Same consideration about weak_ptr here. https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_codec_player_state.cc (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player_state.cc:8: On 2015/05/13 23:17:32, qinmin wrote: > nit:extra line Removed. https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player_state.cc:30: On 2015/05/13 23:17:32, qinmin wrote: > ditto I put 2 lines between different states, e.g. the methods of StatePause are separated from StateWaitingForSurface by 2 lines. I can remove if it really stands out as inappropriate... https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player_state.cc:74: { On 2015/05/13 23:17:32, qinmin wrote: > { has to go to the previous line Done. https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:104: if (! au_queue_.SkipToKeyFrame()) { On 2015/05/13 23:17:32, qinmin wrote: > extra space after ! Oops! Removed. https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_player_android.h (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_player_android.h:119: return weak_ptr_for_ui_thread_; On 2015/05/13 23:17:32, qinmin wrote: > cannot we just return weak_factory_.GetWeakPtr() ? Yes, I had wrong ideas about what thread that pointer should be and is created. Done.
qinmin@chromium.org changed reviewers: + wolenetz@chromium.org
+wolenetz https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:354: &MediaPlayerAndroid::DetachListener, weak_ptr_for_ui_thread())); On 2015/05/14 00:14:59, Tima Vaisburd wrote: > On 2015/05/13 23:17:32, qinmin wrote: > > binding the ui thread weakptr on media thread? > > Yes. What I wanted to do here is to have the weak pointer that will be used > (dereferenced) and invalidated on the UI thread. As far as I understood, weak > pointers can be passed around between threads as long as they are used and > invalidated on the dedicated thread. Normally it is better to bind the weak ptr on the UI thread, and pass the callback to the media thread. That makes it more clear about thread ownership of the weak_ptr. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_decoder.cc:13: //#include "base/tvlog.h" remove unused include https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_decoder.h:31: class MediaCodecDecoder { Class descriptions, and for all the member function/variables below https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_decoder.h:33: nit: remove empty line https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_decoder.h:40: typedef base::Callback<void(base::TimeDelta, base::TimeDelta)> Descriptions https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player.h:13: no new lines among includes https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player.h:43: void (base::TimeDelta duration, int width, int height, bool success)> indent by 4 https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player.h:47: void (base::TimeDelta current_timestamp, base::TimeTicks current_ticks)> ditto https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_player_state.h (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player_state.h:80: virtual void EventRemoveVideoSurface(MediaCodecPlayer* player) {} I would rename this to OnRemoveVideoSurface() or OnRemoveVideoSurfaceEvent(). same below https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_player_android.h (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_player_android.h:118: base::WeakPtr<MediaPlayerAndroid> WeakPtrForUIThread() { this shouldn't be inlined, not a simple getter/setter
https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... media/base/android/access_unit_queue.cc:6: remove empty line https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... media/base/android/access_unit_queue.cc:10: //#include "base/tvlog.h" remove unused include https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... media/base/android/access_unit_queue.h:16: class AccessUnitQueue { class description https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_audio_decoder.cc:9: remote empty line https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_audio_decoder.cc:14: //#include "base/tvlog.h" remove this https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_decoder.cc:378: // Returns false if there was MediaCodec error. this should be moved to .h file https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player.cc:354: &MediaPlayerAndroid::DetachListener, WeakPtrForUIThread())); WeakPtrForUIThread() is no longer thread safe if you use weak_factory in MediaPlayerAndroid. You need to pass a callback from the UI thread. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player.cc:523: goto error; goto statement is something we should avoid. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_player_state.cc (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player_state.cc:42: player->SetState(StatePaused::Instance()); I have bad feelings about these call sequences, and all the functions here has the same issue. Player calls EventPause() when the state == StatePrefetching. And inside this call, we call SetState(StatePaused), and then call player->xxxx(). So when player->xxxx() executes, the state is already changed. From the player's perspective, it is very wierd that calling state->EventPause() will incur an reentrance call.
In addition to inline comments, I changed the semantic of AccessQueue::GetInfo() slighly: only one Info pointer is defined, either for the access unit, or for the configs. https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:354: &MediaPlayerAndroid::DetachListener, weak_ptr_for_ui_thread())); On 2015/05/14 17:54:07, qinmin wrote: > On 2015/05/14 00:14:59, Tima Vaisburd wrote: > > On 2015/05/13 23:17:32, qinmin wrote: > > > binding the ui thread weakptr on media thread? > > > > Yes. What I wanted to do here is to have the weak pointer that will be used > > (dereferenced) and invalidated on the UI thread. As far as I understood, weak > > pointers can be passed around between threads as long as they are used and > > invalidated on the dedicated thread. > > Normally it is better to bind the weak ptr on the UI thread, and pass the > callback to the media thread. That makes it more clear about thread ownership of > the weak_ptr. Done. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... media/base/android/access_unit_queue.cc:6: On 2015/05/14 19:52:57, qinmin wrote: > remove empty line Done. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... media/base/android/access_unit_queue.cc:10: //#include "base/tvlog.h" On 2015/05/14 19:52:57, qinmin wrote: > remove unused include Done. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... media/base/android/access_unit_queue.h:16: class AccessUnitQueue { On 2015/05/14 19:52:57, qinmin wrote: > class description Done. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_audio_decoder.cc:9: On 2015/05/14 19:52:57, qinmin wrote: > remote empty line Done. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_audio_decoder.cc:14: //#include "base/tvlog.h" On 2015/05/14 19:52:57, qinmin wrote: > remove this Done. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_decoder.cc:13: //#include "base/tvlog.h" On 2015/05/14 17:54:07, qinmin wrote: > remove unused include Removed everywhere. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_decoder.h:31: class MediaCodecDecoder { On 2015/05/14 17:54:07, qinmin wrote: > Class descriptions, and for all the member function/variables below I need to thank you, while doing this I did some cleanup. I added a bunch of text. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_decoder.h:33: On 2015/05/14 17:54:07, qinmin wrote: > nit: remove empty line Done. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_decoder.h:40: typedef base::Callback<void(base::TimeDelta, base::TimeDelta)> On 2015/05/14 17:54:07, qinmin wrote: > Descriptions Done. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player.cc:354: &MediaPlayerAndroid::DetachListener, WeakPtrForUIThread())); On 2015/05/14 19:52:58, qinmin wrote: > WeakPtrForUIThread() is no longer thread safe if you use weak_factory in > MediaPlayerAndroid. You need to pass a callback from the UI thread. I did use the callback as you suggested. I'd like to understand though why this usage is invalid, not simply unclear or uncommon. As far as I can get I create another WeakPtr object (by calling WeakPtrForUIThread()) on the Media thread. But why this is unsafe? I looked through weak_ptr.h and .cc, they seems to talk only about dereferencng and invalidation, not creation. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player.cc:523: goto error; On 2015/05/14 19:52:58, qinmin wrote: > goto statement is something we should avoid. Personally I do not find the goto statement awful if it makes the code concise and increases clarity. I rewrote this method without goto, please let me know if you like it better. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player.h:13: On 2015/05/14 17:54:07, qinmin wrote: > no new lines among includes Done. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player.h:43: void (base::TimeDelta duration, int width, int height, bool success)> On 2015/05/14 17:54:07, qinmin wrote: > indent by 4 Done. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player.h:47: void (base::TimeDelta current_timestamp, base::TimeTicks current_ticks)> On 2015/05/14 17:54:07, qinmin wrote: > ditto Done. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_player_state.cc (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player_state.cc:42: player->SetState(StatePaused::Instance()); On 2015/05/14 19:52:58, qinmin wrote: > I have bad feelings about these call sequences, and all the functions here has > the same issue. > > Player calls EventPause() when the state == StatePrefetching. And inside this > call, we call SetState(StatePaused), and then call player->xxxx(). > So when player->xxxx() executes, the state is already changed. > From the player's perspective, it is very wierd that calling state->EventPause() > will incur an reentrance call. Yes, it does look somewhat weird. The code like void StatePlaying::EventPause(MediaCodecPlayer* player) { player->RequestToStopDecoders(); player->SetState(StateStopping::Instance()); } would look more natural. I did it for a theoretical possibility that an operation sends another event, e.g. // in StatePlaying player->RequestToStopDecoders(); // calls state_->EventStart(); State machine thinks we are playing // so there is nothing to do player->SetState(StateStopping::Instance()); As long as we post all events this problem should not arise. I can change the order everywhere but it feels to me a little bit safer now. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_player_state.h (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player_state.h:80: virtual void EventRemoveVideoSurface(MediaCodecPlayer* player) {} On 2015/05/14 17:54:07, qinmin wrote: > I would rename this to OnRemoveVideoSurface() or OnRemoveVideoSurfaceEvent(). > same below Renamed to EventVideoSurfaceRemoved() to keep the common naming scheme. https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_player_android.h (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_player_android.h:118: base::WeakPtr<MediaPlayerAndroid> WeakPtrForUIThread() { On 2015/05/14 17:54:08, qinmin wrote: > this shouldn't be inlined, not a simple getter/setter Done.
https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... media/base/android/media_codec_player.cc:354: &MediaPlayerAndroid::DetachListener, WeakPtrForUIThread())); On 2015/05/15 00:12:39, Tima Vaisburd wrote: > On 2015/05/14 19:52:58, qinmin wrote: > > WeakPtrForUIThread() is no longer thread safe if you use weak_factory in > > MediaPlayerAndroid. You need to pass a callback from the UI thread. > > I did use the callback as you suggested. I'd like to understand though why this > usage is invalid, not simply unclear or uncommon. > > As far as I can get I create another WeakPtr object (by calling > WeakPtrForUIThread()) on the Media thread. But why this is unsafe? I looked > through weak_ptr.h and .cc, they seems to talk only about dereferencng and > invalidation, not creation. In MediaPlayerAndroid, WeakPtrForUIThread() is defined as weak_factory_.GetWeakPtr() and it is called on media thread here. So the weak pointer you get here is bounded to the media thread, rather than ui thread. And it is not safe to post a task on to the ui message loop using a weak ptr bounded to the media thread.
On 2015/05/15 01:14:14, qinmin wrote: > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > File media/base/android/media_codec_player.cc (right): > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > media/base/android/media_codec_player.cc:354: > &MediaPlayerAndroid::DetachListener, WeakPtrForUIThread())); > On 2015/05/15 00:12:39, Tima Vaisburd wrote: > > On 2015/05/14 19:52:58, qinmin wrote: > > > WeakPtrForUIThread() is no longer thread safe if you use weak_factory in > > > MediaPlayerAndroid. You need to pass a callback from the UI thread. > > > > I did use the callback as you suggested. I'd like to understand though why > this > > usage is invalid, not simply unclear or uncommon. > > > > As far as I can get I create another WeakPtr object (by calling > > WeakPtrForUIThread()) on the Media thread. But why this is unsafe? I looked > > through weak_ptr.h and .cc, they seems to talk only about dereferencng and > > invalidation, not creation. > > In MediaPlayerAndroid, WeakPtrForUIThread() is defined as > weak_factory_.GetWeakPtr() and it is called on media thread here. So the weak > pointer you get here is bounded to the media thread, rather than ui thread. > And it is not safe to post a task on to the ui message loop using a weak ptr > bounded to the media thread. They say they bind on first dereference, not creation: "the first time a WeakPtr issued by a WeakPtrFactory is dereferenced, the factory and its WeakPtrs become bound to the calling thread". And the checker is reset in constructor, it seems: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
On 2015/05/15 17:28:57, Tima Vaisburd wrote: > On 2015/05/15 01:14:14, qinmin wrote: > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > File media/base/android/media_codec_player.cc (right): > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > media/base/android/media_codec_player.cc:354: > > &MediaPlayerAndroid::DetachListener, WeakPtrForUIThread())); > > On 2015/05/15 00:12:39, Tima Vaisburd wrote: > > > On 2015/05/14 19:52:58, qinmin wrote: > > > > WeakPtrForUIThread() is no longer thread safe if you use weak_factory in > > > > MediaPlayerAndroid. You need to pass a callback from the UI thread. > > > > > > I did use the callback as you suggested. I'd like to understand though why > > this > > > usage is invalid, not simply unclear or uncommon. > > > > > > As far as I can get I create another WeakPtr object (by calling > > > WeakPtrForUIThread()) on the Media thread. But why this is unsafe? I looked > > > through weak_ptr.h and .cc, they seems to talk only about dereferencng and > > > invalidation, not creation. > > > > In MediaPlayerAndroid, WeakPtrForUIThread() is defined as > > weak_factory_.GetWeakPtr() and it is called on media thread here. So the weak > > pointer you get here is bounded to the media thread, rather than ui thread. > > And it is not safe to post a task on to the ui message loop using a weak ptr > > bounded to the media thread. > > They say they bind on first dereference, not creation: > "the first time a WeakPtr issued by a WeakPtrFactory is dereferenced, > the factory and its WeakPtrs become bound to the calling thread". > And the checker is reset in constructor, it seems: > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... But you are calling it multiple times. "To ensure correct use, the first time a WeakPtr issued by a WeakPtrFactory is dereferenced, the factory and its WeakPtrs become bound to the calling thread or current SequencedWorkerPool token, and cannot be dereferenced or invalidated on any other task runner" So after the weak ptr become bound to the ui thread, the WeakPtrFactory can no longer be dereferenced on the media thread. And if you call WeakPtrForUIThread(), that will dereference the factory.
On 2015/05/14 19:52:58, qinmin wrote: > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > File media/base/android/access_unit_queue.cc (right): > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > media/base/android/access_unit_queue.cc:6: > remove empty line > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > media/base/android/access_unit_queue.cc:10: //#include "base/tvlog.h" > remove unused include > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > File media/base/android/access_unit_queue.h (right): > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > media/base/android/access_unit_queue.h:16: class AccessUnitQueue { > class description > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > File media/base/android/media_codec_audio_decoder.cc (right): > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > media/base/android/media_codec_audio_decoder.cc:9: > remote empty line > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > media/base/android/media_codec_audio_decoder.cc:14: //#include "base/tvlog.h" > remove this > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > File media/base/android/media_codec_decoder.cc (right): > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > media/base/android/media_codec_decoder.cc:378: // Returns false if there was > MediaCodec error. > this should be moved to .h file > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > File media/base/android/media_codec_player.cc (right): > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > media/base/android/media_codec_player.cc:354: > &MediaPlayerAndroid::DetachListener, WeakPtrForUIThread())); > WeakPtrForUIThread() is no longer thread safe if you use weak_factory in > MediaPlayerAndroid. You need to pass a callback from the UI thread. > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > media/base/android/media_codec_player.cc:523: goto error; > goto statement is something we should avoid. > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > File media/base/android/media_codec_player_state.cc (right): > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > media/base/android/media_codec_player_state.cc:42: > player->SetState(StatePaused::Instance()); > I have bad feelings about these call sequences, and all the functions here has > the same issue. > > Player calls EventPause() when the state == StatePrefetching. And inside this > call, we call SetState(StatePaused), and then call player->xxxx(). > So when player->xxxx() executes, the state is already changed. > From the player's perspective, it is very wierd that calling state->EventPause() > will incur an reentrance call. I eliminated the State pattern and its reentrance calls by moving the logic directly into MediaCodecPlayer and using switches instead. Please let me know if you think this is better.
On 2015/05/18 17:43:37, Tima Vaisburd wrote: > On 2015/05/14 19:52:58, qinmin wrote: > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > > File media/base/android/access_unit_queue.cc (right): > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > > media/base/android/access_unit_queue.cc:6: > > remove empty line > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > > media/base/android/access_unit_queue.cc:10: //#include "base/tvlog.h" > > remove unused include > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > > File media/base/android/access_unit_queue.h (right): > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > > media/base/android/access_unit_queue.h:16: class AccessUnitQueue { > > class description > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > File media/base/android/media_codec_audio_decoder.cc (right): > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > media/base/android/media_codec_audio_decoder.cc:9: > > remote empty line > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > media/base/android/media_codec_audio_decoder.cc:14: //#include "base/tvlog.h" > > remove this > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > File media/base/android/media_codec_decoder.cc (right): > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > media/base/android/media_codec_decoder.cc:378: // Returns false if there was > > MediaCodec error. > > this should be moved to .h file > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > File media/base/android/media_codec_player.cc (right): > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > media/base/android/media_codec_player.cc:354: > > &MediaPlayerAndroid::DetachListener, WeakPtrForUIThread())); > > WeakPtrForUIThread() is no longer thread safe if you use weak_factory in > > MediaPlayerAndroid. You need to pass a callback from the UI thread. > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > media/base/android/media_codec_player.cc:523: goto error; > > goto statement is something we should avoid. > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > File media/base/android/media_codec_player_state.cc (right): > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > media/base/android/media_codec_player_state.cc:42: > > player->SetState(StatePaused::Instance()); > > I have bad feelings about these call sequences, and all the functions here has > > the same issue. > > > > Player calls EventPause() when the state == StatePrefetching. And inside this > > call, we call SetState(StatePaused), and then call player->xxxx(). > > So when player->xxxx() executes, the state is already changed. > > From the player's perspective, it is very wierd that calling > state->EventPause() > > will incur an reentrance call. > > I eliminated the State pattern and its reentrance calls by moving the logic > directly into MediaCodecPlayer and using switches instead. > Please let me know if you think this is better. Should media_codec_player_state.h(cc) be removed from the code review?
On 2015/05/19 00:08:26, qinmin wrote: > On 2015/05/18 17:43:37, Tima Vaisburd wrote: > > On 2015/05/14 19:52:58, qinmin wrote: > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > > > File media/base/android/access_unit_queue.cc (right): > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > > > media/base/android/access_unit_queue.cc:6: > > > remove empty line > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > > > media/base/android/access_unit_queue.cc:10: //#include "base/tvlog.h" > > > remove unused include > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > > > File media/base/android/access_unit_queue.h (right): > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/acce... > > > media/base/android/access_unit_queue.h:16: class AccessUnitQueue { > > > class description > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > > File media/base/android/media_codec_audio_decoder.cc (right): > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > > media/base/android/media_codec_audio_decoder.cc:9: > > > remote empty line > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > > media/base/android/media_codec_audio_decoder.cc:14: //#include > "base/tvlog.h" > > > remove this > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > > File media/base/android/media_codec_decoder.cc (right): > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > > media/base/android/media_codec_decoder.cc:378: // Returns false if there was > > > MediaCodec error. > > > this should be moved to .h file > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > > File media/base/android/media_codec_player.cc (right): > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > > media/base/android/media_codec_player.cc:354: > > > &MediaPlayerAndroid::DetachListener, WeakPtrForUIThread())); > > > WeakPtrForUIThread() is no longer thread safe if you use weak_factory in > > > MediaPlayerAndroid. You need to pass a callback from the UI thread. > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > > media/base/android/media_codec_player.cc:523: goto error; > > > goto statement is something we should avoid. > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > > File media/base/android/media_codec_player_state.cc (right): > > > > > > > > > https://codereview.chromium.org/1128383003/diff/60001/media/base/android/medi... > > > media/base/android/media_codec_player_state.cc:42: > > > player->SetState(StatePaused::Instance()); > > > I have bad feelings about these call sequences, and all the functions here > has > > > the same issue. > > > > > > Player calls EventPause() when the state == StatePrefetching. And inside > this > > > call, we call SetState(StatePaused), and then call player->xxxx(). > > > So when player->xxxx() executes, the state is already changed. > > > From the player's perspective, it is very wierd that calling > > state->EventPause() > > > will incur an reentrance call. > > > > I eliminated the State pattern and its reentrance calls by moving the logic > > directly into MediaCodecPlayer and using switches instead. > > Please let me know if you think this is better. > > Should media_codec_player_state.h(cc) be removed from the code review? My apology, forgot to git rm. Done now.
https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_audio_decoder.cc:144: new AudioTimestampHelper(configs_.audio_sampling_rate)); output sampling rate may not equal to config sampling rate, check audio_decoder_job.cc https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_decoder.cc:111: // || IsCodecReconfigureNeeded(configs_, configs_); remove this line if it is not used, and it seems that is_reconfigure_needed is not needed https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_decoder.cc:349: bool success= EnqueueInputBuffer(); space after success. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_decoder.cc:382: //DVLOG(2) << class_name() << "::" << __FUNCTION__; Remove unused lines https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_decoder.cc:478: //DVLOG(2) << class_name() << "::" << __FUNCTION__; ditto https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_decoder.cc:504: //DVLOG(2) << class_name() ditto https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:47: nit: remove the extra line. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:147: interpolator_.SetUpperBound(base::TimeDelta()); Can you call this without calling startInterpolating? There is a DCHECK(upper_bound != kNoTimestamp()) inside TimeDeltaInterpolator::SetUpperBound() https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:181: if (video_decoder_->HasPendingSurface() && nit: extra space after "if" https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:263: // UI thread, Media thread It is bad that these calls can span multiple threads. we should cache the values on UI thread and post tasks from media thread to update the cached values, instead of using clocks. OnMediaMetadataChanged() is a good place for the UI thread to cache values. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:268: // UI thread, Media thread ditto https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:346: SetState(STATE_PLAYING); No idea why the player has to start playing here. OnDemuxerConfigsAvailable() can be called when player is paused, no? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:395: nit: extra line https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:424: // No configuration at all, let's wait for it. This shouldn't happen. I would just put NOT_REACHED() here https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:564: bool MediaCodecPlayer::HasPendingStart() { The function name just don't match what it does. How about hasPendingStartAndReset() or split this into 2 functions. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:608: request_resources_cb_)); request_resources_cb_ is binded to the UI thread, and now it is passed to the video_decoder_, which has no idea about the UI message loop. So it will be called on the wrong message loop. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:734: if (count == 0) { No barrier closure is used here, why not simply remove count and use if(!do_audio && !do_video) https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:71: return configs_.video_size.width(); output video width is different from config width, check media_decoder_job.cc https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_player_android.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_player_android.cc:103: return weak_factory_.GetWeakPtr(); nit: indent by 2
First few comments. I'll continue CR of the player and decoder files next. https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... File media/base/android/au_queue.cc (right): https://codereview.chromium.org/1128383003/diff/20001/media/base/android/au_q... media/base/android/au_queue.cc:15: : has_eos_(false) On 2015/05/13 22:51:46, Tima Vaisburd wrote: > On 2015/05/12 23:58:22, qinmin wrote: > > the ctor can squeeze a single line > > Done. In general, "git cl format" is your friend :) https://codereview.chromium.org/1128383003/diff/140001/content/browser/media/... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/1128383003/diff/140001/content/browser/media/... content/browser/media/android/browser_media_player_manager.cc:93: manager, hmm. Is this ever not |this|? Do we even need the |manager| parameter to BMPM::CreateMediaPlayer()? If so, is it clear to the caller that, for at least MSE, the argument is ignored? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/BUI... File media/base/android/BUILD.gn (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/BUI... media/base/android/BUILD.gn:34: "media_codec_player_state.cc", media_codec_player_state.{cc,h} are no longer part of this CL. remove from gn config? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:19: // whet to do with this. nit: s/whet/what/ https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:21: // Media thread nit: here and elsewhere, might be good to have DCHECKs in place to enforce the method is called on the expected thread. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:23: nit: should probably do some DCHECK sanity here like: DCHECK(!has_eos_); and maybe something like DCHECK(unit.data.access_units.length > 0). https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:34: for (const DemuxerConfigs& conf : data.demuxer_configs) Hmm. Seems simple enough to DCHECK(data.demuxer_configs.length <= 1), or even better, have a local bool that is set true in the loop in l.24 if a unit is kConfigChanged, and then adjust the DCHECK here based on that bool. In general, protecting borders, especially ones on each side of IPC, SGTM. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:46: has_eos_ = false; // assume there os only one AU with EOS hmm. Once EOS has been consumed from the queue, does the queue really no longer care whether or not it previously reached EOS? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:76: void AccessUnitQueue::GetInfo(Info* info) const { nit: do we need this Peek-like method, or can PopFront return the necessary information? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:88: info->configs = &demuxer_configs_.front(); Sanity check that we have a config to return, please. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:23: // Information about the queue state and it's front AccessUnit nit: s/it's/its/ https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:32: // Current size of the queue. Does presence of new configs change this size? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:43: virtual ~AccessUnitQueue(); hmm. AccessUnitQueue doesn't derive, nor is it derived from. Is virtual necessary here? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:55: // removes prior frames and returns true. If it does not nit: squeeze 3rd line of comment onto end of 2nd? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:57: bool SkipToKeyFrame(); What if there is a config prior to the keyframe? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:68: // these units have no data and no PTS, and for each there is nit ditto: squeeze comment paragraphs into longer lines. Here and elsewhere in this CL, please. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:85: std::ostream& operator<<(std::ostream& os, const media::AccessUnit& au); Hmm. Put this into demuxer_stream_player_params.h ?
wolenetz@chromium.org changed reviewers: + watk@chromium.org
Some further comments. I haven't CR'ed the decoder files. Please add some unit tests. Also, I'm adding (read: delegating my CR to) watk@ for this CR, since he's working on similar areas and a longer term prototype that's strongly related to this CL. watk@: Please take a look. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:12: nit: drop extra line https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:50: /* nit: please move the state machine diagram to the .h Also, it would be good to identify at least placeholders for seek, error, completion, destruction, release/request resources, and release/request audio focus (and any others that have slipped my mind). I suspect doing this sooner rather than later will help flush out design/implementation issues. See also media/base/pipeline.h for an example of how much of this is done in the desktop Chromium video stack. Also, please add tests. While the number of tests of MediaSourcePlayer is quite high, I suspect that reviewing them and implementing the necessary coverage in tests of MediaCodecPlayer will help identify simplifications earlier and prevent repeating some of the same mistakes :) https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:403: // DemuxerAndroid::RequestDemuxerData() to avoid the race condition nit: remove extra space after 'the' https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:405: // 1. DestroySelf is posted from UI to Media thread. What is DestroySelf? It's not in existence in this class. Maybe this is the cause of my confusion, but I'm also not sure how this comment clarifies what the bad versus good usage would be. Am I missing somehow implicitly that the bad way doesn't use a weak_ptr, but the good way does? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:416: // Media thread Here, and elsewhere, a DCHECK ( I am on the right thread ) is sufficient to fully replace a comment that says "// the right thread". https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:420: if (state_ != STATE_PREFETCHING) When might this happen with this new player/decoders? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:432: SetState(STATE_WAITING_FOR_SURFACE); I'm not sure how this happens given the current code in OnDemuxerConfigsAvailable. Could the surface be removed after that time? Where does that happen / get handled in this player version? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:449: RequestToStopDecoders(); 10,000' view: why does starvation stop decoders? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:56: // MediaPlayerAndroid implementation. Please comment in the CL description the known gaps that are currently not implemented, such as: CDM/DRM/EME support? Seek support? BrowserSeek support? (or does the AccessUnitQueue make this hack a thing of the past?) etc. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:151: base::Closure attach_listener_cb_; What are the analogous methods in MediaSourcePlayer for these?
Min, the time propagation from decoder is not moved to media thread (yet). I'd like to see the delay. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_audio_decoder.cc:144: new AudioTimestampHelper(configs_.audio_sampling_rate)); On 2015/05/19 19:26:24, qinmin wrote: > output sampling rate may not equal to config sampling rate, check > audio_decoder_job.cc Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_decoder.cc:111: // || IsCodecReconfigureNeeded(configs_, configs_); On 2015/05/19 19:26:25, qinmin wrote: > remove this line if it is not used, and it seems that is_reconfigure_needed is > not needed Removed for now, we will need it in the next version. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_decoder.cc:349: bool success= EnqueueInputBuffer(); On 2015/05/19 19:26:25, qinmin wrote: > space after success. removed |bool success| altogether https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_decoder.cc:382: //DVLOG(2) << class_name() << "::" << __FUNCTION__; On 2015/05/19 19:26:25, qinmin wrote: > Remove unused lines Uncommented log instead of removing https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_decoder.cc:478: //DVLOG(2) << class_name() << "::" << __FUNCTION__; On 2015/05/19 19:26:25, qinmin wrote: > ditto Uncommented https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_decoder.cc:504: //DVLOG(2) << class_name() On 2015/05/19 19:26:25, qinmin wrote: > ditto Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:47: On 2015/05/19 19:26:25, qinmin wrote: > nit: remove the extra line. Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:181: if (video_decoder_->HasPendingSurface() && On 2015/05/19 19:26:25, qinmin wrote: > nit: extra space after "if" Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:263: // UI thread, Media thread On 2015/05/19 19:26:25, qinmin wrote: > It is bad that these calls can span multiple threads. we should cache the values > on UI thread and post tasks from media thread to update the cached values, > instead of using clocks. > > OnMediaMetadataChanged() is a good place for the UI thread to cache values. I did the caching but I'm not sure I followed the last sentence. The cache is inside MediaCodecPlayer. Please take a look. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:268: // UI thread, Media thread On 2015/05/19 19:26:26, qinmin wrote: > ditto Same as above. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:346: SetState(STATE_PLAYING); On 2015/05/19 19:26:25, qinmin wrote: > No idea why the player has to start playing here. OnDemuxerConfigsAvailable() > can be called when player is paused, no? But below you confirmed that this state is impossible in the first place (no configuration after prefetching) so I removed it here. Later, though, I had to recreate it under the same name, but with a completely different meaning: we need to wait for initial configs if we do not have them as Start(). https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:395: On 2015/05/19 19:26:25, qinmin wrote: > nit: extra line Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:424: // No configuration at all, let's wait for it. On 2015/05/19 19:26:25, qinmin wrote: > This shouldn't happen. I would just put NOT_REACHED() here I raised an error https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:564: bool MediaCodecPlayer::HasPendingStart() { On 2015/05/19 19:26:25, qinmin wrote: > The function name just don't match what it does. > How about hasPendingStartAndReset() or split this into 2 functions. Right now called two methods since so far HasPendingStart() is called just once. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:608: request_resources_cb_)); On 2015/05/19 19:26:25, qinmin wrote: > request_resources_cb_ is binded to the UI thread, and now it is passed to the > video_decoder_, which has no idea about the UI message loop. So it will be > called on the wrong message loop. ui_task_runner_ is accessible from VideoDecoder via protected member and is used there. I also verified that this callback is called on UI thread by printing tid. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:734: if (count == 0) { On 2015/05/19 19:26:26, qinmin wrote: > No barrier closure is used here, why not simply remove count and use > if(!do_audio && !do_video) Yes, it was a silly copy-pasting. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:71: return configs_.video_size.width(); On 2015/05/19 19:26:26, qinmin wrote: > output video width is different from config width, check media_decoder_job.cc I've separated output video size from config, but set it only the very first time. Relying on the OUTPUT_FORMAT_CHANGED from decoder to notify the others. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_player_android.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_player_android.cc:103: return weak_factory_.GetWeakPtr(); On 2015/05/19 19:26:26, qinmin wrote: > nit: indent by 2 Done.
https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:346: SetState(STATE_PLAYING); On 2015/05/20 05:19:58, Tima Vaisburd wrote: > On 2015/05/19 19:26:25, qinmin wrote: > > No idea why the player has to start playing here. OnDemuxerConfigsAvailable() > > can be called when player is paused, no? > > But below you confirmed that this state is impossible in the first place (no > configuration after prefetching) so I removed it here. > Later, though, I had to recreate it under the same name, but with a completely > different meaning: we need to wait for initial configs if we do not have them as > Start(). Prefetching and OnDemuxerConfigsAvailable are 2 different things. OnDemuxerConfigsAvailable() is called when DemuxerReady IPC is received, that happens when chunk demuxer finishes initializing itself. Why the player should enter a playing state in that case?
https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:608: request_resources_cb_)); On 2015/05/20 05:19:58, Tima Vaisburd wrote: > On 2015/05/19 19:26:25, qinmin wrote: > > request_resources_cb_ is binded to the UI thread, and now it is passed to the > > video_decoder_, which has no idea about the UI message loop. So it will be > > called on the wrong message loop. > > ui_task_runner_ is accessible from VideoDecoder via protected member and is used > there. I also verified that this callback is called on UI thread by printing > tid. I don't feel it proper for Decoder to know ui_task_runner. They are created on media thread, and running on decoder thread, they don't need to know about the ui thread. I think MediaCodecPlayer should have its own requestVideoResources() call, and pass that function as a callback to the decoder job. And when that call runs, mediaCodecPlayer will post a task to UI thread to inform BrowserMediaPlayerManager.
https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:346: SetState(STATE_PLAYING); On 2015/05/20 15:39:25, qinmin wrote: > On 2015/05/20 05:19:58, Tima Vaisburd wrote: > > On 2015/05/19 19:26:25, qinmin wrote: > > > No idea why the player has to start playing here. > OnDemuxerConfigsAvailable() > > > can be called when player is paused, no? > > > > But below you confirmed that this state is impossible in the first place (no > > configuration after prefetching) so I removed it here. > > Later, though, I had to recreate it under the same name, but with a completely > > different meaning: we need to wait for initial configs if we do not have them > as > > Start(). > > Prefetching and OnDemuxerConfigsAvailable are 2 different things. > OnDemuxerConfigsAvailable() is called when DemuxerReady IPC is received, that > happens when chunk demuxer finishes initializing itself. Why the player should > enter a playing state in that case? Please take a look at PatchSet 9 where I changed this. There, we might wait _for_ OnDemuxerConfigsAvailable to continue, more precisely, to start prefetching.
https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:417: // 3. DestroySelf arrives, we delete the player and detach from DestroySelf? https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... media/base/android/media_codec_player.h:97: STATE_WAITING_FOR_CONFIG, nit: this state isn't represented in the state diagram. Please include it there. https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... media/base/android/media_codec_player.h:102: STATE_WAITING_FOR_SEEK, nit ditto https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... media/base/android/media_codec_player.h:192: MediaMetadata cache_; nit: this member's name lacks self-description. e.g. what is it a cache of?
Matthew, all your (and Min!) comments are thoroughly considered, I'm Android sheriff today and tomorrow, hope to be able to properly respond on Friday. s/DestroySelf/DeleteSoon/, but it requires more explicit wording, as you've said. On Wed, May 20, 2015 at 11:20 AM, <wolenetz@chromium.org> wrote: > > > https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... > File media/base/android/media_codec_player.cc (right): > > > https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... > media/base/android/media_codec_player.cc:417: // 3. DestroySelf arrives, > we delete the player and detach from > DestroySelf? > > > https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... > File media/base/android/media_codec_player.h (right): > > > https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... > media/base/android/media_codec_player.h:97: STATE_WAITING_FOR_CONFIG, > nit: this state isn't represented in the state diagram. Please include > it there. > > > https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... > media/base/android/media_codec_player.h:102: STATE_WAITING_FOR_SEEK, > nit ditto > > > https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... > media/base/android/media_codec_player.h:192: MediaMetadata cache_; > nit: this member's name lacks self-description. e.g. what is it a cache > of? > > https://codereview.chromium.org/1128383003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi, I decided to release the intermediate state today in case you would get a chance to take a look. Please note that not all your comments are addressed yet. https://codereview.chromium.org/1128383003/diff/140001/content/browser/media/... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/1128383003/diff/140001/content/browser/media/... content/browser/media/android/browser_media_player_manager.cc:93: manager, On 2015/05/19 21:58:36, wolenetz wrote: > hmm. Is this ever not |this|? Do we even need the |manager| parameter to > BMPM::CreateMediaPlayer()? If so, is it clear to the caller that, for at least > MSE, the argument is ignored? Yes, I removed |manager| parameter. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/BUI... File media/base/android/BUILD.gn (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/BUI... media/base/android/BUILD.gn:34: "media_codec_player_state.cc", On 2015/05/19 21:58:36, wolenetz wrote: > media_codec_player_state.{cc,h} are no longer part of this CL. remove from gn > config? Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:19: // whet to do with this. On 2015/05/19 21:58:37, wolenetz wrote: > nit: s/whet/what/ Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:21: // Media thread On 2015/05/19 21:58:37, wolenetz wrote: > nit: here and elsewhere, might be good to have DCHECKs in place to enforce the > method is called on the expected thread. I tried to do it everywhere in higher-level classes: MediaCodecPlayer, MediaCodecDecoder and derivatives. This queue is a part of MediaCodecDecoder and the methods are called from there. I made an attempt to have these DCHECKs here, too, but it did not look good. I think the checks in decoder classes are enough. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:23: On 2015/05/19 21:58:37, wolenetz wrote: > nit: should probably do some DCHECK sanity here like: > DCHECK(!has_eos_); > and maybe something like DCHECK(unit.data.access_units.length > 0). Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:34: for (const DemuxerConfigs& conf : data.demuxer_configs) On 2015/05/19 21:58:36, wolenetz wrote: > Hmm. Seems simple enough to DCHECK(data.demuxer_configs.length <= 1), or even > better, have a local bool that is set true in the loop in l.24 if a unit is > kConfigChanged, and then adjust the DCHECK here based on that bool. In general, > protecting borders, especially ones on each side of IPC, SGTM. Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:46: has_eos_ = false; // assume there os only one AU with EOS On 2015/05/19 21:58:37, wolenetz wrote: > hmm. Once EOS has been consumed from the queue, does the queue really no longer > care whether or not it previously reached EOS? The idea was to self-clean after the last access unit pops. I changed to if (access_units_.empty()) has_eos_ = false; https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:76: void AccessUnitQueue::GetInfo(Info* info) const { On 2015/05/19 21:58:37, wolenetz wrote: > nit: do we need this Peek-like method, or can PopFront return the necessary > information? I think we do. The Android MediaCodec might return "try again later" when asked for input buffer, so we need to keep AU around. We could pop it from the queue, but could not remove it and would need to put aside, and remove after a successful MediaCodec.queueInputBuffer() only. On the other hand, as far as I know, after the MediaCodec returned an input buffer, it is impossible to cancel: I must provide a valid encoded frame. This fact determines the order: first check the AU queue for a frame, then ask MediaDecoder for a buffer. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.cc:88: info->configs = &demuxer_configs_.front(); On 2015/05/19 21:58:36, wolenetz wrote: > Sanity check that we have a config to return, please. Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:32: // Current size of the queue. On 2015/05/19 21:58:37, wolenetz wrote: > Does presence of new configs change this size? Yes, |kConfigChanged| is a placeholder access unit. I changed description, hopefully it became clearer. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:43: virtual ~AccessUnitQueue(); On 2015/05/19 21:58:37, wolenetz wrote: > hmm. AccessUnitQueue doesn't derive, nor is it derived from. Is virtual > necessary here? Sorry, I can't see why it was virtual either. Removed. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:55: // removes prior frames and returns true. If it does not On 2015/05/19 21:58:37, wolenetz wrote: > nit: squeeze 3rd line of comment onto end of 2nd? Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:68: // these units have no data and no PTS, and for each there is On 2015/05/19 21:58:37, wolenetz wrote: > nit ditto: squeeze comment paragraphs into longer lines. > Here and elsewhere in this CL, please. Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:85: std::ostream& operator<<(std::ostream& os, const media::AccessUnit& au); On 2015/05/19 21:58:37, wolenetz wrote: > Hmm. Put this into demuxer_stream_player_params.h ? Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:12: On 2015/05/19 23:05:23, wolenetz wrote: > nit: drop extra line Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:403: // DemuxerAndroid::RequestDemuxerData() to avoid the race condition On 2015/05/19 23:05:23, wolenetz wrote: > nit: remove extra space after 'the' Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:405: // 1. DestroySelf is posted from UI to Media thread. On 2015/05/19 23:05:23, wolenetz wrote: > What is DestroySelf? It's not in existence in this class. Maybe this is the > cause of my confusion, but I'm also not sure how this comment clarifies what the > bad versus good usage would be. Am I missing somehow implicitly that the bad way > doesn't use a weak_ptr, but the good way does? Yes, exactly. Fixed and reformulated slightly. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:416: // Media thread On 2015/05/19 23:05:23, wolenetz wrote: > Here, and elsewhere, a DCHECK ( I am on the right thread ) is sufficient to > fully replace a comment that says "// the right thread". I found it easier to grasp a short comment written in plain English (and highlighted with a different color) than the long DCHECK statement where the piece that conveys information ('UI' or 'Media') is buried. I can see that it might feel redundant, though. Do you feel I'd rather remove the comments? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:420: if (state_ != STATE_PREFETCHING) On 2015/05/19 23:05:23, wolenetz wrote: > When might this happen with this new player/decoders? Release() may be called on Media thread before we are done prefetching, for instance. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:432: SetState(STATE_WAITING_FOR_SURFACE); On 2015/05/19 23:05:23, wolenetz wrote: > I'm not sure how this happens given the current code in > OnDemuxerConfigsAvailable. Could the surface be removed after that time? Where > does that happen / get handled in this player version? Yes, the surface can be removed at any time, but changing the surface is not implemented in this version at all. The intention here is to wait for the very first video surface. OnDemuxerConfigsAvailable comes from the renderer process, while the surface comes from Java in browser process. These events are not synchronized. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:449: RequestToStopDecoders(); On 2015/05/19 23:05:23, wolenetz wrote: > 10,000' view: why does starvation stop decoders? I think we need to stop the other channel... Upon restart we redo A/V sync, the same way we do it in the MediaSourcePlayer. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:151: base::Closure attach_listener_cb_; On 2015/05/19 23:05:23, wolenetz wrote: > What are the analogous methods in MediaSourcePlayer for these? AttachListener() and DetachListener(), I changed the comment to make it clearer.
https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:23: // Information about the queue state and it's front AccessUnit On 2015/05/19 21:58:37, wolenetz wrote: > nit: s/it's/its/ Done. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:608: request_resources_cb_)); On 2015/05/20 15:48:16, qinmin wrote: > On 2015/05/20 05:19:58, Tima Vaisburd wrote: > > On 2015/05/19 19:26:25, qinmin wrote: > > > request_resources_cb_ is binded to the UI thread, and now it is passed to > the > > > video_decoder_, which has no idea about the UI message loop. So it will be > > > called on the wrong message loop. > > > > ui_task_runner_ is accessible from VideoDecoder via protected member and is > used > > there. I also verified that this callback is called on UI thread by printing > > tid. > > I don't feel it proper for Decoder to know ui_task_runner. They are created on > media thread, and running on decoder thread, they don't need to know about the > ui thread. I think MediaCodecPlayer should have its own requestVideoResources() > call, and pass that function as a callback to the decoder job. And when that > call runs, mediaCodecPlayer will post a task to UI thread to inform > BrowserMediaPlayerManager. We have two callbacks now, both from the video decoder: requestVideoResources and videoResolutionChanged. I would think we want to avoid an extra thread hopping for efficiency, like we did with demuxer tasks. What do you think about moving the ui_task_runner from Decoder to the VideoDecoder subclass and post the task to it directly, like now? https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:56: // MediaPlayerAndroid implementation. On 2015/05/19 23:05:23, wolenetz wrote: > Please comment in the CL description the known gaps that are currently not > implemented, such as: > CDM/DRM/EME support? > Seek support? > BrowserSeek support? (or does the AccessUnitQueue make this hack a thing of the > past?) > etc. None of this is implemented in this CL. I deliberately removed some pieces in a hope to make it easier for review. I can add it back you and Min would rather like to see them here and not in a subsequent reviews. That is the reason for the states that are not used but mentioned. The browser seek, as far as I understand, will be needed. We might need to recreate the MediaCodec at a random moment, and the first frame after the recreation has to be the key frame. The olnly alternative I see is to keep the AUs since the last key frame in the AU queue. Wouldn't it be a potentially significant data duplication? https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:417: // 3. DestroySelf arrives, we delete the player and detach from On 2015/05/20 18:20:04, wolenetz wrote: > DestroySelf? Fixed. https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... media/base/android/media_codec_player.h:192: MediaMetadata cache_; On 2015/05/20 18:20:04, wolenetz wrote: > nit: this member's name lacks self-description. e.g. what is it a cache of? Done.
https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... File media/base/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_audio_decoder.cc:9: remove extra line https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... File media/base/android/media_codec_audio_decoder.h (right): https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_audio_decoder.h:27: const char * class_name() const override { return "AudioDecoder"; } s/char */char*/ https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.h:67: // Stop decoder thread, release the MediaCodecBridge and other resources. s/Stop/Stops/, s/release/releases/ https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.h:70: // Flush the MediaCodec and reset the AccessUnitQueue. s/Flush/Flushes/, s/reset/resets/ https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.h:74: // Release MediaCodecBridge. s/Release/Releases/ https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.h:77: // Return corresponding conditions. s/Return/Returns/ https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.h:82: // Store configuration for the use of upcoming Configure() s/Store/Stores/ https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.h:87: // Start prefetching: accumulate enough data in AccessUnitQueue. s/Start/Starts/ https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.h:91: // Configure MediaCodec. s/Configure/Configures/ https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.h:94: // Start the decoder thread and resume the playback. s/Start/Starts/ https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.h:97: // Stop the playback process synchronously. s/Stop/Stops/ https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.h:128: // Render the decoded frame and release output buffer, or post s/Render/Renders/, s/post/posts/ https://codereview.chromium.org/1128383003/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.h:136: // Release output buffers that are dequeued and not released yet s/Release/Releases/ https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:608: request_resources_cb_)); On 2015/05/22 23:37:52, Tima Vaisburd wrote: > On 2015/05/20 15:48:16, qinmin wrote: > > On 2015/05/20 05:19:58, Tima Vaisburd wrote: > > > On 2015/05/19 19:26:25, qinmin wrote: > > > > request_resources_cb_ is binded to the UI thread, and now it is passed to > > the > > > > video_decoder_, which has no idea about the UI message loop. So it will be > > > > called on the wrong message loop. > > > > > > ui_task_runner_ is accessible from VideoDecoder via protected member and is > > used > > > there. I also verified that this callback is called on UI thread by printing > > > tid. > > > > I don't feel it proper for Decoder to know ui_task_runner. They are created on > > media thread, and running on decoder thread, they don't need to know about the > > ui thread. I think MediaCodecPlayer should have its own > requestVideoResources() > > call, and pass that function as a callback to the decoder job. And when that > > call runs, mediaCodecPlayer will post a task to UI thread to inform > > BrowserMediaPlayerManager. > > We have two callbacks now, both from the video decoder: requestVideoResources > and videoResolutionChanged. I would think we want to avoid an extra thread > hopping for efficiency, like we did with demuxer tasks. What do you think about > moving the ui_task_runner from Decoder to the VideoDecoder subclass and post the > task to it directly, like now? No, i don't that's logically sound. And I don't think a 2 hop CB will have impact on performances as request_resources_cb is not frequently called. https://codereview.chromium.org/1128383003/diff/180001/content/browser/media/... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/1128383003/diff/180001/content/browser/media/... content/browser/media/android/browser_media_player_manager.cc:468: //this, just drop this line https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... File media/base/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_audio_decoder.cc:9: remove extra line https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... File media/base/android/media_codec_audio_decoder.h (right): https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_audio_decoder.h:27: const char * class_name() const override { return "AudioDecoder"; } s/char */char*/ https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_decoder.h:51: const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner, remove dependence on ui_task_runner in decoders. https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_decoder.h:124: // Associate PTS with device time so we can calculate delays. s/Associate/Associates/ and fix all the verbs below https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_player.cc:12: remove extra line https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_player.cc:107: interpolator_.SetUpperBound(base::TimeDelta()); this is still not addressed. You will hit DCHECK(lower_bound != kNoTimestamp()) in SetUpperBound() if the interpolator is not started https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_player.cc:480: base::AutoLock lock(interpolator_lock_); MediaCodecPlayer should not run on the decoder thread. And we don't need this lock. The decoder can post a callback on to the media thread to call this function. Since the time is propagated back to the render process, I don't see a stringent requirement on precision. https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:81: ui_task_runner_->PostTask( remove reliance on ui task runner in decoders. this can be done by returning a boolean back to MediaCodecPlayer, and let MediaCodecPlayer post a task to ui thread if SetDemuxerConfigs returns true. https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:113: if (!au_queue_.SkipToKeyFrame()) { This will still skip frames that are not rendered, which is bad. Each time we enqueue an AU, AccessUnitQueue.PopFront() is called. So we always lose the key frame that we should roll back to. The current implementation doesn't clean up the vector immediately after rendering.
I tried to address all Min comments and most Matthew comments with the exception of the SM diagram which I plan to do next. One thing that remains to be done is unit tests. We originally planned to give the code a first try in this quarter, which requires at least to do seek, browser seek and the surface change. Would it be possible to postpone the tests to another CL? I wrote a bug to myself as a reminder: http://crbug.com/492944. https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1128383003/diff/140001/media/base/android/acc... media/base/android/access_unit_queue.h:57: bool SkipToKeyFrame(); On 2015/05/19 21:58:37, wolenetz wrote: > What if there is a config prior to the keyframe? As Min pointed out below, instead of going forward in time I should have gone backward, the same way it is done currently. I think this way we will always get the key frame that is consistent with the current configuration. I changed the implementation of AccessUnitQueue to use the list of chunks. This allows for some limited search backwards. https://codereview.chromium.org/1128383003/diff/180001/content/browser/media/... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/1128383003/diff/180001/content/browser/media/... content/browser/media/android/browser_media_player_manager.cc:468: //this, On 2015/05/26 00:49:19, qinmin wrote: > just drop this line Done. https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... File media/base/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_audio_decoder.cc:9: On 2015/05/26 00:49:19, qinmin wrote: > remove extra line Done. https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... File media/base/android/media_codec_audio_decoder.h (right): https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_audio_decoder.h:27: const char * class_name() const override { return "AudioDecoder"; } On 2015/05/26 00:49:19, qinmin wrote: > s/char */char*/ Done. https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_decoder.h:51: const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner, On 2015/05/26 00:49:19, qinmin wrote: > remove dependence on ui_task_runner in decoders. Done. https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_decoder.h:124: // Associate PTS with device time so we can calculate delays. On 2015/05/26 00:49:19, qinmin wrote: > s/Associate/Associates/ and fix all the verbs below Done. https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_player.cc:12: On 2015/05/26 00:49:19, qinmin wrote: > remove extra line Done. https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_player.cc:107: interpolator_.SetUpperBound(base::TimeDelta()); On 2015/05/26 00:49:19, qinmin wrote: > this is still not addressed. You will hit DCHECK(lower_bound != kNoTimestamp()) > in SetUpperBound() if the interpolator is not started Hm, I see that the upper_bound (input parameter) is DCHECKed, not lower bound: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/time_de... The input parameter is 0 which is different from kNoTimestamp(). The lower bound is constructed with 0, as far as I see. What am I missing? https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_player.cc:480: base::AutoLock lock(interpolator_lock_); On 2015/05/26 00:49:19, qinmin wrote: > MediaCodecPlayer should not run on the decoder thread. > And we don't need this lock. The decoder can post a callback on to the media > thread to call this function. Since the time is propagated back to the render > process, I don't see a stringent requirement on precision. Done. GetCurrentTime() now returns a cached value, similarly to media metadata. GetCurrentTime() is only called on UI thread, for Media thread there is GetInterpolatedTime(). https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:81: ui_task_runner_->PostTask( On 2015/05/26 00:49:20, qinmin wrote: > remove reliance on ui task runner in decoders. this can be done by returning a > boolean back to MediaCodecPlayer, and let MediaCodecPlayer post a task to ui > thread if SetDemuxerConfigs returns true. ui_task_runner_ is removed. Here I posted the new callback the same way I did in OnOutputFormatChanged(). https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:113: if (!au_queue_.SkipToKeyFrame()) { On 2015/05/26 00:49:19, qinmin wrote: > This will still skip frames that are not rendered, which is bad. > Each time we enqueue an AU, AccessUnitQueue.PopFront() is called. So we always > lose the key frame that we should roll back to. > The current implementation doesn't clean up the vector immediately after > rendering. Changed the AccessUnitQueue implementation. Now it uses the list of pointers to DemuxerData chunks, so we can search back (search is implemented only within one chunk). Because we store pointers in the queue the copying happens outside of the lock.
With Patch Set 12 I tried to address all your comments except for unit tests, which I humbly asked to commit in another CL. Please provide your further comments and suggestions. https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... media/base/android/media_codec_player.h:97: STATE_WAITING_FOR_CONFIG, On 2015/05/20 18:20:04, wolenetz wrote: > nit: this state isn't represented in the state diagram. Please include it there. Done. https://codereview.chromium.org/1128383003/diff/160001/media/base/android/med... media/base/android/media_codec_player.h:102: STATE_WAITING_FOR_SEEK, On 2015/05/20 18:20:04, wolenetz wrote: > nit ditto Done.
On 2015/05/28 19:44:31, Tima Vaisburd wrote: > With Patch Set 12 I tried to address all your comments except for unit tests, > which I humbly asked to commit in another CL. Please provide your further > comments and suggestions. While I get that MediaSourcePlayerTests have become quite unwieldy, please do include basic unit tests for the function of AUQueue, etc. Elaborate tests can be added later, but going forward without any tests seems the wrong approach to me IMHO.
Got it, will do. On Thu, May 28, 2015 at 1:21 PM, <wolenetz@chromium.org> wrote: > On 2015/05/28 19:44:31, Tima Vaisburd wrote: > >> With Patch Set 12 I tried to address all your comments except for unit >> tests, >> which I humbly asked to commit in another CL. Please provide your further >> comments and suggestions. >> > > While I get that MediaSourcePlayerTests have become quite unwieldy, please > do > include basic unit tests for the function of AUQueue, etc. Elaborate tests > can > be added later, but going forward without any tests seems the wrong > approach to > me IMHO. > > https://codereview.chromium.org/1128383003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/180001/media/base/android/med... media/base/android/media_codec_player.cc:107: interpolator_.SetUpperBound(base::TimeDelta()); On 2015/05/28 02:00:34, Tima Vaisburd wrote: > On 2015/05/26 00:49:19, qinmin wrote: > > this is still not addressed. You will hit DCHECK(lower_bound != > kNoTimestamp()) > > in SetUpperBound() if the interpolator is not started > > Hm, I see that the upper_bound (input parameter) is DCHECKed, not lower bound: > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/time_de... > The input parameter is 0 which is different from kNoTimestamp(). The lower bound > is constructed with 0, as far as I see. What am I missing? hmm..I must be confused by something else. This should be ok. https://codereview.chromium.org/1128383003/diff/200001/media/base/android/acc... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1128383003/diff/200001/media/base/android/acc... media/base/android/access_unit_queue.cc:31: for (const AccessUnit& unit : data.access_units) { When DCHECK_IS_ON() is turned off, this for loop only checks the has_eos variable. In that case, we should start from the last access unit to reduce the number of loops. Or even better, we shouldn't do the loop to get EOS here, just do has_eos_ = data.access_units.back().is_end_of_stream after the lock is acquired. https://codereview.chromium.org/1128383003/diff/220001/media/base/android/acc... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1128383003/diff/220001/media/base/android/acc... media/base/android/access_unit_queue.cc:130: DCHECK(!(*current_chunk_)->demuxer_configs.empty()); This DCHECK is useless, the next DCHECK covers this one. https://codereview.chromium.org/1128383003/diff/220001/media/base/android/acc... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1128383003/diff/220001/media/base/android/acc... media/base/android/access_unit_queue.h:67: int GetLength() const; The function name and description is misleading. What about GetPendingAccessUnitLength() or GetUndecodedAccessUnitLength() https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... media/base/android/media_codec_decoder.cc:402: base::TimeDelta timeout = base::TimeDelta::FromMilliseconds(20); make a constant variable of the delay https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... media/base/android/media_codec_decoder.cc:415: break; no need https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... media/base/android/media_codec_decoder.cc:419: break; no need for break https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... media/base/android/media_codec_player.h:228: MediaMetadata() : duration(), video_size(320, 240) {} default size should be (0,0), MSE is not for video, it could be audio. Sooner or later, the metadata callbacks will update the size in the renderer later. https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... media/base/android/media_codec_player.h:261: bool AudioFinished(); the ordering of all the member functions doesn't match with there ordering in the cc file, please fix that, also in other files.
Publishing AccessUnitQueue tests, other tests are not done yet. https://codereview.chromium.org/1128383003/diff/200001/media/base/android/acc... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1128383003/diff/200001/media/base/android/acc... media/base/android/access_unit_queue.cc:31: for (const AccessUnit& unit : data.access_units) { On 2015/05/31 20:19:12, qinmin(OOO_till_6-5) wrote: > When DCHECK_IS_ON() is turned off, this for loop only checks the has_eos > variable. In that case, we should start from the last access unit to reduce the > number of loops. > Or even better, we shouldn't do the loop to get EOS here, just do has_eos_ = > data.access_units.back().is_end_of_stream after the lock is acquired. Done: check that EOS is in the last unit in chunk when DCHECK_IS_ON(), but assume this without checking when DCHECK is off. https://codereview.chromium.org/1128383003/diff/220001/media/base/android/acc... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1128383003/diff/220001/media/base/android/acc... media/base/android/access_unit_queue.cc:130: DCHECK(!(*current_chunk_)->demuxer_configs.empty()); On 2015/05/31 20:19:12, qinmin(OOO_till_6-5) wrote: > This DCHECK is useless, the next DCHECK covers this one. Done. https://codereview.chromium.org/1128383003/diff/220001/media/base/android/acc... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1128383003/diff/220001/media/base/android/acc... media/base/android/access_unit_queue.h:67: int GetLength() const; On 2015/05/31 20:19:12, qinmin(OOO_till_6-5) wrote: > The function name and description is misleading. What about > GetPendingAccessUnitLength() or GetUndecodedAccessUnitLength() The second name. However, after having chunks the length should probably be in terms of chunks and not access units. https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... media/base/android/media_codec_decoder.cc:402: base::TimeDelta timeout = base::TimeDelta::FromMilliseconds(20); On 2015/05/31 20:19:12, qinmin(OOO_till_6-5) wrote: > make a constant variable of the delay Done. https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... media/base/android/media_codec_decoder.cc:415: break; On 2015/05/31 20:19:12, qinmin(OOO_till_6-5) wrote: > no need Removed |break;| https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... media/base/android/media_codec_decoder.cc:419: break; On 2015/05/31 20:19:12, qinmin(OOO_till_6-5) wrote: > no need for break Ditto https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... media/base/android/media_codec_player.h:228: MediaMetadata() : duration(), video_size(320, 240) {} On 2015/05/31 20:19:13, qinmin(OOO_till_6-5) wrote: > default size should be (0,0), MSE is not for video, it could be audio. > Sooner or later, the metadata callbacks will update the size in the renderer > later. I did non-empty default video size to prevent a crash in case someone would try to calculate the aspect ratio before the size is updated with the true value. I'd think that (1,1) would be as good as (320,240) and maybe clearer, but better than (0,0). > MSE is not for video, it could be audio. I thought for audio-only nobody would call GetVideoWidth/GetVideoHeight so the value does not matter? https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... media/base/android/media_codec_player.h:261: bool AudioFinished(); On 2015/05/31 20:19:13, qinmin(OOO_till_6-5) wrote: > the ordering of all the member functions doesn't match with there ordering in > the cc file, please fix that, also in other files. Done.
Added initial unit tests to the player as a whole. Please take another look. https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/220001/media/base/android/med... media/base/android/media_codec_player.h:228: MediaMetadata() : duration(), video_size(320, 240) {} On 2015/06/01 18:56:52, Tima Vaisburd wrote: > On 2015/05/31 20:19:13, qinmin(OOO_till_6-5) wrote: > > default size should be (0,0), MSE is not for video, it could be audio. > > Sooner or later, the metadata callbacks will update the size in the renderer > > later. > > I did non-empty default video size to prevent a crash in case someone would try > to calculate the aspect ratio before the size is updated with the true value. > I'd think that (1,1) would be as good as (320,240) and maybe clearer, but better > than (0,0). > > > MSE is not for video, it could be audio. > I thought for audio-only nobody would call GetVideoWidth/GetVideoHeight so the > value does not matter? Never mind my comment, made the size (0,0).
Review still in progress! I thought I'd send these minor comments in case you wanted to address any while I continue reviewing. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.cc:61: --current_chunk_; It seems more obvious to use chunks_.begin()? https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.cc:118: void AccessUnitQueue::GetInfo(Info* info) const { Any reason to not return the Info by value here? It seems like it would be a nicer api and it's a tiny struct. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.cc:145: result -= index_in_chunk_; Can you move this outside of the for loop and remove the if-statement? https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.h:51: // Advances the front position to next unit. Logically the preceding units s/units/unit https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.h:73: typedef std::list< scoped_ptr<DemuxerData> > DataChunkQueue; Spaces around the template param is unconventional I think. clang format removes these spaces. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.h:85: // The queue works on two threads. I suggest saying instead what the lock protects. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/dem... File media/base/android/demuxer_stream_player_params.h (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/dem... media/base/android/demuxer_stream_player_params.h:71: std::ostream& operator<<(std::ostream& os, const media::AccessUnit& au); Should this be MEDIA_EXPORTed because AccessUnit is? https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.cc:17: // Stop requesting new data in the PREFETCH state when the queue size reached s/reached/reaches https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.cc:19: const int PREFETCH_LIMIT = 8; The chromium convention is kConstantNaming style https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.cc:61: // Media thread I would remove these comments because they're redundant with the DHCECKs below them. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.h:102: // Decoder will stop asynchronously after all the dequeued s/dequeued/queued? https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.h:114: // Returns true if the new DemuxerConfids requires MediaCodec s/DemuxerConfids/DemuxerConfigs https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:39: DVLOG(1) << "VideoDecoder::~VideoDecoder()"; Why aren't you using "MediaCodecVideoDecoder"? IMO it's confusing to log these names that don't correspond to real classes.
Some more minor comments. I'm still wrapping my head around the architecture so I will send an update tomorrow if I have any higher level comments. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_audio_decoder.cc:172: buffer_index, size); Formatting is a little hard to follow. I ran clang format on this and it turned it into this: if (render_output) { int64 head_position = (static_cast<AudioCodecBridge*>(media_codec_bridge_.get())) ->PlayOutputBuffer(buffer_index, size); https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.cc:262: #if 0 Delete this? https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.cc:481: int timeout_index = 0; I think it would be simpler to just initialize "timeout" to base::TimeDelta::FromMilliseconds(OUTPUT_BUFFER_TIMEOUT_MS); and later set it to base::TimeDelta::FromMilliseconds(0);. Can you also add a comment on why we allow a timeout on the first one but not subsequent ones? It's not obvious to me https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.cc:514: break; Could you add a comment on why it's ok to ignore other statuses? It's not obvious to me. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_player.cc:278: // This method is called to check whether it's save to release the player if s/save/safe https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_player.h:223: // Cached valued for the manager, accessed on the UI thread. s/valued/values https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_player.h:305: // Pendng data to be picked up by the upcoming state. s/Pendng/Pending https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_player_unittest.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_player_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. s/2013/2015 https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_video_decoder.h (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_video_decoder.h:80: // Indexes ouf output buffers that are posted for rendering. s/Indexes ouf/Indices of https://codereview.chromium.org/1128383003/diff/280001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/1128383003/diff/280001/media/media.gyp#newcod... media/media.gyp:1747: 'base/android/media_codec_audio_decoder.h', Slightly out of order :P
https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.cc:61: --current_chunk_; On 2015/06/04 20:56:43, watk wrote: > It seems more obvious to use chunks_.begin()? I need an iterator that points to the last (newly added) element, not the first one. I changed to current_chunk_ = --chunks_.end(); https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.cc:118: void AccessUnitQueue::GetInfo(Info* info) const { On 2015/06/04 20:56:43, watk wrote: > Any reason to not return the Info by value here? It seems like it would be a > nicer api and it's a tiny struct. Still it would be perhaps 16 bytes instead of 4, why wouldn't we care? https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.cc:145: result -= index_in_chunk_; On 2015/06/04 20:56:43, watk wrote: > Can you move this outside of the for loop and remove the if-statement? I redid the method but not sure I like it better the new way. Please take a look. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.h:51: // Advances the front position to next unit. Logically the preceding units On 2015/06/04 20:56:43, watk wrote: > s/units/unit Actually s/does/do because there could be several units preceding the front one. Anyway, done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.h:73: typedef std::list< scoped_ptr<DemuxerData> > DataChunkQueue; On 2015/06/04 20:56:43, watk wrote: > Spaces around the template param is unconventional I think. clang format removes > these spaces. Done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.h:85: // The queue works on two threads. On 2015/06/04 20:56:43, watk wrote: > I suggest saying instead what the lock protects. Wrote "The lock protects all fields together". https://codereview.chromium.org/1128383003/diff/280001/media/base/android/dem... File media/base/android/demuxer_stream_player_params.h (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/dem... media/base/android/demuxer_stream_player_params.h:71: std::ostream& operator<<(std::ostream& os, const media::AccessUnit& au); On 2015/06/04 20:56:43, watk wrote: > Should this be MEDIA_EXPORTed because AccessUnit is? Done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_audio_decoder.cc:172: buffer_index, size); On 2015/06/05 01:00:32, watk wrote: > Formatting is a little hard to follow. I ran clang format on this and it turned > it into this: > > if (render_output) { > int64 head_position = > (static_cast<AudioCodecBridge*>(media_codec_bridge_.get())) > ->PlayOutputBuffer(buffer_index, size); Done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.cc:17: // Stop requesting new data in the PREFETCH state when the queue size reached On 2015/06/04 20:56:43, watk wrote: > s/reached/reaches Done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.cc:19: const int PREFETCH_LIMIT = 8; On 2015/06/04 20:56:43, watk wrote: > The chromium convention is kConstantNaming style Done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.cc:61: // Media thread On 2015/06/04 20:56:43, watk wrote: > I would remove these comments because they're redundant with the DHCECKs below > them. Done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.cc:262: #if 0 On 2015/06/05 01:00:33, watk wrote: > Delete this? It is useful for debugging, I did #if !defined(NDEBUG) https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.cc:481: int timeout_index = 0; On 2015/06/05 01:00:32, watk wrote: > I think it would be simpler to just initialize "timeout" to > base::TimeDelta::FromMilliseconds(OUTPUT_BUFFER_TIMEOUT_MS); and later set it to > base::TimeDelta::FromMilliseconds(0);. > > Can you also add a comment on why we allow a timeout on the first one but not > subsequent ones? It's not obvious to me Yes, it's obviously simpler to just replace the timeout, thank you. I tried to explain in the comments why there are two timeouts, not sure how clear it went. The idea here is that we do want some timeout (you give 20 ms, normally you get the buffer in 1-5 ms), but after you get the first buffer usually there is nothing else, and you do not want to spend another 20 ms to figure this out. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.cc:514: break; On 2015/06/05 01:00:32, watk wrote: > Could you add a comment on why it's ok to ignore other statuses? It's not > obvious to me. Well, there are just two more: MEDIA_CODEC_OUTPUT_BUFFERS_CHANGED which is taken care of in the bridge and MEDIA_CODEC_DEQUEUE_OUTPUT_AGAIN_LATER which breaks the loop. I added them both. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.h:102: // Decoder will stop asynchronously after all the dequeued On 2015/06/04 20:56:43, watk wrote: > s/dequeued/queued? I meant output buffers that we got from MediaCodec by DequeueOutputBuffer() and have not been released yet. My attempts to reformulate it better and avoid the non-existing word 'dequeue' did not succeed. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_decoder.h:114: // Returns true if the new DemuxerConfids requires MediaCodec On 2015/06/04 20:56:43, watk wrote: > s/DemuxerConfids/DemuxerConfigs Done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_player.cc:278: // This method is called to check whether it's save to release the player if On 2015/06/05 01:00:33, watk wrote: > s/save/safe Done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_player.h:223: // Cached valued for the manager, accessed on the UI thread. On 2015/06/05 01:00:33, watk wrote: > s/valued/values Done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_player.h:305: // Pendng data to be picked up by the upcoming state. On 2015/06/05 01:00:33, watk wrote: > s/Pendng/Pending Done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_player_unittest.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_player_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/06/05 01:00:33, watk wrote: > s/2013/2015 Done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:39: DVLOG(1) << "VideoDecoder::~VideoDecoder()"; On 2015/06/04 20:56:43, watk wrote: > Why aren't you using "MediaCodecVideoDecoder"? IMO it's confusing to log these > names that don't correspond to real classes. It corresponds to what class_name() returns. I'd like to have concise names in logs (especially for prefixes!), and these names (AudioDecoder or VideoDecoder) correspond to the logical division (Player or Decoder, Audio or Video). https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... File media/base/android/media_codec_video_decoder.h (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/med... media/base/android/media_codec_video_decoder.h:80: // Indexes ouf output buffers that are posted for rendering. On 2015/06/05 01:00:33, watk wrote: > s/Indexes ouf/Indices of Done. https://codereview.chromium.org/1128383003/diff/280001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/1128383003/diff/280001/media/media.gyp#newcod... media/media.gyp:1747: 'base/android/media_codec_audio_decoder.h', On 2015/06/05 01:00:33, watk wrote: > Slightly out of order :P Oops...
Looks good Tima! I wasn't able to understand it all as well as I'd hoped, and I'm going to OOO for 2 weeks starting Monday, so unfortunately I have to pass the review back to wolenetz@ to give the final stamp. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.cc:61: --current_chunk_; On 2015/06/05 04:17:46, Tima Vaisburd wrote: > On 2015/06/04 20:56:43, watk wrote: > > It seems more obvious to use chunks_.begin()? > > I need an iterator that points to the last (newly added) element, not the first > one. I changed to > current_chunk_ = --chunks_.end(); Oh, I thought was_empty meant chunks_ was empty https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.cc:118: void AccessUnitQueue::GetInfo(Info* info) const { On 2015/06/05 04:17:46, Tima Vaisburd wrote: > On 2015/06/04 20:56:43, watk wrote: > > Any reason to not return the Info by value here? It seems like it would be a > > nicer api and it's a tiny struct. > > Still it would be perhaps 16 bytes instead of 4, why wouldn't we care? With RVO I doubt this will be a copy. In any case, even if we're copying 16 bytes I think it's worth it. Totally up to you, though! https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.cc:145: result -= index_in_chunk_; On 2015/06/05 04:17:46, Tima Vaisburd wrote: > On 2015/06/04 20:56:43, watk wrote: > > Can you move this outside of the for loop and remove the if-statement? > > I redid the method but not sure I like it better the new way. Please take a > look. I agree your previous version was better than the current one. Sorry for being confusing. I was thinking this: int result = 0; DataChunkQueue::const_iterator chunk; for (chunk = current_chunk_; chunk != chunks_.end(); ++chunk) { result += (*chunk)->access_units.size(); } result -= index_in_chunk_;
On 2015/06/05 20:13:22, watk wrote: > Looks good Tima! I wasn't able to understand it all as well as I'd hoped, and > I'm going to OOO for 2 weeks starting Monday, so unfortunately I have to pass > the review back to wolenetz@ to give the final stamp. > > https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... > File media/base/android/access_unit_queue.cc (right): > > https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... > media/base/android/access_unit_queue.cc:61: --current_chunk_; > On 2015/06/05 04:17:46, Tima Vaisburd wrote: > > On 2015/06/04 20:56:43, watk wrote: > > > It seems more obvious to use chunks_.begin()? > > > > I need an iterator that points to the last (newly added) element, not the > first > > one. I changed to > > current_chunk_ = --chunks_.end(); > > Oh, I thought was_empty meant chunks_ was empty > > https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... > media/base/android/access_unit_queue.cc:118: void AccessUnitQueue::GetInfo(Info* > info) const { > On 2015/06/05 04:17:46, Tima Vaisburd wrote: > > On 2015/06/04 20:56:43, watk wrote: > > > Any reason to not return the Info by value here? It seems like it would be a > > > nicer api and it's a tiny struct. > > > > Still it would be perhaps 16 bytes instead of 4, why wouldn't we care? > > With RVO I doubt this will be a copy. In any case, even if we're copying 16 > bytes I think it's worth it. > > Totally up to you, though! > > https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... > media/base/android/access_unit_queue.cc:145: result -= index_in_chunk_; > On 2015/06/05 04:17:46, Tima Vaisburd wrote: > > On 2015/06/04 20:56:43, watk wrote: > > > Can you move this outside of the for loop and remove the if-statement? > > > > I redid the method but not sure I like it better the new way. Please take a > > look. > > I agree your previous version was better than the current one. Sorry for being > confusing. I was thinking this: > > int result = 0; > DataChunkQueue::const_iterator chunk; > for (chunk = current_chunk_; chunk != chunks_.end(); ++chunk) { > result += (*chunk)->access_units.size(); > } > result -= index_in_chunk_; Thank you, watk@, for helping get this CL further through the review. I'll pick up the CR now from here (along with qinmin@). timav@: On a little further retrospection on the scope of even just this CL, it's about 3300 lines. Please consider finding ways of breaking it down into smaller chunks to land. One piece that seems ready to split off and land as a prerequisite for the rest is the access unit queue piece. Can you split that off please? Thanks, Matt
On 2015/06/05 20:39:13, wolenetz wrote: > On 2015/06/05 20:13:22, watk wrote: > > Looks good Tima! I wasn't able to understand it all as well as I'd hoped, and > > I'm going to OOO for 2 weeks starting Monday, so unfortunately I have to pass > > the review back to wolenetz@ to give the final stamp. > > > > > https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... > > File media/base/android/access_unit_queue.cc (right): > > > > > https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... > > media/base/android/access_unit_queue.cc:61: --current_chunk_; > > On 2015/06/05 04:17:46, Tima Vaisburd wrote: > > > On 2015/06/04 20:56:43, watk wrote: > > > > It seems more obvious to use chunks_.begin()? > > > > > > I need an iterator that points to the last (newly added) element, not the > > first > > > one. I changed to > > > current_chunk_ = --chunks_.end(); > > > > Oh, I thought was_empty meant chunks_ was empty > > > > > https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... > > media/base/android/access_unit_queue.cc:118: void > AccessUnitQueue::GetInfo(Info* > > info) const { > > On 2015/06/05 04:17:46, Tima Vaisburd wrote: > > > On 2015/06/04 20:56:43, watk wrote: > > > > Any reason to not return the Info by value here? It seems like it would be > a > > > > nicer api and it's a tiny struct. > > > > > > Still it would be perhaps 16 bytes instead of 4, why wouldn't we care? > > > > With RVO I doubt this will be a copy. In any case, even if we're copying 16 > > bytes I think it's worth it. > > > > Totally up to you, though! > > > > > https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... > > media/base/android/access_unit_queue.cc:145: result -= index_in_chunk_; > > On 2015/06/05 04:17:46, Tima Vaisburd wrote: > > > On 2015/06/04 20:56:43, watk wrote: > > > > Can you move this outside of the for loop and remove the if-statement? > > > > > > I redid the method but not sure I like it better the new way. Please take a > > > look. > > > > I agree your previous version was better than the current one. Sorry for being > > confusing. I was thinking this: > > > > int result = 0; > > DataChunkQueue::const_iterator chunk; > > for (chunk = current_chunk_; chunk != chunks_.end(); ++chunk) { > > result += (*chunk)->access_units.size(); > > } > > result -= index_in_chunk_; > > Thank you, watk@, for helping get this CL further through the review. I'll pick > up the CR now from here (along with qinmin@). > > timav@: On a little further retrospection on the scope of even just this CL, > it's about 3300 lines. Please consider finding ways of breaking it down into > smaller chunks to land. One piece that seems ready to split off and land as a > prerequisite for the rest is the access unit queue piece. Can you split that off > please? > > Thanks, > Matt Yes, sure, I will be happy to do this.
https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.cc:61: --current_chunk_; On 2015/06/05 20:13:22, watk wrote: > On 2015/06/05 04:17:46, Tima Vaisburd wrote: > > On 2015/06/04 20:56:43, watk wrote: > > > It seems more obvious to use chunks_.begin()? > > > > I need an iterator that points to the last (newly added) element, not the > first > > one. I changed to > > current_chunk_ = --chunks_.end(); > > Oh, I thought was_empty meant chunks_ was empty I did not explain it well. I agree with you that right now, when we keep no history, here first and last chunk is the same one. But I'd like to leave an option to have longer history, in this case was_empty means that Advance() passed through the end, but does not mean the chunks_ is actually empty. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.cc:118: void AccessUnitQueue::GetInfo(Info* info) const { On 2015/06/05 20:13:22, watk wrote: > On 2015/06/05 04:17:46, Tima Vaisburd wrote: > > On 2015/06/04 20:56:43, watk wrote: > > > Any reason to not return the Info by value here? It seems like it would be a > > > nicer api and it's a tiny struct. > > > > Still it would be perhaps 16 bytes instead of 4, why wouldn't we care? > > With RVO I doubt this will be a copy. In any case, even if we're copying 16 > bytes I think it's worth it. > > Totally up to you, though! Done. https://codereview.chromium.org/1128383003/diff/280001/media/base/android/acc... media/base/android/access_unit_queue.cc:145: result -= index_in_chunk_; On 2015/06/05 20:13:22, watk wrote: > On 2015/06/05 04:17:46, Tima Vaisburd wrote: > > On 2015/06/04 20:56:43, watk wrote: > > > Can you move this outside of the for loop and remove the if-statement? > > > > I redid the method but not sure I like it better the new way. Please take a > > look. > > I agree your previous version was better than the current one. Sorry for being > confusing. I was thinking this: > > int result = 0; > DataChunkQueue::const_iterator chunk; > for (chunk = current_chunk_; chunk != chunks_.end(); ++chunk) { > result += (*chunk)->access_units.size(); > } > result -= index_in_chunk_; > Shame on me! Done.
> Please consider finding ways of breaking it down into > smaller chunks to land. One piece that seems ready to split off and land as a > prerequisite for the rest is the access unit queue piece. Except for AccessUnitQueue (already split off) I see the following logical pieces: 1. BrowserMediaPlayerManager - removed one argument there, probably too small of a change, 2. Decoder - base class, audio and video, 3. Player - MediaPlayerAndroid and MediaCodecPlayer. Does it sound right to you to split the decoder off?
On 2015/06/05 23:26:09, Tima Vaisburd wrote: > > Please consider finding ways of breaking it down into > > smaller chunks to land. One piece that seems ready to split off and land as a > > prerequisite for the rest is the access unit queue piece. > > Except for AccessUnitQueue (already split off) I see the following logical > pieces: > 1. BrowserMediaPlayerManager - removed one argument there, probably too small of > a change, > 2. Decoder - base class, audio and video, > 3. Player - MediaPlayerAndroid and MediaCodecPlayer. > > Does it sound right to you to split the decoder off? Yes, conceptually, splitting off the decoder would also elicit places where it might be too tightly bound to MPA/MediaCodecPlayer/etc.
On 2015/06/05 23:26:09, Tima Vaisburd wrote: > > Please consider finding ways of breaking it down into > > smaller chunks to land. One piece that seems ready to split off and land as a > > prerequisite for the rest is the access unit queue piece. > > Except for AccessUnitQueue (already split off) I see the following logical > pieces: > 1. BrowserMediaPlayerManager - removed one argument there, probably too small of > a change, > 2. Decoder - base class, audio and video, > 3. Player - MediaPlayerAndroid and MediaCodecPlayer. > > Does it sound right to you to split the decoder off? Yes, conceptually, splitting off the decoder would also elicit places where it might be too tightly bound to MPA/MediaCodecPlayer/etc.
> Yes, conceptually, splitting off the decoder would also elicit places where it > might be too tightly bound to MPA/MediaCodecPlayer/etc. Since decoder includes the AccessUnitQueue, the work of splitting off decoder depends on committing the AccessUnitQueue first. Could you, please, review https://codereview.chromium.org/1162203009/ ? Thanks much!
On 2015/06/08 17:38:56, Tima Vaisburd wrote: > > Yes, conceptually, splitting off the decoder would also elicit places where it > > might be too tightly bound to MPA/MediaCodecPlayer/etc. > > Since decoder includes the AccessUnitQueue, the work of splitting off decoder > depends on committing the AccessUnitQueue first. > Could you, please, review https://codereview.chromium.org/1162203009/ ? Thanks > much! Yep, I totally agree. I've sent my comments now on that prereq CL.
liberato@chromium.org changed reviewers: + liberato@chromium.org
watk@ pointed me at this CL. just a few minor comments. looks really nice. -fl https://codereview.chromium.org/1128383003/diff/320001/media/base/android/med... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/320001/media/base/android/med... media/base/android/media_codec_decoder.cc:345: decoder_thread_.task_runner()->PostDelayedTask( alternatively, could we just not post if state_==STOP (modulo thread safety). https://codereview.chromium.org/1128383003/diff/320001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/320001/media/base/android/med... media/base/android/media_codec_player.cc:681: base::Bind(&MediaCodecPlayer::OnTimeIntervalUpdate, weak_this_))); who calls the time interval update cb if there's no audio stream?
https://codereview.chromium.org/1128383003/diff/320001/media/base/android/med... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1128383003/diff/320001/media/base/android/med... media/base/android/media_codec_decoder.cc:345: decoder_thread_.task_runner()->PostDelayedTask( On 2015/06/09 15:44:30, liberato wrote: > alternatively, could we just not post if state_==STOP (modulo thread safety). I did this to support SyncStop() in any state. SyncStop() is called from player when I did not see the reason to wait, e.g. in destructor or Release() or error. https://codereview.chromium.org/1128383003/diff/320001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1128383003/diff/320001/media/base/android/med... media/base/android/media_codec_player.cc:681: base::Bind(&MediaCodecPlayer::OnTimeIntervalUpdate, weak_this_))); On 2015/06/09 15:44:31, liberato wrote: > who calls the time interval update cb if there's no audio stream? Noone! This part is not finished.
timav@: Should this CL be closed since it's been broken into multiple other pieces, of which at least one has already landed?
On 2015/06/22 22:03:55, wolenetz wrote: > timav@: Should this CL be closed since it's been broken into multiple other > pieces, of which at least one has already landed? Closing this CL since it has been splitted into 3 smaller ones: https://codereview.chromium.org/1162203009/ https://codereview.chromium.org/1176993005/ https://codereview.chromium.org/1184373005/ |
