Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
254473010:
Refactor MSE implementation on Android to simplify the logic and improve the performance (Closed)
|
Created:
6 years, 8 months ago by qinmin Modified:
6 years, 6 months ago CC:
chromium-reviews, fischman+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:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor MSE implementation on Android to simplify the logic and improve the performance
The MediaSourcePlayer processes several pending events, and some of them it should not care. For example: SURFACE_CHANGE and CONFIG_CHANGE.
This change pushes the handling of surface change and config change into the MediaDecoderJob, and simplifies a lot of logic:
1. MSP now assumes that AudioDecoderJob and VideoDecoderJob is always around, it is up to AudioDecoderJob/VideoDecoderJob to handle MediaCodecBridge recreation.
2. When config changes, MediaDecoderJob will not immediately recreate a new MediaCodecBridge.
Instead, it will drain the current MediaCodecBridge until all the buffered data are decoded. Then it starts creating the new MediaCodecBridge.
BUG=357726
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275063
Patch Set 1 : #
Total comments: 14
Patch Set 2 : addressing comments #
Total comments: 4
Patch Set 3 : nits #Patch Set 4 : rebase after recent config IPC change #
Total comments: 31
Patch Set 5 : addressing comments #
Total comments: 12
Patch Set 6 : addressing comments #
Total comments: 29
Patch Set 7 : addressing xhwang's comments #Patch Set 8 : fixing eme playback issue #Patch Set 9 : passing an EOS access unit during config change #
Total comments: 37
Patch Set 10 : addressing comments on unit tests #
Total comments: 6
Patch Set 11 : fixing nits #
Messages
Total messages: 44 (0 generated)
PTAL
Some preliminary comments (nits). Before I review everything together, is it possible/simple to split out the portion of this CL that covers "When config change happens, the new config is attached to the Demuxer message to MSP" and land it separately? https://codereview.chromium.org/254473010/diff/20001/content/renderer/media/a... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/254473010/diff/20001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.cc:394: gfx::Size size = video_stream_->video_decoder_config().coded_size(); nit: Do we need to report coded_size() here? Or is natural_size() (from demuxer_configs[0].video_size) sufficient, allowing us to just query the video config at most once here (by moving the two new lines below to before this clause, and using the retrieved DemuxerConfigs' video_size here)? https://codereview.chromium.org/254473010/diff/20001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.cc:399: GetDeumuxerConfigFromStream(&data->demuxer_configs[0], is_audio); nit: ditto (spelling), here and below. https://codereview.chromium.org/254473010/diff/20001/content/renderer/media/a... File content/renderer/media/android/media_source_delegate.h (right): https://codereview.chromium.org/254473010/diff/20001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.h:181: void GetDeumuxerConfigFromStream( nit: s/Deumuxer/Demuxer https://codereview.chromium.org/254473010/diff/20001/media/base/android/audio... File media/base/android/audio_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/20001/media/base/android/audio... media/base/android/audio_decoder_job.h:57: std::vector<uint8> audio_extra_data_; lint nit: Add #include for vector<> https://codereview.chromium.org/254473010/diff/20001/media/base/android/demux... File media/base/android/demuxer_stream_player_params.h (right): https://codereview.chromium.org/254473010/diff/20001/media/base/android/demux... media/base/android/demuxer_stream_player_params.h:58: // a corresponding DemuxerConfigs is added into this vector. This solves the nit: mention that only the type-specific portion of DemuxerConfigs is populated? https://codereview.chromium.org/254473010/diff/20001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/254473010/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:655: while(demuxer_->num_data_requests() == expected_num_data_requests) lint nit: Missing space before ( in while( https://codereview.chromium.org/254473010/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1638: while(IsDrainingDecoder(true) || IsDrainingDecoder(false)) lint nit: Missing space before ( in while(
https://codereview.chromium.org/254473010/diff/20001/content/renderer/media/a... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/254473010/diff/20001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.cc:394: gfx::Size size = video_stream_->video_decoder_config().coded_size(); Done. we should simply using natural_size. On 2014/04/25 23:13:13, wolenetz wrote: > nit: Do we need to report coded_size() here? Or is natural_size() (from > demuxer_configs[0].video_size) sufficient, allowing us to just query the video > config at most once here (by moving the two new lines below to before this > clause, and using the retrieved DemuxerConfigs' video_size here)? https://codereview.chromium.org/254473010/diff/20001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.cc:399: GetDeumuxerConfigFromStream(&data->demuxer_configs[0], is_audio); On 2014/04/25 23:13:13, wolenetz wrote: > nit: ditto (spelling), here and below. Done. https://codereview.chromium.org/254473010/diff/20001/content/renderer/media/a... File content/renderer/media/android/media_source_delegate.h (right): https://codereview.chromium.org/254473010/diff/20001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.h:181: void GetDeumuxerConfigFromStream( On 2014/04/25 23:13:13, wolenetz wrote: > nit: s/Deumuxer/Demuxer Done. https://codereview.chromium.org/254473010/diff/20001/media/base/android/audio... File media/base/android/audio_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/20001/media/base/android/audio... media/base/android/audio_decoder_job.h:57: std::vector<uint8> audio_extra_data_; On 2014/04/25 23:13:13, wolenetz wrote: > lint nit: Add #include for vector<> Done. https://codereview.chromium.org/254473010/diff/20001/media/base/android/demux... File media/base/android/demuxer_stream_player_params.h (right): https://codereview.chromium.org/254473010/diff/20001/media/base/android/demux... media/base/android/demuxer_stream_player_params.h:58: // a corresponding DemuxerConfigs is added into this vector. This solves the On 2014/04/25 23:13:13, wolenetz wrote: > nit: mention that only the type-specific portion of DemuxerConfigs is populated? Done. https://codereview.chromium.org/254473010/diff/20001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/254473010/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:655: while(demuxer_->num_data_requests() == expected_num_data_requests) On 2014/04/25 23:13:13, wolenetz wrote: > lint nit: Missing space before ( in while( Done. https://codereview.chromium.org/254473010/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1638: while(IsDrainingDecoder(true) || IsDrainingDecoder(false)) On 2014/04/25 23:13:13, wolenetz wrote: > lint nit: Missing space before ( in while( Done.
Some further comments around the ConfigChange IPC change (part 2 of the currenty CL description). I really think that piece of this CL should be split off and landed first, separate from the refactoring of MSP/MDJ, if possible. WDYT? (I have not yet reviewed parts 1 & 3 of this change as defined in the current CL description.) https://codereview.chromium.org/254473010/diff/40001/content/renderer/media/a... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/254473010/diff/40001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.cc:395: if (!is_audio) { Does it make sense that we (before and now) allow kConfigChanged when the corresponding stream is still NULL here? Could this lead to undefined behavior when the browser code sees kConfigChanged but no correspondingly filled-in DemuxerConfig for the stream type? Also, in the previous code we protected against sending configs when requested by CanNotifyDemuxer(). ISTM like we should at least retain that behavior somehow. https://codereview.chromium.org/254473010/diff/40001/content/renderer/media/a... File content/renderer/media/android/media_source_delegate.h (right): https://codereview.chromium.org/254473010/diff/40001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.h:181: void GetDemuxerConfigFromStream( nit: use consistent style (don't wrap prior to first param since it fits on the first line).
Ok, split ConfigChange IPC into a separate CL: https://codereview.chromium.org/257323003/ https://codereview.chromium.org/254473010/diff/40001/content/renderer/media/a... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/254473010/diff/40001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.cc:395: if (!is_audio) { If stream is NULL, GetDemuxerConfigFromStream() will append an empty demuxer config to the IPC. Since there is no codec information in that IPC, MSP will simply set HasVideo() or HasAudio() to false. Which is the correct behavior. added CanNotifyDemuxerReady() in GetDemuxerConfigFromStream(). On 2014/04/28 19:48:02, wolenetz wrote: > Does it make sense that we (before and now) allow kConfigChanged when the > corresponding stream is still NULL here? Could this lead to undefined behavior > when the browser code sees kConfigChanged but no correspondingly filled-in > DemuxerConfig for the stream type? > Also, in the previous code we protected against sending configs when requested > by CanNotifyDemuxer(). ISTM like we should at least retain that behavior > somehow. https://codereview.chromium.org/254473010/diff/40001/content/renderer/media/a... File content/renderer/media/android/media_source_delegate.h (right): https://codereview.chromium.org/254473010/diff/40001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.h:181: void GetDemuxerConfigFromStream( On 2014/04/28 19:48:02, wolenetz wrote: > nit: use consistent style (don't wrap prior to first param since it fits on the > first line). Done.
Thank you for splitting out the ConfigChange IPC piece. Please rebase this onto that other CL and reword this CL's description accordingly to assist code reviewing. Please also consider any further reasonable possibilities of splitting off CLs from this to make reviews (and landings, and identifying any needed reverts) more granular, to get more of this overall CL landed faster and more reliably.
On 2014/05/02 22:31:00, wolenetz wrote: > Thank you for splitting out the ConfigChange IPC piece. > Please rebase this onto that other CL and reword this CL's description > accordingly to assist code reviewing. > Please also consider any further reasonable possibilities of splitting off CLs > from this to make reviews (and landings, and identifying any needed reverts) > more granular, to get more of this overall CL landed faster and more reliably. Ya, i also want to keep the CLs small. However, the issue is mostly related to the tests. Each time we modify some logic, we need to change all 60 unit tests. And the expected test result may be irrelavant to be previous CL, which makes intermediate test changes useless.
On 2014/05/02 22:41:13, qinmin wrote: > On 2014/05/02 22:31:00, wolenetz wrote: > > Thank you for splitting out the ConfigChange IPC piece. > > Please rebase this onto that other CL and reword this CL's description > > accordingly to assist code reviewing. > > Please also consider any further reasonable possibilities of splitting off CLs > > from this to make reviews (and landings, and identifying any needed reverts) > > more granular, to get more of this overall CL landed faster and more reliably. > > Ya, i also want to keep the CLs small. However, the issue is mostly related to > the tests. > Each time we modify some logic, we need to change all 60 unit tests. And the > expected test result may be irrelavant to be previous CL, which makes > intermediate test changes useless. That makes sense - I understand test churn++ as CL is broken into smaller pieces. Please rebase and reword description and then I'll take a look at this one. I assume the configchange IPC CL is expected to land as a prereq to the rest of this CL, right?
On 2014/05/02 22:47:19, wolenetz wrote: > On 2014/05/02 22:41:13, qinmin wrote: > > On 2014/05/02 22:31:00, wolenetz wrote: > > > Thank you for splitting out the ConfigChange IPC piece. > > > Please rebase this onto that other CL and reword this CL's description > > > accordingly to assist code reviewing. > > > Please also consider any further reasonable possibilities of splitting off > CLs > > > from this to make reviews (and landings, and identifying any needed reverts) > > > more granular, to get more of this overall CL landed faster and more > reliably. > > > > Ya, i also want to keep the CLs small. However, the issue is mostly related to > > the tests. > > Each time we modify some logic, we need to change all 60 unit tests. And the > > expected test result may be irrelavant to be previous CL, which makes > > intermediate test changes useless. > > That makes sense - I understand test churn++ as CL is broken into smaller > pieces. Please rebase and reword description and then I'll take a look at this > one. I assume the configchange IPC CL is expected to land as a prereq to the > rest of this CL, right? Yes, that CL should land before this one.
PTAL again, rebased the change after recent DemuxerConfig IPC change.
On 2014/05/13 22:13:54, qinmin wrote: > PTAL again, rebased the change after recent DemuxerConfig IPC change. Sorry for CR delay - I'll get to this tomorrow if not tonight.
On 2014/05/16 00:00:37, wolenetz wrote: > On 2014/05/13 22:13:54, qinmin wrote: > > PTAL again, rebased the change after recent DemuxerConfig IPC change. > > Sorry for CR delay - I'll get to this tomorrow if not tonight. This one's fairly big. I'll continue CR Monday. Thank you for your patience.
On 2014/05/17 02:24:34, wolenetz wrote: > On 2014/05/16 00:00:37, wolenetz wrote: > > On 2014/05/13 22:13:54, qinmin wrote: > > > PTAL again, rebased the change after recent DemuxerConfig IPC change. > > > > Sorry for CR delay - I'll get to this tomorrow if not tonight. > > This one's fairly big. I'll continue CR Monday. Thank you for your patience. Take your time. Thanks matt
First round of comments (on PS4, excluding the unittests in this CR round): https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.cc:539: if (release_resources_pending_) { In this case (release_resources_pending_ was true), do we still want to retain |status| as-is, or change it to OK? https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.cc:543: OnDecoderDrained(); nit: CurrentDataConsumed(...) is called twice if line 535 and 543 are reached in one path through this method. Would be good to either call out in the method docs that this is fine, or fix this method to only call it max once per each OnDecodeCompleted(). https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.h:62: // Returns false if |media_codec_bridge_| cannot be created. |callback| is nit: s/./;/ https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.h:89: virtual bool SetDemuxerConfigs(const DemuxerConfigs& configs); Does this need to be virtual? Neither {audio,video}_decoder_job.h have this. https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.h:167: // Helper function to decoder data on |thread_|. |unit| contains all the data nit: s/decoder/decode/ && s/thread_/decoder_task_runner_ (?) https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.h:172: // flushed at the beginning of this call. nit: comment drain_decoder to help clarify difference between flush and drain (abandon current data == flush, decode EOS if drain_decoder). nit: please also comment w.r.t: Can both needs_flush and drain_decoder be true? What would expected behavior be? aside: Is it legal for |stop_decode_pending_| or |release_resources_pending_| to be true during DecodeInternal? What would expected behavior be? https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.h:209: // Creates |media_codec_bridge_| for decoding purpose. nit: doc retval https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.h:221: // particular stream. nit: doc retval https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_source_player.cc:333: clock_.SetTime(seek_time, seek_time); Why do we no longer need to tell audio_decoder_job_ the actual browser-seeked-to-time (which could be different than what we requested to seek to, and different than what we previously told the job to set base timestamp to in ProcessPendingEvents for SEEK_EVENT_PENDING)? https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_source_player.cc:565: if (!video_decoder_job_->next_video_data_is_iframe()) { aside: I like this simplification :) https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... File media/base/android/media_source_player.h (right): https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_source_player.h:170: // TODO(qinmin/wolenetz): Reorder these based on their priority from nit: Is this TODO now done? https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_source_player.h:245: // TODO(qinmin): currently only |is_waiting_for_audio_decoder_| is used due to s/audio/video ? If not, I'm really confused. https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_source_player.h:246: // empty surface. Will remove is_|waiting_for_audio_decoder_| if needed. nit: s/is_|/|is_/ ? && s/if needed/if not needed/ And since it seems not needed, remove it now and drop the TODO? https://codereview.chromium.org/254473010/diff/100001/media/base/android/vide... File media/base/android/video_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/100001/media/base/android/vide... media/base/android/video_decoder_job.cc:47: // For an empty surface, always pass it to the decoder job so that it nit: pass to decoder job? we are the decoder job. I'm confused. https://codereview.chromium.org/254473010/diff/100001/media/base/android/vide... File media/base/android/video_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/100001/media/base/android/vide... media/base/android/video_decoder_job.h:32: // Passing an java surface object to the codec. nit: s/Passing an/Passes/ https://codereview.chromium.org/254473010/diff/100001/media/base/android/vide... media/base/android/video_decoder_job.h:33: bool SetVideoSurface(gfx::ScopedJavaSurface surface); nit: doc retval
https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.cc:539: if (release_resources_pending_) { We should leave it as it is. For example, if |status| is equal to |MEDIA_CODEC_OUTPUT_END_OF_STREAM|, then changing the |status| to OK will let MSP think that video still hasn't finished. On 2014/05/21 00:48:04, wolenetz wrote: > In this case (release_resources_pending_ was true), do we still want to retain > |status| as-is, or change it to OK? https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.cc:543: OnDecoderDrained(); OnDecoderDrained（） will set drain_decoder_ to false. So if line 535 is reached, we will not reach line 543. On 2014/05/21 00:48:04, wolenetz wrote: > nit: CurrentDataConsumed(...) is called twice if line 535 and 543 are reached in > one path through this method. Would be good to either call out in the method > docs that this is fine, or fix this method to only call it max once per each > OnDecodeCompleted(). https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.h:62: // Returns false if |media_codec_bridge_| cannot be created. |callback| is On 2014/05/21 00:48:04, wolenetz wrote: > nit: s/./;/ Done. https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.h:89: virtual bool SetDemuxerConfigs(const DemuxerConfigs& configs); No, removed the virtual keyword. On 2014/05/21 00:48:04, wolenetz wrote: > Does this need to be virtual? Neither {audio,video}_decoder_job.h have this. https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.h:167: // Helper function to decoder data on |thread_|. |unit| contains all the data On 2014/05/21 00:48:04, wolenetz wrote: > nit: s/decoder/decode/ && s/thread_/decoder_task_runner_ (?) Done. https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.h:172: // flushed at the beginning of this call. added comments here. And added a DCHECK in function definition to verify that needs_flush and drain_decoder will not both be true. On 2014/05/21 00:48:04, wolenetz wrote: > nit: comment drain_decoder to help clarify difference between flush and drain > (abandon current data == flush, decode EOS if drain_decoder). > > nit: please also comment w.r.t: Can both needs_flush and drain_decoder be true? > What would expected behavior be? > > aside: Is it legal for |stop_decode_pending_| or |release_resources_pending_| to > be true during DecodeInternal? What would expected behavior be? https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.h:209: // Creates |media_codec_bridge_| for decoding purpose. On 2014/05/21 00:48:04, wolenetz wrote: > nit: doc retval Done. https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.h:221: // particular stream. On 2014/05/21 00:48:04, wolenetz wrote: > nit: doc retval Done. https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_source_player.cc:333: clock_.SetTime(seek_time, seek_time); you are right, adding it back. previously I thought the actual_browser_seek_time would be identical to the seek_time we passed to ProcessPendingEvents。 On 2014/05/21 00:48:04, wolenetz wrote: > Why do we no longer need to tell audio_decoder_job_ the actual > browser-seeked-to-time (which could be different than what we requested to seek > to, and different than what we previously told the job to set base timestamp to > in ProcessPendingEvents for SEEK_EVENT_PENDING)? https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... File media/base/android/media_source_player.h (right): https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_source_player.h:170: // TODO(qinmin/wolenetz): Reorder these based on their priority from Done. Reordered these events according to their priorities. On 2014/05/21 00:48:04, wolenetz wrote: > nit: Is this TODO now done? https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_source_player.h:245: // TODO(qinmin): currently only |is_waiting_for_audio_decoder_| is used due to nice catch, should be video. Removed is_waiting_for_audio_decoder_. On 2014/05/21 00:48:04, wolenetz wrote: > s/audio/video ? If not, I'm really confused. https://codereview.chromium.org/254473010/diff/100001/media/base/android/medi... media/base/android/media_source_player.h:246: // empty surface. Will remove is_|waiting_for_audio_decoder_| if needed. On 2014/05/21 00:48:04, wolenetz wrote: > nit: s/is_|/|is_/ ? && s/if needed/if not needed/ > And since it seems not needed, remove it now and drop the TODO? Done. https://codereview.chromium.org/254473010/diff/100001/media/base/android/vide... File media/base/android/video_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/100001/media/base/android/vide... media/base/android/video_decoder_job.cc:47: // For an empty surface, always pass it to the decoder job so that it Fixed, should be |media_codec_bridge_|. On 2014/05/21 00:48:04, wolenetz wrote: > nit: pass to decoder job? we are the decoder job. I'm confused. https://codereview.chromium.org/254473010/diff/100001/media/base/android/vide... File media/base/android/video_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/100001/media/base/android/vide... media/base/android/video_decoder_job.h:32: // Passing an java surface object to the codec. On 2014/05/21 00:48:04, wolenetz wrote: > nit: s/Passing an/Passes/ Done. https://codereview.chromium.org/254473010/diff/100001/media/base/android/vide... media/base/android/video_decoder_job.h:33: bool SetVideoSurface(gfx::ScopedJavaSurface surface); On 2014/05/21 00:48:04, wolenetz wrote: > nit: doc retval Done.
ping， Matt, any further comments on this change?
On 2014/05/27 16:13:47, qinmin wrote: > ping， Matt, any further comments on this change? I'll have a look today. Thanks, Matt
looking pretty good. Some comments, plus I will need to take a closer look at the unit tests once I'm more clear on this. I recommend you add xhwang@ due to his recent work on pipeline unification as well as the Cdm/key related code relocations. https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... media/base/android/media_decoder_job.cc:390: if (unit.status == DemuxerStream::kAborted) { If |drain_decoder| is true, and unit.status is kAborted, does drain logic need to precede possible kAborted no-op? https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... media/base/android/media_source_player.cc:389: if (IsEventPending(DECODER_CREATION_EVENT_PENDING)) { nit: this pending event is now specific to video decoder creation. Rename it? https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... media/base/android/media_source_player.cc:545: manager()->OnError(player_id(), MEDIA_ERROR_DECODE); nit: Is this line worthy of being in a helper method now that it is done in at least two places? https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... File media/base/android/media_source_player.h (right): https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... media/base/android/media_source_player.h:178: static const char* GetEventName(PendingEventFlags event); nit: It looks like GetEventName()'s |kPendingEventNames| could get out of sync w/enum PendingEventFlags. Please add comment at each place to help keep them in-sync. https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:229: EXPECT_FALSE(player_.IsPlaying()); nit: also expect_true(getmediadecoderjob(each audio, video))? In general, what is the reasoning behind choice of GetMediaCodecBridge(...) vs GetMediaDecoderJob(...) in these tests? Is there some simple guidance that could be put in comments for those two methods? https://codereview.chromium.org/254473010/diff/120001/media/base/android/vide... File media/base/android/video_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/120001/media/base/android/vide... media/base/android/video_decoder_job.h:32: // Passes an java surface object to the codec. Returns true if the surface nit: s/an//
+xhwang https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... media/base/android/media_decoder_job.cc:390: if (unit.status == DemuxerStream::kAborted) { decoder draining only happens when config changes, and we don't process any new AUs during this process. so if drain_decoder is true, unit.status should be equal to kConfigChanged and should not be kAborted. On 2014/05/27 22:21:13, wolenetz wrote: > If |drain_decoder| is true, and unit.status is kAborted, does drain logic need > to precede possible kAborted no-op? https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... media/base/android/media_source_player.cc:389: if (IsEventPending(DECODER_CREATION_EVENT_PENDING)) { On 2014/05/27 22:21:13, wolenetz wrote: > nit: this pending event is now specific to video decoder creation. Rename it? Done. https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... media/base/android/media_source_player.cc:545: manager()->OnError(player_id(), MEDIA_ERROR_DECODE); On 2014/05/27 22:21:13, wolenetz wrote: > nit: Is this line worthy of being in a helper method now that it is done in at > least two places? Done. https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... File media/base/android/media_source_player.h (right): https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... media/base/android/media_source_player.h:178: static const char* GetEventName(PendingEventFlags event); On 2014/05/27 22:21:13, wolenetz wrote: > nit: It looks like GetEventName()'s |kPendingEventNames| could get out of sync > w/enum PendingEventFlags. Please add comment at each place to help keep them > in-sync. Done. https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/254473010/diff/120001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:229: EXPECT_FALSE(player_.IsPlaying()); MediaDecoderJob was created in MSP ctor, so it should always be non-null and calling EXPECT_TRUE() on GetMediaDecoderJob() is pointless. GetMediaCodecBridge(), on the other hand, could return null if the respective stream is not available or if decoder is not ready. So calling EXPECT_TRUE() is fine. added comments for these 2 functions. On 2014/05/27 22:21:13, wolenetz wrote: > nit: also expect_true(getmediadecoderjob(each audio, video))? > In general, what is the reasoning behind choice of GetMediaCodecBridge(...) vs > GetMediaDecoderJob(...) in these tests? Is there some simple guidance that could > be put in comments for those two methods? https://codereview.chromium.org/254473010/diff/120001/media/base/android/vide... File media/base/android/video_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/120001/media/base/android/vide... media/base/android/video_decoder_job.h:32: // Passes an java surface object to the codec. Returns true if the surface On 2014/05/27 22:21:13, wolenetz wrote: > nit: s/an// Done.
I have to run for now. This is my first round of comments. I suspect this will break some EME cases, but I have to dig more to really understand how it works now. https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.cc:101: set_is_content_encrypted(configs.is_audio_encrypted); Why |is_audio_encrypted| is special? Actually, can we just store a DemuxerConfigs in this class, instead all of it's members? See https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_d... hmm, I see. DemuxerConfigs contains both audio and video configs... This is a bit confusing. We probably should clean this up in a separate CL. https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.cc:115: configs.audio_extra_data.begin()); If we separate DemuxerConfigs into AudioConfig and VideoConfig, we can just have a Matches() helper function to do this, similar to: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_d... https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... File media/base/android/audio_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.h:25: AudioDecoderJob(const base::Closure& request_data_cb, Add include for base::Closure. https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.h:27: ~AudioDecoderJob(); virtual https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.h:28: // MediaDecoderJob implementation. https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.h:31: // Set the volume of the audio output. nit: Set_s_ https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.cc:626: return false; Will this trigger decode error? https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.h:153: MediaCodecStatus QueueInputBuffer(const AccessUnit& unit, bool drain_decoder); AccessUnit has "bool end_of_stream". Can we just use an EOS unit? https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.h:168: // all the data to be decoded. |start_time_ticks| and One AccessUnit is just one buffer to be decoded. "all the data" here is a bit misleading. Can we drop the "all"? https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.h:173: // this call. If |drain_decoder| is true, an EOS frame will be fed to the ditto. AccessUnit has "bool end_of_stream". Can't you just pass in a EOS AccessUnit for draining the decoder? https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.h:235: virtual bool IsDemuxerConfigChanged(const DemuxerConfigs& configs) const = 0; ditto, we are mixing "Configs" and "Config". Let's make them consistent. https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.h:284: base::Closure on_demuxer_config_changed_cb_; usually we use OnFoo() as the name of a function that serves as a callback. But we don't use "on" on callback names. How about just "config_changed_cb_"?
https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.cc:101: set_is_content_encrypted(configs.is_audio_encrypted); is_content_encrypted_ is private in base class, so a set_is_content_encrypted_() is needed. Other variables are not inherited. ok, added a TODO here to split audio and video configs into 2 stuctures. On 2014/05/30 21:58:07, xhwang wrote: > Why |is_audio_encrypted| is special? > > Actually, can we just store a DemuxerConfigs in this class, instead all of it's > members? See > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_d... > > hmm, I see. DemuxerConfigs contains both audio and video configs... This is a > bit confusing. We probably should clean this up in a separate CL. https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... File media/base/android/audio_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.h:25: AudioDecoderJob(const base::Closure& request_data_cb, On 2014/05/30 21:58:07, xhwang wrote: > Add include for base::Closure. Done. https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.h:27: ~AudioDecoderJob(); On 2014/05/30 21:58:07, xhwang wrote: > virtual Done. https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.h:28: On 2014/05/30 21:58:07, xhwang wrote: > // MediaDecoderJob implementation. Done. https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.h:31: // Set the volume of the audio output. this is not a simple setter, it calls another function. On 2014/05/30 21:58:07, xhwang wrote: > nit: Set_s_ https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.cc:626: return false; On 2014/05/30 21:58:07, xhwang wrote: > Will this trigger decode error? for video we won't. Decoding will stall indefinitely until a MediaCodecBridge can be successfully created. For audio, yes, this will trigger an decode error being sent to the WMPA https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.h:153: MediaCodecStatus QueueInputBuffer(const AccessUnit& unit, bool drain_decoder); We might still need to differentiate between real EOS unit and whether the unit is purely used for draining the MediaCodec later. On 2014/05/30 21:58:07, xhwang wrote: > AccessUnit has "bool end_of_stream". Can we just use an EOS unit? https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.h:168: // all the data to be decoded. |start_time_ticks| and On 2014/05/30 21:58:07, xhwang wrote: > One AccessUnit is just one buffer to be decoded. "all the data" here is a bit > misleading. Can we drop the "all"? Done. https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.h:173: // this call. If |drain_decoder| is true, an EOS frame will be fed to the same reason above, we want to differentiate between real EOS unit and draining, that affects the decode status we pass to OnDecodeCompleted(). On 2014/05/30 21:58:07, xhwang wrote: > ditto. AccessUnit has "bool end_of_stream". Can't you just pass in a EOS > AccessUnit for draining the decoder? https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.h:235: virtual bool IsDemuxerConfigChanged(const DemuxerConfigs& configs) const = 0; On 2014/05/30 21:58:07, xhwang wrote: > ditto, we are mixing "Configs" and "Config". Let's make them consistent. Done. https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.h:284: base::Closure on_demuxer_config_changed_cb_; On 2014/05/30 21:58:07, xhwang wrote: > usually we use OnFoo() as the name of a function that serves as a callback. But > we don't use "on" on callback names. How about just "config_changed_cb_"? Done.
wolenetz@, xhwang@, PTAL again. MediaDecoderJob::Decode() could fail due to delayed AddKey() call for EME, and that applies to both audio and video. As a result, I am bringing back the is_waiting_for_audio_ variable for MediaSourcePlayer. When SetCdm() is called, we pass the MediaDrmBridge to MediaDecoderJob, and request the MediaCodecBridge to be recreated once MediaCrypto becomes ready. Tested xhwang's EME video http://goo.gl/8ceCVj, and it should work fine now.
EME part looks good! I also tested some EME test pages and they work fine. Just one more comment about EOS and draining decoder. https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... File media/base/android/audio_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.h:31: // Set the volume of the audio output. On 2014/05/30 23:43:33, qinmin wrote: > this is not a simple setter, it calls another function. > > On 2014/05/30 21:58:07, xhwang wrote: > > nit: Set_s_ > Sorry for not being clear. I was simply suggesting s/Set/Sets in the comment. https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.cc:626: return false; On 2014/05/30 23:43:33, qinmin wrote: > On 2014/05/30 21:58:07, xhwang wrote: > > Will this trigger decode error? > > for video we won't. Decoding will stall indefinitely until a MediaCodecBridge > can be successfully created. > For audio, yes, this will trigger an decode error being sent to the WMPA > I see. So the latest PS will make it consistent for audio and video? https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.h:153: MediaCodecStatus QueueInputBuffer(const AccessUnit& unit, bool drain_decoder); On 2014/05/30 23:43:33, qinmin wrote: > We might still need to differentiate between real EOS unit and whether the unit > is purely used for draining the MediaCodec later. > > On 2014/05/30 21:58:07, xhwang wrote: > > AccessUnit has "bool end_of_stream". Can we just use an EOS unit? > Hmm, what's the difference? On desktop, we use the same EOS for flushing the codec during config change and at the end of playback. What's the specific reason we can't do the same here? I am just trying to see whether we can simplify the code here.
https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... File media/base/android/audio_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.h:31: // Set the volume of the audio output. On 2014/06/02 23:07:11, xhwang wrote: > On 2014/05/30 23:43:33, qinmin wrote: > > this is not a simple setter, it calls another function. > > > > On 2014/05/30 21:58:07, xhwang wrote: > > > nit: Set_s_ > > > > Sorry for not being clear. I was simply suggesting s/Set/Sets in the comment. Done. https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.cc:626: return false; Yes, no error will be thrown for both audio and video now. So MSP will wait for the the MediaCrypto to become ready. On 2014/06/02 23:07:11, xhwang wrote: > On 2014/05/30 23:43:33, qinmin wrote: > > On 2014/05/30 21:58:07, xhwang wrote: > > > Will this trigger decode error? > > > > for video we won't. Decoding will stall indefinitely until a MediaCodecBridge > > can be successfully created. > > For audio, yes, this will trigger an decode error being sent to the WMPA > > > > I see. So the latest PS will make it consistent for audio and video? https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/254473010/diff/140001/media/base/android/medi... media/base/android/media_decoder_job.h:153: MediaCodecStatus QueueInputBuffer(const AccessUnit& unit, bool drain_decoder); I thought the return status to OnDecodeCompleted() matters (MEDIA_CODEC_OK vs Media_CODEC_INPUT_END_OF_STREAM). But seems that's not the case. Actually there is a bug in MSP that we are not updating the timestamp if the return status is Media_CODEC_INPUT_END_OF_STREAM. The new PS should fix the bug and passing an pre-created EOS access unit to the MediaCodec. On 2014/06/02 23:07:11, xhwang wrote: > On 2014/05/30 23:43:33, qinmin wrote: > > We might still need to differentiate between real EOS unit and whether the > unit > > is purely used for draining the MediaCodec later. > > > > On 2014/05/30 21:58:07, xhwang wrote: > > > AccessUnit has "bool end_of_stream". Can we just use an EOS unit? > > > > Hmm, what's the difference? On desktop, we use the same EOS for flushing the > codec during config change and at the end of playback. What's the specific > reason we can't do the same here? I am just trying to see whether we can > simplify the code here.
Looking pretty good. This time, my comments include CR of the unit tests. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_decoder_job.cc:356: decoder_task_runner_->PostTask(FROM_HERE, base::Bind( nit: Before posting task, DCHECK(!(needs_flush_ && drain_decoder_)), since DecodeInternal no longer has clear indication of EOS vs drain_decoder_ and no longer can do that DCHECK? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:333: int expected_num_requests = (player_.HasAudio() ? 1 : 0) + nit: Do all calls to this helper supply valid configs? If so, maybe we shouldn't trust |player_| Has*() implementation, and should check them in this helper versus the supplied configs? Something like EXPECT_TRUE(player_.Has[Audio/Video]() == (configs.[video/audio]_codec is a known codec)? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:355: EXPECT_EQ(expect_player_requests_audio_data, These are no-ops now. Remove? Can we deterministically expect that the decoder bridges are created based on some input here? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:522: EXPECT_TRUE(GetMediaDecoderJob(is_audio)->is_decoding()); nit: add some verification that media codec bridge is alive? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:598: // Wait for the decoder job to finish decoding and be reset pending the nit: here and multiple other places in this file, the decoder job is not 'reset' any longer. Perhaps s/decoder job/media codec bridge/? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:671: // Run until decoder starts to request new data. Should IsDraining(is_audio) be true here, and false after the while loop, below? Seems like a good thing to add checks, if so. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:891: EXPECT_FALSE(GetMediaCodecBridge(false)); nit: We should expect prefetch of 1 read at this point, IIUC. Check this? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:952: player_.OnDemuxerDataAvailable(CreateAbortedAck(false)); Why send kAborted instead of simulating the asynch response to a data request that renderer processed before browser seek commenced? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1161: EXPECT_TRUE(StartTimeTicks() == previous); aside: Does this (and the change below) fix the infrequent flakiness of this test? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1455: // Test that browser seek is requested if player Release() + Start() occurs nit: s/browser seek/no browser seek/ https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1574: // TODO(qinmin): Simulation of multiple in-flight data requests (one from With this change, is it still possible for multiple in-flight data requests as described in the TODO? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1612: configs.audio_sampling_rate = 11025; nit: Add tests that no actual config change occurs if kConfigChanged AU provides unchanged config for each of audio and video? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1720: // video decoder job results without any browser seek. nit: fix comment. no new job is created, just a new bridge. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1723: // New video decoder job should have been created and configured, without any nit: ditto https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1740: player_.OnDemuxerSeekDone(kNoTimestamp()); Just prior to this line, should IsDrainingDecoder(true) be false? Or only sometime later? If the former, please check that before OnDemuxerSeekDone(). https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1852: Why is |decoder_callback_hook_executed_| no longer expected to be true here? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1977: // Start(), then the player starts to decode the new data without any browser nit: s/browser// https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:2104: DemuxerConfigs configs = CreateAudioDemuxerConfigs(kCodecVorbis); nit: Can this 11025 alternate config (and similar for kCodecVP9 alternate config) be abstracted into helpers and the result be simpler?
EME part lgtm. I'll defer to wolenetz@ to finish the review.
nit: Add 306314 to the BUGS line in description.
306314 should have been fixed by https://codereview.chromium.org/196133020 https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_decoder_job.cc:356: decoder_task_runner_->PostTask(FROM_HERE, base::Bind( On 2014/06/04 00:21:46, wolenetz wrote: > nit: Before posting task, DCHECK(!(needs_flush_ && drain_decoder_)), since > DecodeInternal no longer has clear indication of EOS vs drain_decoder_ and no > longer can do that DCHECK? Done. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:333: int expected_num_requests = (player_.HasAudio() ? 1 : 0) + No, StartAudioDecoderWithInvalidConfig test will pass invalid configs to the decoder job. However, HasAudio()/HasVideo() only checks whether the stream has audio/video part. It doesn't have any impact on whether data request are sent. Since the decoder is created lazily when Decode() is called, prefetch() will fetch the data even if the config is invalid. On 2014/06/04 00:21:47, wolenetz wrote: > nit: Do all calls to this helper supply valid configs? If so, maybe we shouldn't > trust |player_| Has*() implementation, and should check them in this helper > versus the supplied configs? Something like > EXPECT_TRUE(player_.Has[Audio/Video]() == (configs.[video/audio]_codec is a > known codec)? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:355: EXPECT_EQ(expect_player_requests_audio_data, Done. Start() won't create the decoder job because it will call Prefetch() first. So at this moment we cannot determine whether the MediaCodecBridges are created. Tests that call this function should check the decoder bridge by themselves On 2014/06/04 00:21:47, wolenetz wrote: > These are no-ops now. Remove? Can we deterministically expect that the decoder > bridges are created based on some input here? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:522: EXPECT_TRUE(GetMediaDecoderJob(is_audio)->is_decoding()); On 2014/06/04 00:21:47, wolenetz wrote: > nit: add some verification that media codec bridge is alive? Done. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:598: // Wait for the decoder job to finish decoding and be reset pending the On 2014/06/04 00:21:46, wolenetz wrote: > nit: here and multiple other places in this file, the decoder job is not 'reset' > any longer. Perhaps s/decoder job/media codec bridge/? Done. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:671: // Run until decoder starts to request new data. it really depends on config_unit_in_prefetch and config_unit_index. If config_unit_in_prefetch is true, then IsDraining() will be false since there is no need to drain the decoder. Otherwise, only if config_unit_index==0, then IsDraining() will be true. added a EXPECT_EQ here and a EXPECT_FALSE(IsDrainging(is_audio)) after the while statement On 2014/06/04 00:21:47, wolenetz wrote: > Should IsDraining(is_audio) be true here, and false after the while loop, below? > Seems like a good thing to add checks, if so. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:891: EXPECT_FALSE(GetMediaCodecBridge(false)); On 2014/06/04 00:21:46, wolenetz wrote: > nit: We should expect prefetch of 1 read at this point, IIUC. Check this? Done. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:952: player_.OnDemuxerDataAvailable(CreateAbortedAck(false)); If i understand correctly, browser seek can result in 2 behaviors: 1. if chunk demuxer is processing a pending read request, it will send abort and then the seek ack 2. Otherwise, chunk demuxer just sends the seek ack So this is behavior 1. Anyway, changed this to send a data response. On 2014/06/04 00:21:47, wolenetz wrote: > Why send kAborted instead of simulating the asynch response to a data request > that renderer processed before browser seek commenced? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1161: EXPECT_TRUE(StartTimeTicks() == previous); I haven't seen this tests failing with this patch, so I think we are ok. The previous flakiness are mostly solved by the introduction of DecodeAudioDataUntilOutputBecomesAvailable() i think On 2014/06/04 00:21:47, wolenetz wrote: > aside: Does this (and the change below) fix the infrequent flakiness of this > test? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1455: // Test that browser seek is requested if player Release() + Start() occurs On 2014/06/04 00:21:46, wolenetz wrote: > nit: s/browser seek/no browser seek/ Done. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1574: // TODO(qinmin): Simulation of multiple in-flight data requests (one from This should have been fixed by the previous async data request patch. Removed the TODO. On 2014/06/04 00:21:47, wolenetz wrote: > With this change, is it still possible for multiple in-flight data requests as > described in the TODO? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1612: configs.audio_sampling_rate = 11025; On 2014/06/04 00:21:46, wolenetz wrote: > nit: Add tests that no actual config change occurs if kConfigChanged AU provides > unchanged config for each of audio and video? Done. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1720: // video decoder job results without any browser seek. On 2014/06/04 00:21:47, wolenetz wrote: > nit: fix comment. no new job is created, just a new bridge. Done. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1723: // New video decoder job should have been created and configured, without any On 2014/06/04 00:21:47, wolenetz wrote: > nit: ditto Done. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1740: player_.OnDemuxerSeekDone(kNoTimestamp()); Done. Yes, WaitForAudioDecodeDone() should make IsDrainingDecoder(true) to become false. On 2014/06/04 00:21:46, wolenetz wrote: > Just prior to this line, should IsDrainingDecoder(true) be false? Or only > sometime later? If the former, please check that before OnDemuxerSeekDone(). https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1852: it should still be true, added the EXPECT_TRUE() back. On 2014/06/04 00:21:46, wolenetz wrote: > Why is |decoder_callback_hook_executed_| no longer expected to be true here? https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:1977: // Start(), then the player starts to decode the new data without any browser On 2014/06/04 00:21:46, wolenetz wrote: > nit: s/browser// Done. https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:2104: DemuxerConfigs configs = CreateAudioDemuxerConfigs(kCodecVorbis); ok, changed the CreateAudioDemuxerConfigs() to accept a bool value for low sampling rate, CreateVideoDemuxerConfigs already accepts a codec param for video. On 2014/06/04 00:21:46, wolenetz wrote: > nit: Can this 11025 alternate config (and similar for kCodecVP9 alternate > config) be abstracted into helpers and the result be simpler?
lgtm % nits: https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/254473010/diff/220001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:952: player_.OnDemuxerDataAvailable(CreateAbortedAck(false)); On 2014/06/04 19:29:15, qinmin wrote: > If i understand correctly, browser seek can result in 2 behaviors: > 1. if chunk demuxer is processing a pending read request, it will send abort and > then the seek ack > 2. Otherwise, chunk demuxer just sends the seek ack > > So this is behavior 1. > Anyway, changed this to send a data response. > > On 2014/06/04 00:21:47, wolenetz wrote: > > Why send kAborted instead of simulating the asynch response to a data request > > that renderer processed before browser seek commenced? > You're correct. Either behavior would have been fine here. Sorry. https://codereview.chromium.org/254473010/diff/240001/media/base/android/medi... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/254473010/diff/240001/media/base/android/medi... media/base/android/media_source_player.cc:291: audio_decoder_job_->SetDrmBridge(drm_bridge_); Should these two be done prior to registering the player, in case OnKeyAdded->StartInternal->Prefetch->etc occur immediately as part of RegisterPlayer()? https://codereview.chromium.org/254473010/diff/240001/media/base/android/medi... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/254473010/diff/240001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:838: // Test audio codec will be created when codec is successfully started. nit: circular / recursive comment :) https://codereview.chromium.org/254473010/diff/240001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:2001: // Start(), then the player starts to decode the new data without any nit: un-wrap to next line.
https://codereview.chromium.org/254473010/diff/240001/media/base/android/medi... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/254473010/diff/240001/media/base/android/medi... media/base/android/media_source_player.cc:291: audio_decoder_job_->SetDrmBridge(drm_bridge_); This won't happen. RegisterPlayer() is a simple function that just records the callbacks. OnKeyAdded() should come as a result of a separate IPC, so it shouldn't interrupt this function. On 2014/06/04 21:42:24, wolenetz wrote: > Should these two be done prior to registering the player, in case > OnKeyAdded->StartInternal->Prefetch->etc occur immediately as part of > RegisterPlayer()? https://codereview.chromium.org/254473010/diff/240001/media/base/android/medi... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/254473010/diff/240001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:838: // Test audio codec will be created when codec is successfully started. fixed On 2014/06/04 21:42:24, wolenetz wrote: > nit: circular / recursive comment :) https://codereview.chromium.org/254473010/diff/240001/media/base/android/medi... media/base/android/media_source_player_unittest.cc:2001: // Start(), then the player starts to decode the new data without any On 2014/06/04 21:42:24, wolenetz wrote: > nit: un-wrap to next line. Done.
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/254473010/260001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/254473010/260001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/254473010/250010
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
Message was sent while issue was closed.
Change committed as 275063 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
