Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10837206:
Rewrite media::Pipeline state transition machinery and simplify shutdown. (Closed)
|
Created:
8 years, 4 months ago by scherkus (not reviewing) Modified:
8 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src Visibility:
Public. |
DescriptionRewrite media::Pipeline state transition machinery and simplify shutdown.
Shutdown is now a single call to Stop() and is called regardless of the current state of pending operations.
BUG=110228
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156011
Patch Set 1 #Patch Set 2 : fix .h #Patch Set 3 : rebase #Patch Set 4 : rebase #Patch Set 5 : remove dead code #Patch Set 6 : more dead code #Patch Set 7 : one more time #
Total comments: 83
Patch Set 8 : address comments #Patch Set 9 : #
Total comments: 31
Patch Set 10 : Removed PIPELINE_ERROR_REQUIRED_FILTER_MISSING #Patch Set 11 : fixes #
Total comments: 8
Patch Set 12 : nits #
Total comments: 2
Patch Set 13 : nits #
Messages
Total messages: 14 (0 generated)
ready for review post-acolwell's video decoder initialization and buffering status changes This change is likely to conflict with others but I'm cool dealing w/ merge pain
I like it! But also it's quite a hairy transformation (into something much better). acolwell: please review carefully :) http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc File media/base/pipeline.cc (right): http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:91: base::AutoLock auto_lock(lock_); lock in dtor ==> bug http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:253: #define RETURN_STRING(state) \ s/R_S/MAYBE_RETURN_STRING/? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:270: return ""; NOTREACHED? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:280: << "State transitions don't happen when there's an error"; emit status_ http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:305: NOTREACHED() << "State has no transition: " << state_; Put this and the next line at l.303? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:409: void Pipeline::DoStateTransition(PipelineStatus status) { why not StateTransitionTask? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:428: << "Filter state transitions must be completed via pending_callbacks_"; wat? Does this want to DCHECK_EQ(pending_callbacks.get(), state_ == kInitPrerolling || state_ == kStarting || state_ == kSeeking) << state_; ? But if it does, why are we dropping pending_callbacks_ immediately below? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:434: DVLOG(2) << GetStateString(state_) << " -> " Put this idea in SetState() ? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:440: InitializeDemuxer(done_cb); FWIW I'm pretty sure all our toolchains support returning a void function, so you can say: return InitializeDemuxer(done_cb); to make it easier to see what the last statement in each case is. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:448: clock_->SetTime(demuxer_->GetStartTime(), demuxer_->GetStartTime()); What can race with this? IOW why the lock? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:469: has_audio_ = !!audio_renderer_ && !audio_disabled_; Are the !!'s really necessary? (ditto for next line) http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:514: break; You should see the comment two lines down and return instead of breaking! http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:516: NOTREACHED() << "You should return after transitioning, not break!"; A smart compiler should flag this as dead code. Not sure it adds value. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:616: pending_callbacks_.reset(); Shouldn't you be able to DCHECK that this already IsNull()? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:624: running_ = false; You sure you don't want to do this first? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:716: // TODO(scherkus): Can I remove this?! Not before testing it! (and the CL that removes the call to it should also remove the function in VRB & GVD). I can't test this for you until I get back to SEA, unfortch. Probably want to hold off on this part of the change for now. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:748: // Playback rate changes are only carried out while playing. We no longer track pending PR changes? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:910: if (!audio_decoder_) { nit: seems strange that this check is here instead of in the caller. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:937: if (!stream) { ditto http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h File media/base/pipeline.h (left): http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h#oldc... media/base/pipeline.h:415: void ReportStatus(const PipelineStatusCB& cb, PipelineStatus status); I fear this removal, but I don't think there's a bug in this CL. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h File media/base/pipeline.h (right): http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h#newc... media/base/pipeline.h:344: void DoInitialPreroll(const base::TimeDelta& seek_timestamp, "initial" and "seek_timestamp" seem at odds to me. Drop the param and just use demuxer_->GetStartTime() inside? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h#newc... media/base/pipeline.h:353: void DoSeek(const base::TimeDelta& seek_timestamp, this is an int64; why pass-by-ref? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline_unitte... File media/base/pipeline_unittest.cc (right): http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline_unitte... media/base/pipeline_unittest.cc:116: if (audio_stream_) { braces no longer necessary (here and below).
I like it too. http://codereview.chromium.org/10837206/diff/12002/media/base/media_log.cc File media/base/media_log.cc (right): http://codereview.chromium.org/10837206/diff/12002/media/base/media_log.cc#ne... media/base/media_log.cc:62: const char* MediaLog::PipelineStateToString(Pipeline::State state) { Remove and call Pipeline::GetStateString() instead http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc File media/base/pipeline.cc (right): http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:270: return ""; nit: How about "INVALID_STATE" instead of "" so this is a little more obvious in log lines? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:427: DCHECK(pending_callbacks_.get()) nit: !pending_callbacks_.is_null() ? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:582: PlaybackRateChangedTask(GetPlaybackRate()); Why was this moved from before initial preroll to here? Won't this cause the wrong data to get prerolled for cases where PlaybackRate != 0 && PlaybackRange != 1? The audio_renderer_algorithm won't be engaged during preroll if you do this right? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:629: error_cb_.Reset(); nit: early return here and below & de-indent the error_cb case? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:716: // TODO(scherkus): Can I remove this?! No. The GPU video decoder & WebRTC decoders need this to avoid blocking the pipeline on shutdown. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:723: stop_cb_ = stop_cb; I'm pretty sure you could get rid of stop_cb_ if you just included stop_cb in the bind below. It might also simplify OnStopCompleted() http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:745: if (state_ == kStopping || state_ == kStopped) This can be removed since it is covered by the condition below this. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:768: if (state_ == kStopping || state_ == kStopped) ditto. covered by check below http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:784: if (state_ != kStarted) { Add DCHECK(state_ == kStopping || state_ == kStopped)? These seem like the only states where this would not be a bug. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:791: DCHECK_EQ(state_, kStarted) ? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:795: base::TimeDelta seek_timestamp = std::max(time, demuxer_->GetStartTime()); Should this also be capped by duration? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:855: ended_cb_.Run(status_); Are there plans to change this to a closure? It seems like status_ == PIPELINE_OK here all the time. DCHECK_EQ(state_, PIPELINE_OK) for now? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:877: if (!demuxer_) { DCHECK(demuxer_)? I can't think of a case where this would fail and it isn't the sign of a coding error. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:916: if (!audio_renderer_) { Should we DCHECK() instead? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:922: audio_decoder_, std::swap() to a local variable here so we can clear Pipeline's reference as early as possible? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:943: if (!video_renderer_) { Should we put a DCHECK() here since this failing is likely a coding error? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h File media/base/pipeline.h (right): http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h#newc... media/base/pipeline.h:75: // [ Seeking (for each filter) ] <--------[ Flushing (for each filter) ] Needs updating http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline_unitte... File media/base/pipeline_unittest.cc (right): http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline_unitte... media/base/pipeline_unittest.cc:1223: NOTREACHED() << "WTF"; Remove WTF or replace with a more helpful comment.
Thanks for the review. Outstanding Qs: - Should GetNextState() check for presence of audio/video streams + adjust next state? Or something else? (would allow DCHECKify DoInitializeXXX() methods) - Should we do away with PIPELINE_ERROR_REQUIRED_FILTER_MISSING + crash if you don't have all filters? - Can we think of something better for playback rate changes or is it good enough for time being? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc File media/base/pipeline.cc (right): http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:91: base::AutoLock auto_lock(lock_); On 2012/09/02 20:29:29, Ami Fischman wrote: > lock in dtor ==> bug > Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:253: #define RETURN_STRING(state) \ On 2012/09/02 20:29:29, Ami Fischman wrote: > s/R_S/MAYBE_RETURN_STRING/? Can't think of a good name and MAYBE_R_S seems funny. RETURN_STRING_FOR_STATE(), perhaps?! http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:270: return ""; On 2012/09/02 20:29:29, Ami Fischman wrote: > NOTREACHED? Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:270: return ""; On 2012/09/05 13:22:22, acolwell wrote: > nit: How about "INVALID_STATE" instead of "" so this is a little more obvious in > log lines? Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:280: << "State transitions don't happen when there's an error"; On 2012/09/02 20:29:29, Ami Fischman wrote: > emit status_ Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:305: NOTREACHED() << "State has no transition: " << state_; On 2012/09/02 20:29:29, Ami Fischman wrote: > Put this and the next line at l.303? Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:409: void Pipeline::DoStateTransition(PipelineStatus status) { On 2012/09/02 20:29:29, Ami Fischman wrote: > why not StateTransitionTask? Somewhere along the way I started mixing task-post-function-name styles. I'll keep it FooTask() for now. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:427: DCHECK(pending_callbacks_.get()) On 2012/09/05 13:22:22, acolwell wrote: > nit: !pending_callbacks_.is_null() ? Done (see new code / response to fischman) http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:428: << "Filter state transitions must be completed via pending_callbacks_"; On 2012/09/02 20:29:29, Ami Fischman wrote: > wat? > Does this want to > DCHECK_EQ(pending_callbacks.get(), > state_ == kInitPrerolling || state_ == kStarting || state_ == > kSeeking) > << state_; Yes > ? But if it does, why are we dropping pending_callbacks_ immediately below? pending_callbacks_ is only used for those states if we ended up here then pending_callbacks_ finished executing, meaning we can reset it the dcheck verifies that: * For states where we don't expect it, we didn't have an active pending_callbacks_ * For states where we do expect it, someone else didn't reset the object (see other instances of pending_callbacks_.reset()) Since that's not completely obvious from an all-in-one DCHECK I split it into if / else so I can have clearer debug messages, but I admit it looks a little iffy http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:434: DVLOG(2) << GetStateString(state_) << " -> " On 2012/09/02 20:29:29, Ami Fischman wrote: > Put this idea in SetState() ? Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:440: InitializeDemuxer(done_cb); On 2012/09/02 20:29:29, Ami Fischman wrote: > FWIW I'm pretty sure all our toolchains support returning a void function, so > you can say: > return InitializeDemuxer(done_cb); > to make it easier to see what the last statement in each case is. I find it odd looking but will give it a shot. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:448: clock_->SetTime(demuxer_->GetStartTime(), demuxer_->GetStartTime()); On 2012/09/02 20:29:29, Ami Fischman wrote: > What can race with this? > IOW why the lock? Pipeline::GetMediaTime() is called from any thread (renderer, various media threads) http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:469: has_audio_ = !!audio_renderer_ && !audio_disabled_; On 2012/09/02 20:29:29, Ami Fischman wrote: > Are the !!'s really necessary? (ditto for next line) AFAIK it gets around MSVC performance warning -- see timely discussion on chromium-dev: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/di06BqaudD0/dis... I'll remove it and see what trybots say. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:514: break; On 2012/09/02 20:29:29, Ami Fischman wrote: > You should see the comment two lines down and return instead of breaking! Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:516: NOTREACHED() << "You should return after transitioning, not break!"; On 2012/09/02 20:29:29, Ami Fischman wrote: > A smart compiler should flag this as dead code. Not sure it adds value. Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:582: PlaybackRateChangedTask(GetPlaybackRate()); On 2012/09/05 13:22:22, acolwell wrote: > Why was this moved from before initial preroll to here? Won't this cause the > wrong data to get prerolled for cases where PlaybackRate != 0 && PlaybackRange > != 1? The audio_renderer_algorithm won't be engaged during preroll if you do > this right? Good catch -- playback rate should always get set prior to prerolling and not prior to playing. I inserted a playback-rate-updating task prior to prerolling audio renderers, but I'm not totally happy w/ the solution (the Times(2) gmock business, etc...). Let me know what you think. The larger Q is what to do w/ playback rate changes in general. i.e., would it make sense for them to be a parameter to Preroll()? http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:616: pending_callbacks_.reset(); On 2012/09/02 20:29:29, Ami Fischman wrote: > Shouldn't you be able to DCHECK that this already IsNull()? No -- pending_callbacks_ is non-NULL but has completed (i.e. it doesn't magically self delete itself after completing) http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:624: running_ = false; On 2012/09/02 20:29:29, Ami Fischman wrote: > You sure you don't want to do this first? Not sure, but seems reasonable! http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:629: error_cb_.Reset(); On 2012/09/05 13:22:22, acolwell wrote: > nit: early return here and below & de-indent the error_cb case? If we call Stop() during initialization/seek we'll actually want to fire both seek_cb_ followed by stop_cb_ Comment added. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:716: // TODO(scherkus): Can I remove this?! On 2012/09/02 20:29:29, Ami Fischman wrote: > Not before testing it! (and the CL that removes the call to it should also > remove the function in VRB & GVD). > > I can't test this for you until I get back to SEA, unfortch. > Probably want to hold off on this part of the change for now. Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:723: stop_cb_ = stop_cb; On 2012/09/05 13:22:22, acolwell wrote: > I'm pretty sure you could get rid of stop_cb_ if you just included stop_cb in > the bind below. It might also simplify OnStopCompleted() It's admittedly used in a few spots for DCHECK-ing so I'd like to keep it around for the time being http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:745: if (state_ == kStopping || state_ == kStopped) On 2012/09/05 13:22:22, acolwell wrote: > This can be removed since it is covered by the condition below this. Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:748: // Playback rate changes are only carried out while playing. On 2012/09/02 20:29:29, Ami Fischman wrote: > We no longer track pending PR changes? Since playback_rate_ is set under lock there's no need to cache a copy of the pending value on the pipeline thread Instead we now set the PR and Volume prior to calling Play() on everyone regardless of whether the value changed (see DoPlay() and gmock expectation changes in pipeline_unittest). http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:768: if (state_ == kStopping || state_ == kStopped) On 2012/09/05 13:22:22, acolwell wrote: > ditto. covered by check below Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:784: if (state_ != kStarted) { On 2012/09/05 13:22:22, acolwell wrote: > Add DCHECK(state_ == kStopping || state_ == kStopped)? These seem like the only > states where this would not be a bug. Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:791: On 2012/09/05 13:22:22, acolwell wrote: > DCHECK_EQ(state_, kStarted) ? Hrmm.. unlike seek_cb_ it doesn't seem to be a high-value DCHECK considering we've got the if statement right above http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:795: base::TimeDelta seek_timestamp = std::max(time, demuxer_->GetStartTime()); On 2012/09/05 13:22:22, acolwell wrote: > Should this also be capped by duration? Today we rely on demuxers to cap the seek -- I'd rather not slip that change in w/ this CL http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:855: ended_cb_.Run(status_); On 2012/09/05 13:22:22, acolwell wrote: > Are there plans to change this to a closure? It seems like status_ == > PIPELINE_OK here all the time. > DCHECK_EQ(state_, PIPELINE_OK) for now? Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:877: if (!demuxer_) { On 2012/09/05 13:22:22, acolwell wrote: > DCHECK(demuxer_)? I can't think of a case where this would fail and it isn't the > sign of a coding error. Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:910: if (!audio_decoder_) { On 2012/09/02 20:29:29, Ami Fischman wrote: > nit: seems strange that this check is here instead of in the caller. I wanted to keep the state machine switch statement simpler. I suppose this can be a DCHECK() and GetNextState() could inspect for the presence of audio_decoder_ to skip over this state. Hmmmmmmmmmmm http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:916: if (!audio_renderer_) { On 2012/09/05 13:22:22, acolwell wrote: > Should we DCHECK() instead? Maybe. IIRC today we require a demuxer (i.e., otherwise we'll crash) Do we want to have the same requirement for all filters? (I'm fine w/ that -- we can likely delete the error code too) http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:922: audio_decoder_, On 2012/09/05 13:22:22, acolwell wrote: > std::swap() to a local variable here so we can clear Pipeline's reference as > early as possible? I think it's OK for now -- member variable will go away in https://chromiumcodereview.appspot.com/10918022/ http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h File media/base/pipeline.h (left): http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h#oldc... media/base/pipeline.h:415: void ReportStatus(const PipelineStatusCB& cb, PipelineStatus status); On 2012/09/02 20:29:29, Ami Fischman wrote: > I fear this removal, but I don't think there's a bug in this CL. There isn't. Promise ;) http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h File media/base/pipeline.h (right): http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h#newc... media/base/pipeline.h:75: // [ Seeking (for each filter) ] <--------[ Flushing (for each filter) ] On 2012/09/05 13:22:22, acolwell wrote: > Needs updating Done -- feel free to suggest further improvements. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h#newc... media/base/pipeline.h:344: void DoInitialPreroll(const base::TimeDelta& seek_timestamp, On 2012/09/02 20:29:29, Ami Fischman wrote: > "initial" and "seek_timestamp" seem at odds to me. > Drop the param and just use demuxer_->GetStartTime() inside? Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.h#newc... media/base/pipeline.h:353: void DoSeek(const base::TimeDelta& seek_timestamp, On 2012/09/02 20:29:29, Ami Fischman wrote: > this is an int64; why pass-by-ref? Habit with non-POD-types, I suppose. Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline_unitte... File media/base/pipeline_unittest.cc (right): http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline_unitte... media/base/pipeline_unittest.cc:116: if (audio_stream_) { On 2012/09/02 20:29:29, Ami Fischman wrote: > braces no longer necessary (here and below). Done. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline_unitte... media/base/pipeline_unittest.cc:1223: NOTREACHED() << "WTF"; On 2012/09/05 13:22:22, acolwell wrote: > Remove WTF or replace with a more helpful comment. whoops haha
https://chromiumcodereview.appspot.com/10837206/diff/22002/media/base/pipelin... File media/base/pipeline.cc (right): https://chromiumcodereview.appspot.com/10837206/diff/22002/media/base/pipelin... media/base/pipeline.cc:467: has_video_ = video_renderer_; FYI: E:\b\build\slave\win\build\src\media\base\pipeline.cc(467) : warning C4800: 'media::VideoRenderer *' : forcing value to bool 'true' or 'false' (performance warning) !!video_renderer_ then?
On 2012/09/05 15:19:30, scherkus wrote: > https://chromiumcodereview.appspot.com/10837206/diff/22002/media/base/pipelin... > File media/base/pipeline.cc (right): > > https://chromiumcodereview.appspot.com/10837206/diff/22002/media/base/pipelin... > media/base/pipeline.cc:467: has_video_ = video_renderer_; > FYI: > E:\b\build\slave\win\build\src\media\base\pipeline.cc(467) : warning C4800: > 'media::VideoRenderer *' : forcing value to bool 'true' or 'false' (performance > warning) > > !!video_renderer_ then? That's not very chrome-stylish. Why not video_renderer_!=NULL ?
On 2012/09/06 08:44:31, Ami Fischman wrote: > On 2012/09/05 15:19:30, scherkus wrote: > > > https://chromiumcodereview.appspot.com/10837206/diff/22002/media/base/pipelin... > > File media/base/pipeline.cc (right): > > > > > https://chromiumcodereview.appspot.com/10837206/diff/22002/media/base/pipelin... > > media/base/pipeline.cc:467: has_video_ = video_renderer_; > > FYI: > > E:\b\build\slave\win\build\src\media\base\pipeline.cc(467) : warning C4800: > > 'media::VideoRenderer *' : forcing value to bool 'true' or 'false' > (performance > > warning) > > > > !!video_renderer_ then? > > That's not very chrome-stylish. Why not video_renderer_!=NULL ? That works, but is less exciting. Done.
http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc File media/base/pipeline.cc (right): http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:253: #define RETURN_STRING(state) \ On 2012/09/05 15:04:17, scherkus wrote: > On 2012/09/02 20:29:29, Ami Fischman wrote: > > s/R_S/MAYBE_RETURN_STRING/? > > Can't think of a good name and MAYBE_R_S seems funny. RETURN_STRING_FOR_STATE(), > perhaps?! SGTM. Or leave as-is, given the short-livedness of the definition. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:910: if (!audio_decoder_) { On 2012/09/05 15:04:17, scherkus wrote: > On 2012/09/02 20:29:29, Ami Fischman wrote: > > nit: seems strange that this check is here instead of in the caller. > > I wanted to keep the state machine switch statement simpler. > > I suppose this can be a DCHECK() and GetNextState() could inspect for the > presence of audio_decoder_ to skip over this state. > > Hmmmmmmmmmmm Yes, that's what I meant. http://codereview.chromium.org/10837206/diff/12002/media/base/pipeline.cc#new... media/base/pipeline.cc:916: if (!audio_renderer_) { On 2012/09/05 15:04:17, scherkus wrote: > On 2012/09/05 13:22:22, acolwell wrote: > > Should we DCHECK() instead? > > Maybe. IIRC today we require a demuxer (i.e., otherwise we'll crash) > > Do we want to have the same requirement for all filters? (I'm fine w/ that -- we > can likely delete the error code too) Yes please. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc File media/base/pipeline.cc (right): http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:91: DCHECK(!running_) << "Stop() must complete before destroying object"; Should this assert running on a particular thread? http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:271: NOTREACHED(); Is it not possible to omit this and the next lines? http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:427: if (state_ == kInitPrerolling || state_ == kStarting || state_ == kSeeking) { I think what you really want is the single-statement DCHECK with a comment elucidating the (mis-coding) dangers you're guarding against. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:530: &SetPlaybackRateForPreroll, audio_renderer_, GetPlaybackRate())); Is there some reason you can't set the playbackrate in-line here and avoid the extra indirection? http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:569: &SetPlaybackRateForPreroll, audio_renderer_, GetPlaybackRate())); ditto http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:882: if (!demuxer_) { You said "done" to acolwell's comment about turning this into a DCHECK, but didn't.
http://codereview.chromium.org/10837206/diff/22002/media/base/media_log.cc File media/base/media_log.cc (right): http://codereview.chromium.org/10837206/diff/22002/media/base/media_log.cc#ne... media/base/media_log.cc:74: case Pipeline::kInitPrerolling: Any reason to not reuse Pipeline::GetStateString() here? http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc File media/base/pipeline.cc (right): http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:426: // TODO(scherkus): not every state transition uses |pending_callbacks_| today. nit: I think this comment needs to be changed into an action so it is clear what there is "to do". Perhaps "Make every state transition use |pending_callbacks_|"? http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:467: has_video_ = video_renderer_; On 2012/09/05 15:19:30, scherkus wrote: > FYI: > E:\b\build\slave\win\build\src\media\base\pipeline.cc(467) : warning C4800: > 'media::VideoRenderer *' : forcing value to bool 'true' or 'false' (performance > warning) > > !!video_renderer_ then? I'd prefer video_renderer_ != NULL since that matches most of our other code. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:496: if (status_ == PIPELINE_OK) Based on the DCHECK on line 500, I believe this conditional can be dropped and the line below moved after the DCHECKS. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:516: audio_renderer->SetPlaybackRate(playback_rate); This doesn't seem right. Won't this cause the audio renderer to get out of sync with the video renderer and clock. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:530: &SetPlaybackRateForPreroll, audio_renderer_, GetPlaybackRate())); I don't think this is right. I think you should set the rate on both renderers like the original code does. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:567: if (audio_renderer_) { How about creating a helper function for DoSeek() & DoInitialPreroll() to remove duplicate code. Something like AddPrerollFunctions(seek_timestamp, &bound_fns); http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:569: &SetPlaybackRateForPreroll, audio_renderer_, GetPlaybackRate())); I don't think you want to bind to a static function here. I think you should bind to a member function that sets the playback rate and volume on both renderers. Don't bind the rate because it may change during the pause,flush, and demuxer seek. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:731: DoStop(base::Bind(&Pipeline::OnStopCompleted, this)); Move the bind into DoStop() since all call sites do tthe exact same bind. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline_unitte... File media/base/pipeline_unittest.cc (right): http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline_unitte... media/base/pipeline_unittest.cc:265: EXPECT_CALL(*mocks_->audio_renderer(), SetVolume(_)); I really would prefer it if we could avoid changing the playback rate and volume behavior in this CL so that these changes to the test aren't needed. Would that be difficult to accomplish? It feels unrelated to the bulk of the changes.
PTAL Two tiny things: 1) I went back to my original playback rate changes, please see explanation: http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline_unitte... 2) I think I can move clock_->SetTime() code into kInitPrerolling block -- need some crowdsourced thinking on that one http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.cc#new... http://codereview.chromium.org/10837206/diff/22002/media/base/media_log.cc File media/base/media_log.cc (right): http://codereview.chromium.org/10837206/diff/22002/media/base/media_log.cc#ne... media/base/media_log.cc:74: case Pipeline::kInitPrerolling: On 2012/09/06 09:19:10, acolwell wrote: > Any reason to not reuse Pipeline::GetStateString() here? !!!!!!!!! I can't believe I never thought of this before! http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc File media/base/pipeline.cc (right): http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:91: DCHECK(!running_) << "Stop() must complete before destroying object"; On 2012/09/06 09:11:34, Ami Fischman wrote: > Should this assert running on a particular thread? That would be a nice thing! Here's hoping it doesn't spuriously DCHECK (it shouldn't as we drain the threads in ~WMPI()) http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:271: NOTREACHED(); On 2012/09/06 09:11:34, Ami Fischman wrote: > Is it not possible to omit this and the next lines? gcc complains about reaching end of non-void function (see my revert your suggestions to the end of GetNextState() in PS#11) http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:426: // TODO(scherkus): not every state transition uses |pending_callbacks_| today. On 2012/09/06 09:19:10, acolwell wrote: > nit: I think this comment needs to be changed into an action so it is clear what > there is "to do". Perhaps "Make every state transition use > |pending_callbacks_|"? Done. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:427: if (state_ == kInitPrerolling || state_ == kStarting || state_ == kSeeking) { On 2012/09/06 09:11:34, Ami Fischman wrote: > I think what you really want is the single-statement DCHECK with a comment > elucidating the (mis-coding) dangers you're guarding against. The comment I can agree with but the resulting DCHECK won't be fun to debug if it fires. Coded it up. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:467: has_video_ = video_renderer_; On 2012/09/06 09:19:10, acolwell wrote: > On 2012/09/05 15:19:30, scherkus wrote: > > FYI: > > E:\b\build\slave\win\build\src\media\base\pipeline.cc(467) : warning C4800: > > 'media::VideoRenderer *' : forcing value to bool 'true' or 'false' > (performance > > warning) > > > > !!video_renderer_ then? > I'd prefer video_renderer_ != NULL since that matches most of our other code. Done. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:496: if (status_ == PIPELINE_OK) On 2012/09/06 09:19:10, acolwell wrote: > Based on the DCHECK on line 500, I believe this conditional can be dropped and > the line below moved after the DCHECKS. Done. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:516: audio_renderer->SetPlaybackRate(playback_rate); On 2012/09/06 09:19:10, acolwell wrote: > This doesn't seem right. Won't this cause the audio renderer to get out of sync > with the video renderer and clock. Removed entirely -- see response in pipeline_unittest.cc http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:530: &SetPlaybackRateForPreroll, audio_renderer_, GetPlaybackRate())); On 2012/09/06 09:19:10, acolwell wrote: > I don't think this is right. I think you should set the rate on both renderers > like the original code does. Removed entirely -- see response in pipeline_unittest.cc http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:567: if (audio_renderer_) { On 2012/09/06 09:19:10, acolwell wrote: > How about creating a helper function for DoSeek() & DoInitialPreroll() to remove > duplicate code. Something like > > AddPrerollFunctions(seek_timestamp, &bound_fns); I think the tiny bit of duplication here is OK -- keeps things looking same-y http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:569: &SetPlaybackRateForPreroll, audio_renderer_, GetPlaybackRate())); On 2012/09/06 09:19:10, acolwell wrote: > I don't think you want to bind to a static function here. I think you should > bind to a member function that sets the playback rate and volume on both > renderers. Don't bind the rate because it may change during the pause,flush, and > demuxer seek. Removed entirely -- see response in pipeline_unittest.cc http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:731: DoStop(base::Bind(&Pipeline::OnStopCompleted, this)); On 2012/09/06 09:19:10, acolwell wrote: > Move the bind into DoStop() since all call sites do tthe exact same bind. I prefer to have the resulting callback function documented where we initiate the stop as opposed to buried within the function. Sames goes for all the DoXXX() functions. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline.cc#new... media/base/pipeline.cc:882: if (!demuxer_) { On 2012/09/06 09:11:34, Ami Fischman wrote: > You said "done" to acolwell's comment about turning this into a DCHECK, but > didn't. Remove this stuff so we crash when faced with missing filter. http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline_unitte... File media/base/pipeline_unittest.cc (right): http://codereview.chromium.org/10837206/diff/22002/media/base/pipeline_unitte... media/base/pipeline_unittest.cc:265: EXPECT_CALL(*mocks_->audio_renderer(), SetVolume(_)); On 2012/09/06 09:19:10, acolwell wrote: > I really would prefer it if we could avoid changing the playback rate and volume > behavior in this CL so that these changes to the test aren't needed. Would that > be difficult to accomplish? It feels unrelated to the bulk of the changes. Even though we chatted about rolling out my playback rate changes... when I squinted at the existing code I noticed we only respected playback rate changes _after_ seeking completed -- i.e., we're already in kStarted! I chatted w/ vrk@ and she confirmed that audio scaling is only applied when audio data is consumed and not during decoding, meaning setting the playback rate post-preroll is equivalent to setting it pre-preroll. The key part is to update it prior to calling Play() on clocks + renderers which we now do. I did some before/after listening tests where we set playbackRate immediately after setting currentTime and everything checks out. I hope this alleviates any concerns! http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.cc File media/base/pipeline.cc (right): http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.cc#new... media/base/pipeline.cc:453: // Perform any exit actions for the current state. I find this is a bit kludgy. It's the only case where we want to do something after a state. I was thinking of tossing it into the kInitPrerolling block where we set has_audio_ et al as we haven't decoded/prerolling yet so it _should_ be OK.
LGTM % nits http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.cc File media/base/pipeline.cc (right): http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.cc#new... media/base/pipeline.cc:320: NOTREACHED() << "State has no transition: " << state_; nit: If this NOTREACHED was outside the switch you'd also catch incorrect uses of "break" instead of "return" above. http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.cc#new... media/base/pipeline.cc:446: DCHECK((pending_callbacks_.get() && You missed my suggestion to use DCHECK_EQ, which makes this much easier to debug when it trips: DCHECK_EQ( pending_callbacks_.get(), state_ == kInitPrerolling || state_ == kStarting || state_ == kSeeking) << state_; ? http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.cc#new... media/base/pipeline.cc:457: base::AutoLock l(lock_); nit: this var name is unstylish. http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.h File media/base/pipeline.h (right): http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.h#newc... media/base/pipeline.h:122: // and renderers if such streams are present. Failing to do so will result in In reality the collection is constructed by clients who don't know what streams are present, so this comment is a bit misleadingly general. We could just proscribe "incomplete" collections. That's right; I said *proscribe*!
done -- will wait for acolwell to do final inspection http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.cc File media/base/pipeline.cc (right): http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.cc#new... media/base/pipeline.cc:320: NOTREACHED() << "State has no transition: " << state_; On 2012/09/07 13:19:41, Ami Fischman wrote: > nit: If this NOTREACHED was outside the switch you'd also catch incorrect uses > of "break" instead of "return" above. Done. http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.cc#new... media/base/pipeline.cc:446: DCHECK((pending_callbacks_.get() && On 2012/09/07 13:19:41, Ami Fischman wrote: > You missed my suggestion to use DCHECK_EQ, which makes this much easier to debug > when it trips: > DCHECK_EQ( > pending_callbacks_.get(), > state_ == kInitPrerolling || state_ == kStarting || state_ == kSeeking) << > state_; > ? Nifty! Done. http://codereview.chromium.org/10837206/diff/19004/media/base/pipeline.cc#new... media/base/pipeline.cc:457: base::AutoLock l(lock_); On 2012/09/07 13:19:41, Ami Fischman wrote: > nit: this var name is unstylish. Code deleted.
LGTM http://codereview.chromium.org/10837206/diff/21012/media/base/pipeline.cc File media/base/pipeline.cc (right): http://codereview.chromium.org/10837206/diff/21012/media/base/pipeline.cc#new... media/base/pipeline.cc:431: status_ = (status_ != PIPELINE_OK ? status_ : status); nit: Move this below the kStopping early return below so we don't modify status_ while stopping? Doing this provides the same behavior as ErrorChangedTask() while stopping.
thanks! http://codereview.chromium.org/10837206/diff/21012/media/base/pipeline.cc File media/base/pipeline.cc (right): http://codereview.chromium.org/10837206/diff/21012/media/base/pipeline.cc#new... media/base/pipeline.cc:431: status_ = (status_ != PIPELINE_OK ? status_ : status); On 2012/09/11 11:50:31, acolwell wrote: > nit: Move this below the kStopping early return below so we don't modify status_ > while stopping? Doing this provides the same behavior as ErrorChangedTask() > while stopping. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
