Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
149123:
Asynchronous initialization of media::PipelineThread... (Closed)
|
Created:
11 years, 6 months ago by Alpha Left Google Modified:
9 years, 6 months ago CC:
chromium-reviews_googlegroups.com Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAsynchronous initialization of media::PipelineThread
The initialization of media::PipelineThread needs to
done asynchronously or a lot of dead lock will happen.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19787
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 75
Patch Set 4 : '' #
Total comments: 31
Patch Set 5 : '' #
Total comments: 6
Patch Set 6 : '' #
Total comments: 2
Messages
Total messages: 10 (0 generated)
The code still needs to be cleaned up but it shows the basic idea of getting rid of the nested message loop. The idea of using a waitable event to wait for initialization complete was bad because it creates more problems.. I still need to add one more unit test on throwing pipeline error during initialization, e.g. codec not supported.
ping!
awesome stuff! http://codereview.chromium.org/149123/diff/13/1009 File media/base/pipeline_impl.cc (right): http://codereview.chromium.org/149123/diff/13/1009#newcode6 Line 6: // potential deadlocks, nested message loops, etc... you can remove "nested message loops" from this list ;) http://codereview.chromium.org/149123/diff/13/1009#newcode299 Line 299: // dedicated thread and posts a task to call the StartTask method on that StartTask -> StartTask() http://codereview.chromium.org/149123/diff/13/1009#newcode320 Line 320: // that that Thread::Stop call will quit the appropriate message loop. add () to methods in this comment update the comment as well (it mentions grabbing a lock) http://codereview.chromium.org/149123/diff/13/1009#newcode364 Line 364: // Called from any thread. Sets the pipeline error_ member and schedules a error_ -> |error_| http://codereview.chromium.org/149123/diff/13/1009#newcode366 Line 366: // continue to run until the client calls Pipeline::Stop, but nothing will Stop -> Stop() http://codereview.chromium.org/149123/diff/13/1009#newcode378 Line 378: // internally. Old comment I missed :( I'd replace it with something simple "Helper to improve readability for posting tasks to the pipeline thread." http://codereview.chromium.org/149123/diff/13/1009#newcode384 Line 384: // Main initialization method called on the pipeline thread. This code attempts update this comment and add () to method names, || to variable names etc most importantly mention this is no longer a synchronous method and that it depends on the current state http://codereview.chromium.org/149123/diff/13/1009#newcode399 Line 399: void PipelineThread::StartTask() { DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); http://codereview.chromium.org/149123/diff/13/1009#newcode400 Line 400: // If we havge received the stop signal, return immediately. havge -> have http://codereview.chromium.org/149123/diff/13/1009#newcode404 Line 404: DCHECK(state_ >= kCreated && state_ < kStarted); I'm always worried when using enums with greater/less than. It enforces a special ordering in the header file that is only apparent inside the code Since you have this condition in a few places, I would consider adding a helper method that actually confirms the correct states: bool IsPipelineInitializing() { return state_ == kInitDataSource || ...; } then have: DCHECK(state_ == kCreated || IsPipelineInitializing()); http://codereview.chromium.org/149123/diff/13/1009#newcode409 Line 409: if (CreateDataSource()) (relates to my comment below) some of these functions always return true, in which case they should just be void furthermore, this method proceeds to the next state if Create*() returned false -- is this intended? Maybe you should only set the state if these functions succeed If we couldn't create a demuxer I would consider that to be a fatal error.. I wouldn't continue to create more filters In summary, some of these failures are fatal http://codereview.chromium.org/149123/diff/13/1009#newcode430 Line 430: if (audio_renderer) instead of this, I would assume that we only reach this state if we created an audio decoder therefore you can skip a state and set state_ to kInitVideoDecoder if CreateRenderer<AudioDecoder, AudioRenderer>() fails you can add a DCHECK(audio_renderer) here to confirm that you don't mistakenly enter this state http://codereview.chromium.org/149123/diff/13/1009#newcode430 Line 430: if (audio_renderer) unindent by two http://codereview.chromium.org/149123/diff/13/1009#newcode438 Line 438: if (CreateRenderer<VideoDecoder, VideoRenderer>()) unindent by two http://codereview.chromium.org/149123/diff/13/1009#newcode444 Line 444: if (video_renderer) ditto for video http://codereview.chromium.org/149123/diff/13/1009#newcode448 Line 448: if (PipelineOk() && !pipeline_->rendered_mime_types_.empty()) { tiny readability nit: I think it would be clearer if this checked for failing conditions and errored/returned immediately then you had all the "success" code together if (!PipelineOk() || ...) { Error(...); return; } state_ = kStarted; pipeline_->initialized_ = true; etc... http://codereview.chromium.org/149123/diff/13/1009#newcode469 Line 469: void PipelineThread::StopTask() { DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); http://codereview.chromium.org/149123/diff/13/1009#newcode530 Line 530: void PipelineThread::SetPlaybackRateTask(float rate) { DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); http://codereview.chromium.org/149123/diff/13/1009#newcode570 Line 570: void PipelineThread::SetTimeTask() { DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); http://codereview.chromium.org/149123/diff/13/1009#newcode579 Line 579: void PipelineThread::ErrorTask() { DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); http://codereview.chromium.org/149123/diff/13/1009#newcode580 Line 580: if (state_ > kCreated && state_ < kStarted) { you could use something like IsPipelineInitializing() here as well http://codereview.chromium.org/149123/diff/13/1009#newcode590 Line 590: void PipelineThread::GetFilter(scoped_refptr<Filter>* filter_out) const { DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); http://codereview.chromium.org/149123/diff/13/1009#newcode600 Line 600: scoped_refptr<Filter> PipelineThread::CreateFilter( DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); http://codereview.chromium.org/149123/diff/13/1009#newcode609 Line 609: FilterHostImpl* host = new FilterHostImpl(this, filter.get()); use a scoped_ptr and call release() when adding it to filter_hosts_ http://codereview.chromium.org/149123/diff/13/1009#newcode639 Line 639: bool PipelineThread::CreateDataSource() { I would consider making these functions void if they always return true http://codereview.chromium.org/149123/diff/13/1009#newcode639 Line 639: bool PipelineThread::CreateDataSource() { DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); http://codereview.chromium.org/149123/diff/13/1009#newcode648 Line 648: bool PipelineThread::CreateDemuxer() { DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); http://codereview.chromium.org/149123/diff/13/1009#newcode658 Line 658: bool PipelineThread::CreateDecoder() { DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); http://codereview.chromium.org/149123/diff/13/1009#newcode679 Line 679: bool PipelineThread::CreateRenderer() { DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); http://codereview.chromium.org/149123/diff/13/1007 File media/base/pipeline_impl.h (right): http://codereview.chromium.org/149123/diff/13/1007#newcode243 Line 243: // StartTask message on the pipeline thread. fix up this comment (i.e., add () to method calls, make sure it is still applicable) http://codereview.chromium.org/149123/diff/13/1007#newcode310 Line 310: // Creates a decoder of type Decoder. Returns true if asynchronous action if -> if the I'd also add that returning false could also mean that perhaps there is no audio/video to decode http://codereview.chromium.org/149123/diff/13/1007#newcode316 Line 316: // true if asynchronous action of creating renderer has started. if -> if the http://codereview.chromium.org/149123/diff/13/1007#newcode340 Line 340: scoped_ptr<PipelineCallback> init_callback_; comments for these http://codereview.chromium.org/149123/diff/13/1008 File media/base/pipeline_impl_unittest.cc (right): http://codereview.chromium.org/149123/diff/13/1008#newcode203 Line 203: // Try to execute Start/Stop on the Pipeline many times and very fast. This test add () http://codereview.chromium.org/149123/diff/13/1008#newcode204 Line 204: // is trying to simulate the situation that the pipeline can gets dead locked "that the pipeline can gets dead locked" -> "where the pipeline can get dead locked" http://codereview.chromium.org/149123/diff/13/1008#newcode205 Line 205: // very easily by quick Start/Stop. quick -> quickly calling http://codereview.chromium.org/149123/diff/13/1008#newcode206 Line 206: TEST_F(PipelineImplTest, FastPipelineStartStop) { nit: "StressTest" is probably a more accurate description of the test http://codereview.chromium.org/149123/diff/13/1008#newcode206 Line 206: TEST_F(PipelineImplTest, FastPipelineStartStop) { does this test actually trigger the deadlock? if so... that's awesome! http://codereview.chromium.org/149123/diff/13/1008#newcode210 Line 210: for (int i = 0; i < kTimes; ++i) { I think we should be re-creating the pipeline every time, right?
http://codereview.chromium.org/149123/diff/13/1009 File media/base/pipeline_impl.cc (right): http://codereview.chromium.org/149123/diff/13/1009#newcode6 Line 6: // potential deadlocks, nested message loops, etc... On 2009/06/30 02:14:18, scherkus wrote: > you can remove "nested message loops" from this list ;) Done. http://codereview.chromium.org/149123/diff/13/1009#newcode299 Line 299: // dedicated thread and posts a task to call the StartTask method on that On 2009/06/30 02:14:18, scherkus wrote: > StartTask -> StartTask() Done. http://codereview.chromium.org/149123/diff/13/1009#newcode320 Line 320: // that that Thread::Stop call will quit the appropriate message loop. On 2009/06/30 02:14:18, scherkus wrote: > add () to methods in this comment > > update the comment as well (it mentions grabbing a lock) Done. http://codereview.chromium.org/149123/diff/13/1009#newcode364 Line 364: // Called from any thread. Sets the pipeline error_ member and schedules a On 2009/06/30 02:14:18, scherkus wrote: > error_ -> |error_| Done. http://codereview.chromium.org/149123/diff/13/1009#newcode366 Line 366: // continue to run until the client calls Pipeline::Stop, but nothing will On 2009/06/30 02:14:18, scherkus wrote: > Stop -> Stop() Done. http://codereview.chromium.org/149123/diff/13/1009#newcode378 Line 378: // internally. On 2009/06/30 02:14:18, scherkus wrote: > Old comment I missed :( > > I'd replace it with something simple "Helper to improve readability for posting > tasks to the pipeline thread." Done. http://codereview.chromium.org/149123/diff/13/1009#newcode384 Line 384: // Main initialization method called on the pipeline thread. This code attempts On 2009/06/30 02:14:18, scherkus wrote: > update this comment and add () to method names, || to variable names etc > > most importantly mention this is no longer a synchronous method and that it > depends on the current state Done. http://codereview.chromium.org/149123/diff/13/1009#newcode399 Line 399: void PipelineThread::StartTask() { On 2009/06/30 02:14:18, scherkus wrote: > DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); Done. http://codereview.chromium.org/149123/diff/13/1009#newcode400 Line 400: // If we havge received the stop signal, return immediately. On 2009/06/30 02:14:18, scherkus wrote: > havge -> have Done. http://codereview.chromium.org/149123/diff/13/1009#newcode409 Line 409: if (CreateDataSource()) On 2009/06/30 02:14:18, scherkus wrote: > (relates to my comment below) > > some of these functions always return true, in which case they should just be > void > > furthermore, this method proceeds to the next state if Create*() returned false > -- is this intended? Maybe you should only set the state if these functions > succeed We can't do this because CreateDecoder and CreateRenderer are template functions, and it looks much easier to understand about the state transition grouped in the same place. I intended for CreateDecoder and CreateRenderer to return false for skipping the state. > > If we couldn't create a demuxer I would consider that to be a fatal error.. I > wouldn't continue to create more filters > > In summary, some of these failures are fatal > > In order to handle this, I can raise an error in CreateDataSource and CreateDemuxer. http://codereview.chromium.org/149123/diff/13/1009#newcode430 Line 430: if (audio_renderer) On 2009/06/30 02:14:18, scherkus wrote: > instead of this, I would assume that we only reach this state if we created an > audio decoder > > therefore you can skip a state and set state_ to kInitVideoDecoder if > CreateRenderer<AudioDecoder, AudioRenderer>() fails > > you can add a DCHECK(audio_renderer) here to confirm that you don't mistakenly > enter this state If we add extra state jumping in this function, it would be hard to understand the flow. So I'd keep this and have the semantics be "if there's no such a/v stream, just do nothing in that state" http://codereview.chromium.org/149123/diff/13/1009#newcode438 Line 438: if (CreateRenderer<VideoDecoder, VideoRenderer>()) On 2009/06/30 02:14:18, scherkus wrote: > unindent by two Done. http://codereview.chromium.org/149123/diff/13/1009#newcode444 Line 444: if (video_renderer) On 2009/06/30 02:14:18, scherkus wrote: > ditto for video I want to avoid too much state transition but keep it simple as a streamline. http://codereview.chromium.org/149123/diff/13/1009#newcode448 Line 448: if (PipelineOk() && !pipeline_->rendered_mime_types_.empty()) { On 2009/06/30 02:14:18, scherkus wrote: > tiny readability nit: I think it would be clearer if this checked for failing > conditions and errored/returned immediately then you had all the "success" code > together > > if (!PipelineOk() || ...) { > Error(...); > return; > } > > state_ = kStarted; > pipeline_->initialized_ = true; > etc... Done. http://codereview.chromium.org/149123/diff/13/1009#newcode469 Line 469: void PipelineThread::StopTask() { On 2009/06/30 02:14:18, scherkus wrote: > DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); Done. http://codereview.chromium.org/149123/diff/13/1009#newcode530 Line 530: void PipelineThread::SetPlaybackRateTask(float rate) { On 2009/06/30 02:14:18, scherkus wrote: > DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); Done. http://codereview.chromium.org/149123/diff/13/1009#newcode570 Line 570: void PipelineThread::SetTimeTask() { On 2009/06/30 02:14:18, scherkus wrote: > DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); Done. http://codereview.chromium.org/149123/diff/13/1009#newcode579 Line 579: void PipelineThread::ErrorTask() { On 2009/06/30 02:14:18, scherkus wrote: > DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); Done. http://codereview.chromium.org/149123/diff/13/1009#newcode580 Line 580: if (state_ > kCreated && state_ < kStarted) { On 2009/06/30 02:14:18, scherkus wrote: > you could use something like IsPipelineInitializing() here as well Done. http://codereview.chromium.org/149123/diff/13/1009#newcode590 Line 590: void PipelineThread::GetFilter(scoped_refptr<Filter>* filter_out) const { On 2009/06/30 02:14:18, scherkus wrote: > DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); Done. http://codereview.chromium.org/149123/diff/13/1009#newcode600 Line 600: scoped_refptr<Filter> PipelineThread::CreateFilter( On 2009/06/30 02:14:18, scherkus wrote: > DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); Done. http://codereview.chromium.org/149123/diff/13/1009#newcode609 Line 609: FilterHostImpl* host = new FilterHostImpl(this, filter.get()); On 2009/06/30 02:14:18, scherkus wrote: > use a scoped_ptr and call release() when adding it to filter_hosts_ Done. http://codereview.chromium.org/149123/diff/13/1009#newcode639 Line 639: bool PipelineThread::CreateDataSource() { On 2009/06/30 02:14:18, scherkus wrote: > I would consider making these functions void if they always return true Done. http://codereview.chromium.org/149123/diff/13/1009#newcode639 Line 639: bool PipelineThread::CreateDataSource() { On 2009/06/30 02:14:18, scherkus wrote: > DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); Done. http://codereview.chromium.org/149123/diff/13/1009#newcode648 Line 648: bool PipelineThread::CreateDemuxer() { On 2009/06/30 02:14:18, scherkus wrote: > DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); Done. http://codereview.chromium.org/149123/diff/13/1009#newcode658 Line 658: bool PipelineThread::CreateDecoder() { On 2009/06/30 02:14:18, scherkus wrote: > DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); Done. http://codereview.chromium.org/149123/diff/13/1009#newcode679 Line 679: bool PipelineThread::CreateRenderer() { On 2009/06/30 02:14:18, scherkus wrote: > DCHECK_EQ(PlatformThread::CurrentId(), thread_.thread_id()); Done. http://codereview.chromium.org/149123/diff/13/1007 File media/base/pipeline_impl.h (right): http://codereview.chromium.org/149123/diff/13/1007#newcode243 Line 243: // StartTask message on the pipeline thread. On 2009/06/30 02:14:18, scherkus wrote: > fix up this comment (i.e., add () to method calls, make sure it is still > applicable) Done. http://codereview.chromium.org/149123/diff/13/1007#newcode316 Line 316: // true if asynchronous action of creating renderer has started. On 2009/06/30 02:14:18, scherkus wrote: > if -> if the Done. http://codereview.chromium.org/149123/diff/13/1007#newcode340 Line 340: scoped_ptr<PipelineCallback> init_callback_; On 2009/06/30 02:14:18, scherkus wrote: > comments for these Done. http://codereview.chromium.org/149123/diff/13/1008 File media/base/pipeline_impl_unittest.cc (right): http://codereview.chromium.org/149123/diff/13/1008#newcode203 Line 203: // Try to execute Start/Stop on the Pipeline many times and very fast. This test On 2009/06/30 02:14:18, scherkus wrote: > add () Done. http://codereview.chromium.org/149123/diff/13/1008#newcode204 Line 204: // is trying to simulate the situation that the pipeline can gets dead locked On 2009/06/30 02:14:18, scherkus wrote: > "that the pipeline can gets dead locked" -> "where the pipeline can get dead > locked" Done. http://codereview.chromium.org/149123/diff/13/1008#newcode205 Line 205: // very easily by quick Start/Stop. On 2009/06/30 02:14:18, scherkus wrote: > quick -> quickly calling Done. http://codereview.chromium.org/149123/diff/13/1008#newcode206 Line 206: TEST_F(PipelineImplTest, FastPipelineStartStop) { On 2009/06/30 02:14:18, scherkus wrote: > nit: "StressTest" is probably a more accurate description of the test Done. http://codereview.chromium.org/149123/diff/13/1008#newcode206 Line 206: TEST_F(PipelineImplTest, FastPipelineStartStop) { On 2009/06/30 02:14:18, scherkus wrote: > does this test actually trigger the deadlock? > > if so... that's awesome! If does! yay! http://codereview.chromium.org/149123/diff/13/1008#newcode210 Line 210: for (int i = 0; i < kTimes; ++i) { On 2009/06/30 02:14:18, scherkus wrote: > I think we should be re-creating the pipeline every time, right? Done.
Mostly comment/style nits, but looking great http://codereview.chromium.org/149123/diff/2002/2005 File media/base/pipeline_impl.cc (right): http://codereview.chromium.org/149123/diff/2002/2005#newcode392 Line 392: // When all required filters have been created and have called their could you add a blank // line above this line? http://codereview.chromium.org/149123/diff/2002/2005#newcode396 Line 396: // If initialization fails, the client's callback will still be called, but could you add a blank // line above this line? http://codereview.chromium.org/149123/diff/2002/2005#newcode398 Line 398: // TODO(hclam): StartTask is now starting the pipeline asynchronous. It could you add a blank // line above this line? thanks -- this comment was getting a little hard to read :) http://codereview.chromium.org/149123/diff/2002/2005#newcode448 Line 448: pipeline_->InsertRenderedMimeType(AudioDecoder::major_mime_type()); is it possible to move this code above if CreateRenderer<AudioDecoder, AudioRenderer>() returns true? http://codereview.chromium.org/149123/diff/2002/2005#newcode472 Line 472: if (IsPipelineOk() && !pipeline_->rendered_mime_types_.empty()) { sorry this is a bit of a style nit... but try to keep "success" code out of if statements and return from errors if (error condition) { // error code return; } // success code http://codereview.chromium.org/149123/diff/2002/2005#newcode669 Line 669: host.release(); since scoped_ptr<>::release() returns the released pointer, you can rewrite this as: filter->SetFilterHost(host.get()); filter_hosts_.push_back(host.release()); http://codereview.chromium.org/149123/diff/2002/2005#newcode693 Line 693: if (!data_source) { this seems very strange... this might require some more thinking. CreateFilter() will return NULL if the the pipeline is not OK (i.e., an error has been set), which also means StartTask() should not get executed again (your DCHECK asserts this). Therefore I'd rather see this code DCHECK that the filter exists http://codereview.chromium.org/149123/diff/2002/2003 File media/base/pipeline_impl.h (right): http://codereview.chromium.org/149123/diff/2002/2003#newcode166 Line 166: // asynchronous initialization. Initialization is done in multiple pass in pass -> passes http://codereview.chromium.org/149123/diff/2002/2003#newcode178 Line 178: // Initialization is a streamline of state transition from "Created" to streamline -> series transition -> transitions http://codereview.chromium.org/149123/diff/2002/2003#newcode179 Line 179: // "Started". If error happens during initialization, this object will transit If error -> If any error transit -> transition http://codereview.chromium.org/149123/diff/2002/2003#newcode180 Line 180: // to "Error" state from any state. If Stop() is called during initialization, to -> to the can drop "from any state" http://codereview.chromium.org/149123/diff/2002/2003#newcode181 Line 181: // this object will transits to "Stopped" state from any state. transits -> transition http://codereview.chromium.org/149123/diff/2002/2003#newcode258 Line 258: bool IsPipelineOk() { return PIPELINE_OK == pipeline_->error_; } nice :D http://codereview.chromium.org/149123/diff/2002/2003#newcode275 Line 275: // pass. It is executed as a result of calling Start() or pass -> passes http://codereview.chromium.org/149123/diff/2002/2003#newcode276 Line 276: // InitializationComplete() that transits initialization to next stage. It transits -> advances to next -> to the next stage -> state (maybe?) http://codereview.chromium.org/149123/diff/2002/2003#newcode309 Line 309: // and this object will remain in "Error" state. in -> in the
http://codereview.chromium.org/149123/diff/2002/2005 File media/base/pipeline_impl.cc (right): http://codereview.chromium.org/149123/diff/2002/2005#newcode392 Line 392: // When all required filters have been created and have called their On 2009/06/30 22:05:01, scherkus wrote: > could you add a blank // line above this line? Done. http://codereview.chromium.org/149123/diff/2002/2005#newcode396 Line 396: // If initialization fails, the client's callback will still be called, but On 2009/06/30 22:05:01, scherkus wrote: > could you add a blank // line above this line? Done. http://codereview.chromium.org/149123/diff/2002/2005#newcode398 Line 398: // TODO(hclam): StartTask is now starting the pipeline asynchronous. It On 2009/06/30 22:05:01, scherkus wrote: > could you add a blank // line above this line? > > thanks -- this comment was getting a little hard to read :) Done. http://codereview.chromium.org/149123/diff/2002/2005#newcode448 Line 448: pipeline_->InsertRenderedMimeType(AudioDecoder::major_mime_type()); On 2009/06/30 22:05:01, scherkus wrote: > is it possible to move this code above if CreateRenderer<AudioDecoder, > AudioRenderer>() returns true? I guess it is safe, if the filter later reports failure, renderer mime type doesn't really matter.. http://codereview.chromium.org/149123/diff/2002/2005#newcode472 Line 472: if (IsPipelineOk() && !pipeline_->rendered_mime_types_.empty()) { On 2009/06/30 22:05:01, scherkus wrote: > sorry this is a bit of a style nit... but try to keep "success" code out of if > statements and return from errors > > if (error condition) { > // error code > return; > } > // success code > Done. http://codereview.chromium.org/149123/diff/2002/2005#newcode669 Line 669: host.release(); On 2009/06/30 22:05:01, scherkus wrote: > since scoped_ptr<>::release() returns the released pointer, you can rewrite this > as: > > filter->SetFilterHost(host.get()); > filter_hosts_.push_back(host.release()); > Done. http://codereview.chromium.org/149123/diff/2002/2005#newcode693 Line 693: if (!data_source) { On 2009/06/30 22:05:01, scherkus wrote: > this seems very strange... this might require some more thinking. > > CreateFilter() will return NULL if the the pipeline is not OK (i.e., an error > has been set), which also means StartTask() should not get executed again (your > DCHECK asserts this). > > Therefore I'd rather see this code DCHECK that the filter exists Done. http://codereview.chromium.org/149123/diff/2002/2003 File media/base/pipeline_impl.h (right): http://codereview.chromium.org/149123/diff/2002/2003#newcode166 Line 166: // asynchronous initialization. Initialization is done in multiple pass in On 2009/06/30 22:05:01, scherkus wrote: > pass -> passes Done. http://codereview.chromium.org/149123/diff/2002/2003#newcode178 Line 178: // Initialization is a streamline of state transition from "Created" to On 2009/06/30 22:05:01, scherkus wrote: > streamline -> series > transition -> transitions Done. http://codereview.chromium.org/149123/diff/2002/2003#newcode179 Line 179: // "Started". If error happens during initialization, this object will transit On 2009/06/30 22:05:01, scherkus wrote: > If error -> If any error > transit -> transition Done. http://codereview.chromium.org/149123/diff/2002/2003#newcode180 Line 180: // to "Error" state from any state. If Stop() is called during initialization, On 2009/06/30 22:05:01, scherkus wrote: > to -> to the > > can drop "from any state" Done. http://codereview.chromium.org/149123/diff/2002/2003#newcode181 Line 181: // this object will transits to "Stopped" state from any state. On 2009/06/30 22:05:01, scherkus wrote: > transits -> transition Done. http://codereview.chromium.org/149123/diff/2002/2003#newcode275 Line 275: // pass. It is executed as a result of calling Start() or On 2009/06/30 22:05:01, scherkus wrote: > pass -> passes Done. http://codereview.chromium.org/149123/diff/2002/2003#newcode276 Line 276: // InitializationComplete() that transits initialization to next stage. It On 2009/06/30 22:05:01, scherkus wrote: > transits -> advances > to next -> to the next > stage -> state (maybe?) Done. http://codereview.chromium.org/149123/diff/2002/2003#newcode309 Line 309: // and this object will remain in "Error" state. On 2009/06/30 22:05:01, scherkus wrote: > in -> in the Done.
LGTM - with some tiny comment nits Awesome!!!! http://codereview.chromium.org/149123/diff/3006/2010 File media/base/pipeline_impl.cc (right): http://codereview.chromium.org/149123/diff/3006/2010#newcode610 Line 610: if (IsPipelineInitializing()) { if it helps you can empty the rendered_mime_types_ vector here or perhaps it should be done in StopTask()... hmm... http://codereview.chromium.org/149123/diff/3006/2008 File media/base/pipeline_impl.h (right): http://codereview.chromium.org/149123/diff/3006/2008#newcode180 Line 180: // transit to "Error" state from any state. If Stop() is called during transit to "Error" state from any state --> transition to the "Error" state http://codereview.chromium.org/149123/diff/3006/2008#newcode181 Line 181: // initialization, this object will transition to "Stopped" state from any state. 80 chars -- but you can delete "from any state" to be <80 chars :)
http://codereview.chromium.org/149123/diff/3006/2010 File media/base/pipeline_impl.cc (right): http://codereview.chromium.org/149123/diff/3006/2010#newcode610 Line 610: if (IsPipelineInitializing()) { On 2009/07/01 01:27:59, scherkus wrote: > if it helps you can empty the rendered_mime_types_ vector here > > or perhaps it should be done in StopTask()... > > hmm... Sounds good! http://codereview.chromium.org/149123/diff/3006/2008 File media/base/pipeline_impl.h (right): http://codereview.chromium.org/149123/diff/3006/2008#newcode180 Line 180: // transit to "Error" state from any state. If Stop() is called during On 2009/07/01 01:27:59, scherkus wrote: > transit to "Error" state from any state > --> > transition to the "Error" state Done. http://codereview.chromium.org/149123/diff/3006/2008#newcode181 Line 181: // initialization, this object will transition to "Stopped" state from any state. On 2009/07/01 01:27:59, scherkus wrote: > 80 chars -- but you can delete "from any state" to be <80 chars :) Done.
LGTM
LGTM 2 comments about comments. http://codereview.chromium.org/149123/diff/3011/2014 File media/base/pipeline_impl.cc (right): http://codereview.chromium.org/149123/diff/3011/2014#newcode400 Line 400: // TODO(hclam): StartTask is now starting the pipeline asynchronous. It asynchronous -> asynchronously http://codereview.chromium.org/149123/diff/3011/2012 File media/base/pipeline_impl.h (right): http://codereview.chromium.org/149123/diff/3011/2012#newcode336 Line 336: // Creates a Demuxer. Returns true if the asynchronous action of creating void function's done return...same elsewhere in file. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
