Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1184373005:
MediaCodecPlayer (stage 1 - play/pause only) (Closed)
|
Created:
5 years, 6 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, mlamouri+watch-media_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@mtplayer-decoder Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaCodecPlayer (stage 1 - play/pause only)
This CL implements the basic playback with pause/resume till
completion only (no seek, dynamic configuration, surface change,
encryption).
The MediaCodecPlayer uses one long-lived Media thread for the IPC
with demuxer and for the player's internal operations. The player
controls two MediaCodecDecoder objects (audio and video) that work
with Android MediaCodec.
This CL depends on https://codereview.chromium.org/1176993005/
(the MediaCodecDecoder CL) which should be committed first.
For discussion see https://codereview.chromium.org/1128383003/
BUG=407577
Committed: https://crrev.com/389da2e28a6e675da59bdb5fb492cbba77df9874
Cr-Commit-Position: refs/heads/master@{#336441}
Patch Set 1 #Patch Set 2 : Used TestDataFactory for unit test #
Total comments: 2
Patch Set 3 : Rebased #
Total comments: 15
Patch Set 4 : Rebase with unit test cleanup #Patch Set 5 : Restored state machine diagrams #
Total comments: 4
Patch Set 6 : Addressed Chris comments #
Total comments: 9
Patch Set 7 : Made metadata callback methods private #
Total comments: 7
Patch Set 8 : Addressed Min's comments #
Total comments: 23
Patch Set 9 : Addressed wolenetz@ comments, some questions #
Total comments: 4
Patch Set 10 : Added DCHECK into RUN_ON_MEDIA_THREAD() #Patch Set 11 : Fixed OnVideoSizeChanged() shading #
Messages
Total messages: 35 (6 generated)
timav@chromium.org changed reviewers: + liberato@chromium.org, qinmin@chromium.org, watk@chromium.org, wolenetz@chromium.org
This is the part 3 of the original MediaCodecPlayer CL. It is prepared according to Matt's recommendation, i.e. based on the decoder CL and thus contains only the relevant changes. Compared to the original CL review.https://codereview.chromium.org/1128383003/ I slighly modified the tests. Please review.
https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:107: // Because of that it should go after CreateDecoders(). can you check for this case in OnDemuxerConfigs...()? https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:372: if (state_ != STATE_PREFETCHING) does this actually happen? https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:611: ui_task_runner_->PostTask(FROM_HERE, attach_listener_cb_); does this introduce a race with video_decoder->start()? i don't see what harm it causes, but i mention it anyway. https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.h:26: // The player works as a state machine. Here are relationships between states: this comment would be clearer with a little color and animation. and audio narration. in 7.1. text-only comments in media code don't set the right example. https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.h:154: MetadataChangedCallback; nit: spacing looks weird.
https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:107: // Because of that it should go after CreateDecoders(). On 2015/06/23 14:58:58, liberato wrote: > can you check for this case in OnDemuxerConfigs...()? Added DCHECKs there that audio and video decoders exists. OnDemuxerConfigsAvailable() will try to propagate configs into decoders, so they should exist. I modified the comment as well. https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:372: if (state_ != STATE_PREFETCHING) On 2015/06/23 14:58:59, liberato wrote: > does this actually happen? I think it can happen now with Pause() or Release() racing with OnPrefetchDone(), but maybe I should not post the callback in MediaCodecDecoder::PrefetchNextChunk()? https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:611: ui_task_runner_->PostTask(FROM_HERE, attach_listener_cb_); On 2015/06/23 14:58:59, liberato wrote: > does this introduce a race with video_decoder->start()? i don't see what harm > it causes, but i mention it anyway. The attach and detach listener callbacks are only related to audio, they request and abandon audio focus. As far as I can see the race exists with the audio, too: audio can start before the focus changes. I think both races are acceptable though. https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.h:26: // The player works as a state machine. Here are relationships between states: On 2015/06/23 14:58:59, liberato wrote: > this comment would be clearer with a little color and animation. and audio > narration. in 7.1. text-only comments in media code don't set the right > example. I take this as a subtle hint that these diagrams are better be removed. Well, it is definitely easier than was adding them. Removed most of it. https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.h:154: MetadataChangedCallback; On 2015/06/23 14:58:59, liberato wrote: > nit: spacing looks weird. Removed parameter names.
https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.h:26: // The player works as a state machine. Here are relationships between states: On 2015/06/24 02:31:07, Tima Vaisburd wrote: > On 2015/06/23 14:58:59, liberato wrote: > > this comment would be clearer with a little color and animation. and audio > > narration. in 7.1. text-only comments in media code don't set the right > > example. > > I take this as a subtle hint that these diagrams are better be removed. Well, it > is definitely easier than was adding them. Removed most of it. no! they're great. please git reflog them back! i just felt the need to comment on them, kind of like commenting on mt. rushmore -- you just have to say _something_. i can see them being a maintenance problem (nobody is going to keep them up to date), but i'd rather deal with that if it actually arises. i'm sure there are more compact ways to portray it, but that's a problem we should be glad to have more often. sorry about the confusion.
On 2015/06/24 06:17:57, liberato wrote: No worries. I'll restore them tomorrow.
On 2015/06/24 06:28:15, Tima Vaisburd wrote: > On 2015/06/24 06:17:57, liberato wrote: > > No worries. I'll restore them tomorrow. Restored.
I only had comments on comments :P lgtm otherwise https://codereview.chromium.org/1184373005/diff/20001/media/base/android/medi... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/20001/media/base/android/medi... media/base/android/media_codec_player.h:222: // Cached values for the manager, accessed on the UI thread. Remove "accessed on the UI thread"? https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.h:49: /* Should this block comment be turned into // line comments for consistency with above? https://codereview.chromium.org/1184373005/diff/80001/media/base/android/medi... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/80001/media/base/android/medi... media/base/android/media_codec_player.cc:724: default: If you remove the default case you should get a compiler warning if you add a new state, so you can't forget to update this switch. https://codereview.chromium.org/1184373005/diff/80001/media/base/android/medi... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/80001/media/base/android/medi... media/base/android/media_codec_player.h:288: // Error callback might be posted internally Should end with a period. I'm also not quite sure what it means by "posted internally". Perhaps you could clarify.
https://codereview.chromium.org/1184373005/diff/20001/media/base/android/medi... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/20001/media/base/android/medi... media/base/android/media_codec_player.h:222: // Cached values for the manager, accessed on the UI thread. On 2015/06/24 22:36:07, watk wrote: > Remove "accessed on the UI thread"? Done. https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.h:49: /* On 2015/06/24 22:36:07, watk wrote: > Should this block comment be turned into // line comments for consistency with > above? Done. https://codereview.chromium.org/1184373005/diff/80001/media/base/android/medi... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/80001/media/base/android/medi... media/base/android/media_codec_player.cc:724: default: On 2015/06/24 22:36:07, watk wrote: > If you remove the default case you should get a compiler warning if you add a > new state, so you can't forget to update this switch. Done. https://codereview.chromium.org/1184373005/diff/80001/media/base/android/medi... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/80001/media/base/android/medi... media/base/android/media_codec_player.h:288: // Error callback might be posted internally On 2015/06/24 22:36:07, watk wrote: > Should end with a period. I'm also not quite sure what it means by "posted > internally". Perhaps you could clarify. Added more explanation.
https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... File media/base/android/media_codec_player.cc (left): https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... media/base/android/media_codec_player.cc:55: weak_factory_(this) { do we need this? WeakPtrForUIThread() already returns a weak ptr on UI thread. https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... media/base/android/media_codec_player.h:198: void OnMediaMetadataChanged(base::TimeDelta duration, Why are these 2 method protected instead of private? https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... media/base/android/media_codec_player.h:213: STATE_WAITING_FOR_SURFACE, will PlayerState be a combination of these? Considering that a seek can happen while player is waiting for surface. In that case, we should call this flag instead of enum. https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... media/base/android/media_codec_player.h:222: MediaMetadata() : duration(), video_size(0, 0) {} no need for the initializers, they are the same as the default ctor of the variables
https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... File media/base/android/media_codec_player.cc (left): https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... media/base/android/media_codec_player.cc:55: weak_factory_(this) { On 2015/06/25 18:14:13, qinmin wrote: > do we need this? WeakPtrForUIThread() already returns a weak ptr on UI thread. This weak_factory_ produces weak_this_ that is only used on Media thread. I think we do? https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... media/base/android/media_codec_player.h:198: void OnMediaMetadataChanged(base::TimeDelta duration, On 2015/06/25 18:14:13, qinmin wrote: > Why are these 2 method protected instead of private? I do not know how did they end up in the protected section, thank you for spotting this. They are inherited from public MediaPlayerAndroid methods and I think I wanted them to be public because of that. I put them now in the private section and it works, but is it ok to change the scope in the derived class? https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... media/base/android/media_codec_player.h:213: STATE_WAITING_FOR_SURFACE, On 2015/06/25 18:14:13, qinmin wrote: > will PlayerState be a combination of these? Considering that a seek can happen > while player is waiting for surface. In that case, we should call this flag > instead of enum. I prefer to keep WAITING_FOR_SURFACE as a state because it follows the logic I have everywhere else: if we came to this state we cannot continue and need to get event "got surface". If we are waiting for surface and the seek request comes, we need to move to the state WAITING_FOR_SEEK and request a seek from demuxer (as far as I get). Does it sound reasonable? You could ask why to have WAITING_FOR_SEEK here at all since it belongs to the next CL. https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... media/base/android/media_codec_player.h:222: MediaMetadata() : duration(), video_size(0, 0) {} On 2015/06/25 18:14:13, qinmin wrote: > no need for the initializers, they are the same as the default ctor of the > variables Removed ctor and the default will do the right thing.
https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... File media/base/android/media_codec_player.cc (left): https://codereview.chromium.org/1184373005/diff/100001/media/base/android/med... media/base/android/media_codec_player.cc:55: weak_factory_(this) { On 2015/06/25 19:12:34, Tima Vaisburd wrote: > On 2015/06/25 18:14:13, qinmin wrote: > > do we need this? WeakPtrForUIThread() already returns a weak ptr on UI thread. > > This weak_factory_ produces weak_this_ that is only used on Media thread. I > think we do? Isn't MediaCodecPlayer ctor called on the UI thread? and so the weak factory is now created on the UI thread, and so do the weak_this_ variable.
lgtm % comments https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... media/base/android/media_codec_player.cc:95: ReleaseDecoderResources(); nit: not needed, MediaCodecDecoder dtor will automatically call this for you https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... media/base/android/media_codec_player.cc:689: MediaCodecDecoder::SetTimeCallback(), This is strange, why passing a variable that is member of MediaCodecDecoder to its child class? Cannot the child class get this by itself? https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... media/base/android/media_codec_player.h:311: base::WeakPtrFactory<MediaCodecPlayer> weak_factory_; I would this this to media_weak_factory_ to avoid confuse reviewers, same for the weak_this_
lgtm thanks for clarifying -fl https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.cc:372: if (state_ != STATE_PREFETCHING) On 2015/06/24 02:31:07, Tima Vaisburd wrote: > On 2015/06/23 14:58:59, liberato wrote: > > does this actually happen? > > I think it can happen now with Pause() or Release() racing with > OnPrefetchDone(), but maybe I should not post the callback in > MediaCodecDecoder::PrefetchNextChunk()? i'm only worried about it missing the state transition to WAITING_FOR_SURFACE. but it looks like it always goes through prefetch, so it's okay.
https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... media/base/android/media_codec_player.cc:95: ReleaseDecoderResources(); On 2015/06/25 20:33:00, qinmin wrote: > nit: not needed, MediaCodecDecoder dtor will automatically call this for you Removed https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... media/base/android/media_codec_player.cc:689: MediaCodecDecoder::SetTimeCallback(), On 2015/06/25 20:33:00, qinmin wrote: > This is strange, why passing a variable that is member of MediaCodecDecoder to > its child class? Cannot the child class get this by itself? I'm passing null callback here. I'm calling constructor, SetTimeCallback(). Added a comment. The null callback is later checked at https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... media/base/android/media_codec_player.h:311: base::WeakPtrFactory<MediaCodecPlayer> weak_factory_; On 2015/06/25 20:33:00, qinmin wrote: > I would this this to media_weak_factory_ to avoid confuse reviewers, same for > the weak_this_ Done.
On 2015/06/25 20:36:46, liberato wrote: > lgtm > > thanks for clarifying > -fl > > https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... > File media/base/android/media_codec_player.cc (right): > > https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... > media/base/android/media_codec_player.cc:372: if (state_ != STATE_PREFETCHING) > On 2015/06/24 02:31:07, Tima Vaisburd wrote: > > On 2015/06/23 14:58:59, liberato wrote: > > > does this actually happen? > > > > I think it can happen now with Pause() or Release() racing with > > OnPrefetchDone(), but maybe I should not post the callback in > > MediaCodecDecoder::PrefetchNextChunk()? > > i'm only worried about it missing the state transition to WAITING_FOR_SURFACE. > but it looks like it always goes through prefetch, so it's okay. Yes, Prefetching ---(prefetch done)---> (check for surface) --(surface exist? Start) --> Playing (surface does not exist?) --> WaitingForSurface We do not always transition to WaitingForSurface
On 2015/06/25 21:23:38, Tima Vaisburd wrote: > On 2015/06/25 20:36:46, liberato wrote: > > lgtm > > > > thanks for clarifying > > -fl > > > > > https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... > > File media/base/android/media_codec_player.cc (right): > > > > > https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... > > media/base/android/media_codec_player.cc:372: if (state_ != STATE_PREFETCHING) > > On 2015/06/24 02:31:07, Tima Vaisburd wrote: > > > On 2015/06/23 14:58:59, liberato wrote: > > > > does this actually happen? > > > > > > I think it can happen now with Pause() or Release() racing with > > > OnPrefetchDone(), but maybe I should not post the callback in > > > MediaCodecDecoder::PrefetchNextChunk()? > > > > i'm only worried about it missing the state transition to WAITING_FOR_SURFACE. > > > but it looks like it always goes through prefetch, so it's okay. > > Yes, Prefetching ---(prefetch done)---> (check for surface) --(surface exist? > Start) --> Playing > (surface does not > exist?) --> WaitingForSurface > > We do not always transition to WaitingForSurface true, but that's not quite what i meant. i was worried about the case where we should transition to WaitingForSurface, but the race causes us to return early and we miss it. -fl
https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... media/base/android/media_codec_player.cc:689: MediaCodecDecoder::SetTimeCallback(), On 2015/06/25 21:15:45, Tima Vaisburd wrote: > On 2015/06/25 20:33:00, qinmin wrote: > > This is strange, why passing a variable that is member of MediaCodecDecoder to > > its child class? Cannot the child class get this by itself? > > I'm passing null callback here. I'm calling constructor, SetTimeCallback(). > Added a comment. The null callback is later checked at > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... Then cannot we just remove this from MediaCodecVideoDecoder ctor?
On 2015/06/25 21:54:07, qinmin wrote: > https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... > File media/base/android/media_codec_player.cc (right): > > https://codereview.chromium.org/1184373005/diff/120001/media/base/android/med... > media/base/android/media_codec_player.cc:689: > MediaCodecDecoder::SetTimeCallback(), > On 2015/06/25 21:15:45, Tima Vaisburd wrote: > > On 2015/06/25 20:33:00, qinmin wrote: > > > This is strange, why passing a variable that is member of MediaCodecDecoder > to > > > its child class? Cannot the child class get this by itself? > > > > I'm passing null callback here. I'm calling constructor, SetTimeCallback(). > > Added a comment. The null callback is later checked at > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... > > Then cannot we just remove this from MediaCodecVideoDecoder ctor? I use it for decoder unit tests to make sure that the player is playing. I'd like to have the ability to test this for both audio and video decoders. Also it was easier to test video e.g. in stop/resume because the timestamps are more predictable for video.
Mostly questions and minor comments. https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/40001/media/base/android/medi... media/base/android/media_codec_player.h:26: // The player works as a state machine. Here are relationships between states: On 2015/06/24 06:17:56, liberato wrote: > On 2015/06/24 02:31:07, Tima Vaisburd wrote: > > On 2015/06/23 14:58:59, liberato wrote: > > > this comment would be clearer with a little color and animation. and audio > > > narration. in 7.1. text-only comments in media code don't set the right > > > example. > > > > I take this as a subtle hint that these diagrams are better be removed. Well, > it > > is definitely easier than was adding them. Removed most of it. > > no! > > they're great. please git reflog them back! i just felt the need to comment on > them, kind of like commenting on mt. rushmore -- you just have to say > _something_. > > i can see them being a maintenance problem (nobody is going to keep them up to > date), but i'd rather deal with that if it actually arises. i'm sure there are > more compact ways to portray it, but that's a problem we should be glad to have > more often. > > sorry about the confusion. aside: I use "aside:" when I see something I like and want to mention that or I have a non-l*tm-blocking question or other non-nit that I'd like to mention... :) https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:124: void MediaCodecPlayer::SetVideoSurface(gfx::ScopedJavaSurface surface) { nit: Here and in other RUN_ON_MEDIA_THREAD() below, could this ever be called on the media thread, or is it always called on UI thread? If the latter, DCHECK we're on UI first, please. https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:318: FROM_HERE, base::Bind(metadata_changed_cb_, duration_, gfx::Size())); MediaSourcePlayer doesn't appear to signal metadata change when handling OnDemuxerDurationChanged(). Why is MediaCodecPlayer different? https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:532: if (state_ == STATE_WAITING_FOR_CONFIG) { nit: DCHECK if neither audio nor video codecs are != UnknownVideoCodec, and don't start prefetching in that case, too? https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:62: // DemuxerConfigs: On which transition(s) is DemuxerConfigs: to live? It looks wrong. https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:94: // Events and actions for seek workflow. nit: Seek isn't implemented in this CL, so perhaps this portion needs to be added when Seek is implemented? No strong feeling; I'm fine if you include it. Though adding it later would also help trigger code reviewers to validate the seek implementation against the state diagram if added at same time... https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:217: // This method cached the current time and calls manager's OnTimeUpdate(). nit: s/cached/caches/ https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:223: void OnPrefetchDone(); nit: why not parameterize (all of) these decoder callbacks by stream type? https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:283: // Error callback is posted by decoders if they get errors from MediaCodec Does the player not intercept all error callbacks from the decoders? How else can it correctly transition to Error state in those cases? https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:284: // operations of by this class itself if we cannot configure or start decoder. nit: s/of/or/
https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:124: void MediaCodecPlayer::SetVideoSurface(gfx::ScopedJavaSurface surface) { On 2015/06/25 22:31:39, wolenetz wrote: > nit: Here and in other RUN_ON_MEDIA_THREAD() below, could this ever be called on > the media thread, or is it always called on UI thread? If the latter, DCHECK > we're on UI first, please. RUN_ON_MEDIA_THREAD() macro will call the method again, second time on the Media thread. So the only place to check would be within the macro itself. However, it already ensures the threading model "from any thread to Media thread". In a sense the originating thread becomes less important, I think. I'd rather keep the macro as-is if you agree. https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:318: FROM_HERE, base::Bind(metadata_changed_cb_, duration_, gfx::Size())); On 2015/06/25 22:31:39, wolenetz wrote: > MediaSourcePlayer doesn't appear to signal metadata change when handling > OnDemuxerDurationChanged(). Why is MediaCodecPlayer different? I talked to Min, in this case the renderer process already knows. Removed the notification. https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:532: if (state_ == STATE_WAITING_FOR_CONFIG) { On 2015/06/25 22:31:39, wolenetz wrote: > nit: DCHECK if neither audio nor video codecs are != UnknownVideoCodec, and > don't start prefetching in that case, too? I added DCHECK that at least one codec must be present. I have a question about skipping such input though. I recently read a mail on chromium mailing list that explicitly advised against doing both assertion and gracefully handling the same condition. In other words, the letter said: // Do not do this: pointer either can or cannot be zero: ASSERT(ptr); if (!ptr) return; I'm not 100% sure myself, this is an IPC protocol. On the other hand we only did DCHECKs with the AccessUnitQueue. Do you feel strongly that we need to skip the impossible input? https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:62: // DemuxerConfigs: On 2015/06/25 22:31:39, wolenetz wrote: > On which transition(s) is DemuxerConfigs: to live? It looks wrong. Indeed! Thank you. Fixed. https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:94: // Events and actions for seek workflow. On 2015/06/25 22:31:39, wolenetz wrote: > nit: Seek isn't implemented in this CL, so perhaps this portion needs to be > added when Seek is implemented? No strong feeling; I'm fine if you include it. > Though adding it later would also help trigger code reviewers to validate the > seek implementation against the state diagram if added at same time... Actually, I quite like this idea. Especially now as I have the seek CL that I'm working on. I'll move it there. https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:217: // This method cached the current time and calls manager's OnTimeUpdate(). On 2015/06/25 22:31:39, wolenetz wrote: > nit: s/cached/caches/ Done. https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:223: void OnPrefetchDone(); On 2015/06/25 22:31:39, wolenetz wrote: > nit: why not parameterize (all of) these decoder callbacks by stream type? OnPrefetchDone() is a barrier callback. OnError() is called not only from decoders, but from the player itself. There is one case that OnStopDone() is also generated by the player itself. I added the stream type to OnStarvation() and OnTimeUpdate(). https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:283: // Error callback is posted by decoders if they get errors from MediaCodec On 2015/06/25 22:31:39, wolenetz wrote: > Does the player not intercept all error callbacks from the decoders? How else > can it correctly transition to Error state in those cases? That's what I meant. I tried to also say why decoders post error callback, but it turned out confusing. Removed "if they get..." https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:284: // operations of by this class itself if we cannot configure or start decoder. On 2015/06/25 22:31:39, wolenetz wrote: > nit: s/of/or/ Done.
Getting quite close. Can MediaCodecPlayer handle a video-only stream's playback? (ditto for audio-only) https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:124: void MediaCodecPlayer::SetVideoSurface(gfx::ScopedJavaSurface surface) { On 2015/06/26 00:02:05, Tima Vaisburd wrote: > On 2015/06/25 22:31:39, wolenetz wrote: > > nit: Here and in other RUN_ON_MEDIA_THREAD() below, could this ever be called > on > > the media thread, or is it always called on UI thread? If the latter, DCHECK > > we're on UI first, please. > > RUN_ON_MEDIA_THREAD() macro will call the method again, second time on the Media > thread. So the only place to check would be within the macro itself. However, it > already ensures the threading model "from any thread to Media thread". In a > sense the originating thread becomes less important, I think. I'd rather keep > the macro as-is if you agree. This was just a nit. Mainly, I would like to have the thread expectations as tight as possible in the code, and just using this macro makes it non-obvious on which thread a call might be originally received. If it is indeed the case that the thread expectation is stable and known now at (some of) the various usage points of this macro, please add appropriate DCHECKs (and drop usage of the macro in favor of explicit posting to the media thread if on UI/other thread, or DCHECK+no-op if expected to be on media thread). However, if a usage point of this macro could validly occur on more than one thread, then that usage point may remain as-is. wdyt? https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:532: if (state_ == STATE_WAITING_FOR_CONFIG) { On 2015/06/26 00:02:05, Tima Vaisburd wrote: > On 2015/06/25 22:31:39, wolenetz wrote: > > nit: DCHECK if neither audio nor video codecs are != UnknownVideoCodec, and > > don't start prefetching in that case, too? > > I added DCHECK that at least one codec must be present. I have a question about > skipping such input though. I recently read a mail on chromium mailing list that > explicitly advised against doing both assertion and gracefully handling the same > condition. In other words, the letter said: > // Do not do this: pointer either can or cannot be zero: > ASSERT(ptr); > if (!ptr) return; > > I'm not 100% sure myself, this is an IPC protocol. On the other hand we only did > DCHECKs with the AccessUnitQueue. Do you feel strongly that we need to skip the > impossible input? no strong feeling. whether we prefetch or not on a release build should hopefully both result in exposure of "something is wrong here" if the impossible input occurs. https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.h:223: void OnPrefetchDone(); On 2015/06/26 00:02:05, Tima Vaisburd wrote: > On 2015/06/25 22:31:39, wolenetz wrote: > > nit: why not parameterize (all of) these decoder callbacks by stream type? > > OnPrefetchDone() is a barrier callback. OnError() is called not only from > decoders, but from the player itself. There is one case that OnStopDone() is > also generated by the player itself. I added the stream type to OnStarvation() > and OnTimeUpdate(). Acknowledged. https://codereview.chromium.org/1184373005/diff/160001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:435: DVLOG(1) << __FUNCTION__; nit: log the |type| https://codereview.chromium.org/1184373005/diff/160001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/160001/media/base/android/med... media/base/android/media_codec_player.h:31: // | <------------------[ WaitingForConfig ] [ Error ] How do we reach Error state if a decoder posts our |error_cb_|, but we don't intercept? I think I might be confused... Is there a possibility a decoder could be in Error, but Player might remain in some non-Error state?
Audio-only streams are supported, video-only - not yet. https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:124: void MediaCodecPlayer::SetVideoSurface(gfx::ScopedJavaSurface surface) { On 2015/06/26 00:59:25, wolenetz wrote: > On 2015/06/26 00:02:05, Tima Vaisburd wrote: > > On 2015/06/25 22:31:39, wolenetz wrote: > > > nit: Here and in other RUN_ON_MEDIA_THREAD() below, could this ever be > called > > on > > > the media thread, or is it always called on UI thread? If the latter, DCHECK > > > we're on UI first, please. > > > > RUN_ON_MEDIA_THREAD() macro will call the method again, second time on the > Media > > thread. So the only place to check would be within the macro itself. However, > it > > already ensures the threading model "from any thread to Media thread". In a > > sense the originating thread becomes less important, I think. I'd rather keep > > the macro as-is if you agree. > > This was just a nit. Mainly, I would like to have the thread expectations as > tight as possible in the code, and just using this macro makes it non-obvious on > which thread a call might be originally received. If it is indeed the case that > the thread expectation is stable and known now at (some of) the various usage > points of this macro, please add appropriate DCHECKs (and drop usage of the > macro in favor of explicit posting to the media thread if on UI/other thread, or > DCHECK+no-op if expected to be on media thread). However, if a usage point of > this macro could validly occur on more than one thread, then that usage point > may remain as-is. wdyt? Added DCHECK. Indeed, "not media thread" should mean "UI thread" here. https://codereview.chromium.org/1184373005/diff/140001/media/base/android/med... media/base/android/media_codec_player.cc:532: if (state_ == STATE_WAITING_FOR_CONFIG) { On 2015/06/26 00:59:25, wolenetz wrote: > On 2015/06/26 00:02:05, Tima Vaisburd wrote: > > On 2015/06/25 22:31:39, wolenetz wrote: > > > nit: DCHECK if neither audio nor video codecs are != UnknownVideoCodec, and > > > don't start prefetching in that case, too? > > > > I added DCHECK that at least one codec must be present. I have a question > about > > skipping such input though. I recently read a mail on chromium mailing list > that > > explicitly advised against doing both assertion and gracefully handling the > same > > condition. In other words, the letter said: > > // Do not do this: pointer either can or cannot be zero: > > ASSERT(ptr); > > if (!ptr) return; > > > > I'm not 100% sure myself, this is an IPC protocol. On the other hand we only > did > > DCHECKs with the AccessUnitQueue. Do you feel strongly that we need to skip > the > > impossible input? > > no strong feeling. whether we prefetch or not on a release build should > hopefully both result in exposure of "something is wrong here" if the impossible > input occurs. Then I leave it as is. https://codereview.chromium.org/1184373005/diff/160001/media/base/android/med... File media/base/android/media_codec_player.cc (right): https://codereview.chromium.org/1184373005/diff/160001/media/base/android/med... media/base/android/media_codec_player.cc:435: DVLOG(1) << __FUNCTION__; On 2015/06/26 00:59:25, wolenetz wrote: > nit: log the |type| Done. https://codereview.chromium.org/1184373005/diff/160001/media/base/android/med... File media/base/android/media_codec_player.h (right): https://codereview.chromium.org/1184373005/diff/160001/media/base/android/med... media/base/android/media_codec_player.h:31: // | <------------------[ WaitingForConfig ] [ Error ] On 2015/06/26 00:59:25, wolenetz wrote: > How do we reach Error state if a decoder posts our |error_cb_|, but we don't > intercept? I think I might be confused... Is there a possibility a decoder could > be in Error, but Player might remain in some non-Error state? That should not be possible.
lgtm. We also chatted separately, confirming that video-only, surface change, midstream config change are all not in scope of this CL to keep the size down, but will be in later CLs (seek, too, IIUC).
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, qinmin@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1184373005/#ps180001 (title: "Added DCHECK into RUN_ON_MEDIA_THREAD()")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1184373005/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, qinmin@chromium.org, wolenetz@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1184373005/#ps200001 (title: "Fixed OnVideoSizeChanged() shading")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1184373005/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/389da2e28a6e675da59bdb5fb492cbba77df9874 Cr-Commit-Position: refs/heads/master@{#336441} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
