Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3014059:
media: recycle buffers/direct rendering etc. (third patch) (Closed)
|
Created:
10 years, 4 months ago by jiesun Modified:
9 years, 7 months ago CC:
chromium-reviews, scherkus (not reviewing), fbarchard, awong, Paweł Hajdan Jr., pam+watch_chromium.org, Alpha Left Google Visibility:
Public. |
Descriptionmedia: recycle buffers/direct rendering etc. (third patch)
1. ffmpeg use direct rendering for ogg/h264. webm still do not use direct rendering. for both cases, decoder owns buffers.
2. video renderer change to support flush in a more flexible way.
3. openmax 's both path had been merged and both recycle buffer,
4. finish/fine tune seek logic in openmax code .
TEST=test matrix/player_x11/layout test.
BUG = None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=58279
Patch Set 1 : work for desktop #Patch Set 2 : rc1 #Patch Set 3 : fix openmax #Patch Set 4 : rebase #Patch Set 5 : egl patch #
Total comments: 26
Patch Set 6 : layout #
Total comments: 66
Patch Set 7 : refine state #Patch Set 8 : clean up #Patch Set 9 : always decoder own buffers #Patch Set 10 : rebase #
Total comments: 126
Patch Set 11 : minor change #Patch Set 12 : code review #
Total comments: 1
Patch Set 13 : add state diagram #
Total comments: 18
Patch Set 14 : address code review issues #
Total comments: 4
Patch Set 15 : code review #
Messages
Total messages: 15 (0 generated)
please have a look.
I'm reviewing this patch now.
I haven't looked into decoders and renderers yet but I'm sending my comments on pipeline first. Also for state changes like this we should have a test in pipeline_impl_unittest that can cover the state transition. http://codereview.chromium.org/3014059/diff/22002/40001 File media/base/pipeline_impl.cc (right): http://codereview.chromium.org/3014059/diff/22002/40001#newcode335 media/base/pipeline_impl.cc:335: playback_rate_ = 0.0f; Why ResetState() not set error_ to PIPELINE_OK? So the error persist even after ResetState() is called? http://codereview.chromium.org/3014059/diff/22002/40001#newcode386 media/base/pipeline_impl.cc:386: return stop_pending_ ? kStopping : kSeeking; Can we use a method to check stop_pending_ instead of inspecting the variable directly? In the method we can do: bool IsStopPending() { if (!stop_pending) return false; DCHECK_EQ(kStopping, state_); DCHECK_EQ(kPausing, state_); DCHECK_EQ(kFlushing, state_); return true; } http://codereview.chromium.org/3014059/diff/22002/40001#newcode648 media/base/pipeline_impl.cc:648: if (state_ == kStopped || (stop_pending_ && error == PIPELINE_OK)) { Use a method instead of inspecting the variable directly. http://codereview.chromium.org/3014059/diff/22002/40001#newcode652 media/base/pipeline_impl.cc:652: } else if (state_ == kError || (stop_pending_ && error != PIPELINE_OK)) { Use a method instead of inspecting the variable directly. http://codereview.chromium.org/3014059/diff/22002/40001#newcode671 media/base/pipeline_impl.cc:671: if (IsPipelineStopped() || stop_pending_) { Use a method instead of inspecting the variable directly. http://codereview.chromium.org/3014059/diff/22002/40001#newcode819 media/base/pipeline_impl.cc:819: // We had to use parallel flushing among filters. Why not group this to the next block of code? Say: if (remaining_transitions == filters_.size()) { // This is the beginning of a new state transition. Perform parallel state transitions here. if (state_ == kFlushing) { ... } return; } // Do sequential state transition here. Mediafilter*filter = filters_[filters_.size() - remaining_transiitons_]; http://codereview.chromium.org/3014059/diff/22002/40001#newcode821 media/base/pipeline_impl.cc:821: for (size_t i = 0; i < remaining_transitions_; i++) { Use filters_.size() instead of remaining_transitions_. http://codereview.chromium.org/3014059/diff/22002/40001#newcode887 media/base/pipeline_impl.cc:887: ResetState(); Any reason we do ResetState() here? http://codereview.chromium.org/3014059/diff/22002/40001#newcode1027 media/base/pipeline_impl.cc:1027: initial_state = kStopping; nit: Why not just call StartDestroyingFilters(kStopping); http://codereview.chromium.org/3014059/diff/22002/40001#newcode1029 media/base/pipeline_impl.cc:1029: initial_state = kPausing; nit: Why not just call StartDestroyingFilters(kPausing); http://codereview.chromium.org/3014059/diff/22002/40001#newcode1034 media/base/pipeline_impl.cc:1034: void PipelineImpl::StartDestroyingFilters(State initial_state) { To avoid we maybe calling this method directly in the future can we merge this method with TearDownPipeline()? http://codereview.chromium.org/3014059/diff/22002/40002 File media/base/pipeline_impl.h (right): http://codereview.chromium.org/3014059/diff/22002/40002#newcode308 media/base/pipeline_impl.h:308: void StartDestroyingFilters(State intial_state); nit: Comments about the parameter, what is the use of initial_state. Also typo for initial_state. http://codereview.chromium.org/3014059/diff/22002/40003 File media/filters/ffmpeg_video_allocator.h (left): http://codereview.chromium.org/3014059/diff/22002/40003#oldcode30 media/filters/ffmpeg_video_allocator.h:30: ~RefCountedAVFrame() { DCHECK_EQ(usage_count_, 0); } Why remove this check?
code review concerns fixed. PipelineImpl() 's unittest did cover Stop(), I did not change the interface and semantics of the interface. so I consider this is already covered in current unittests.. http://codereview.chromium.org/3014059/diff/22002/40001 File media/base/pipeline_impl.cc (right): http://codereview.chromium.org/3014059/diff/22002/40001#newcode335 media/base/pipeline_impl.cc:335: playback_rate_ = 0.0f; current the only site ResetState() is called is inside a error == PIPELINE_OK if statement. I am reuse this in a error branch, which should be reset the error_ value. http://codereview.chromium.org/3014059/diff/22002/40001#newcode386 media/base/pipeline_impl.cc:386: return stop_pending_ ? kStopping : kSeeking; On 2010/08/21 00:08:39, Alpha wrote: > Can we use a method to check stop_pending_ instead of inspecting the variable > directly? > > In the method we can do: > bool IsStopPending() { > if (!stop_pending) > return false; > DCHECK_EQ(kStopping, state_); > DCHECK_EQ(kPausing, state_); > DCHECK_EQ(kFlushing, state_); > return true; > } Done. http://codereview.chromium.org/3014059/diff/22002/40001#newcode648 media/base/pipeline_impl.cc:648: if (state_ == kStopped || (stop_pending_ && error == PIPELINE_OK)) { On 2010/08/21 00:08:39, Alpha wrote: > Use a method instead of inspecting the variable directly. Done. http://codereview.chromium.org/3014059/diff/22002/40001#newcode652 media/base/pipeline_impl.cc:652: } else if (state_ == kError || (stop_pending_ && error != PIPELINE_OK)) { On 2010/08/21 00:08:39, Alpha wrote: > Use a method instead of inspecting the variable directly. Done. http://codereview.chromium.org/3014059/diff/22002/40001#newcode671 media/base/pipeline_impl.cc:671: if (IsPipelineStopped() || stop_pending_) { On 2010/08/21 00:08:39, Alpha wrote: > Use a method instead of inspecting the variable directly. Done. http://codereview.chromium.org/3014059/diff/22002/40001#newcode819 media/base/pipeline_impl.cc:819: // We had to use parallel flushing among filters. On 2010/08/21 00:08:39, Alpha wrote: > Why not group this to the next block of code? > > Say: > if (remaining_transitions == filters_.size()) { > // This is the beginning of a new state transition. Perform parallel state > transitions here. > if (state_ == kFlushing) { > ... > } > return; > } > > // Do sequential state transition here. > Mediafilter*filter = filters_[filters_.size() - remaining_transiitons_]; > Done. http://codereview.chromium.org/3014059/diff/22002/40001#newcode821 media/base/pipeline_impl.cc:821: for (size_t i = 0; i < remaining_transitions_; i++) { On 2010/08/21 00:08:39, Alpha wrote: > Use filters_.size() instead of remaining_transitions_. > Done. http://codereview.chromium.org/3014059/diff/22002/40001#newcode887 media/base/pipeline_impl.cc:887: ResetState(); reuse it in error case, specificly, I need stop_pending_ to be reset to false. http://codereview.chromium.org/3014059/diff/22002/40001#newcode1027 media/base/pipeline_impl.cc:1027: initial_state = kStopping; On 2010/08/21 00:08:39, Alpha wrote: > nit: Why not just call StartDestroyingFilters(kStopping); Done. http://codereview.chromium.org/3014059/diff/22002/40001#newcode1029 media/base/pipeline_impl.cc:1029: initial_state = kPausing; On 2010/08/21 00:08:39, Alpha wrote: > nit: Why not just call StartDestroyingFilters(kPausing); Done. http://codereview.chromium.org/3014059/diff/22002/40001#newcode1034 media/base/pipeline_impl.cc:1034: void PipelineImpl::StartDestroyingFilters(State initial_state) { not sure if this is good reason to merge functions. http://codereview.chromium.org/3014059/diff/22002/40002 File media/base/pipeline_impl.h (right): http://codereview.chromium.org/3014059/diff/22002/40002#newcode308 media/base/pipeline_impl.h:308: void StartDestroyingFilters(State intial_state); On 2010/08/21 00:08:39, Alpha wrote: > nit: Comments about the parameter, what is the use of initial_state. Also typo > for initial_state. > Done. http://codereview.chromium.org/3014059/diff/22002/40003 File media/filters/ffmpeg_video_allocator.h (left): http://codereview.chromium.org/3014059/diff/22002/40003#oldcode30 media/filters/ffmpeg_video_allocator.h:30: ~RefCountedAVFrame() { DCHECK_EQ(usage_count_, 0); } FFMPEG-mt handle release in delayed fashion, that's why I am reinstall the original get/release handler when we stopped. so ffmpeg-mt may still hold usage count when we destruct.
http://codereview.chromium.org/3014059/diff/8002/41006 File media/filters/ffmpeg_video_allocator.h (left): http://codereview.chromium.org/3014059/diff/8002/41006#oldcode30 media/filters/ffmpeg_video_allocator.h:30: ~RefCountedAVFrame() { DCHECK_EQ(usage_count_, 0); } I understand the reason to remove this. Is there anyway we can make sure all the buffers are released? http://codereview.chromium.org/3014059/diff/8002/41007 File media/filters/ffmpeg_video_decode_engine.cc (right): http://codereview.chromium.org/3014059/diff/8002/41007#newcode137 media/filters/ffmpeg_video_decode_engine.cc:137: if (pending_output_buffers_ == 0 && pending_input_buffers_ == 0) nit: if(!pending_output_buffers_ && !pending_input_buffers_) http://codereview.chromium.org/3014059/diff/8002/41007#newcode156 media/filters/ffmpeg_video_decode_engine.cc:156: if (pending_output_buffers_ == 0 && pending_input_buffers_ == 0) nit: if(!pending_output_buffers_ && !pending_input_buffers_) http://codereview.chromium.org/3014059/diff/8002/41007#newcode158 media/filters/ffmpeg_video_decode_engine.cc:158: } else { nit: else if (!output_eos_reached_) http://codereview.chromium.org/3014059/diff/8002/41007#newcode272 media/filters/ffmpeg_video_decode_engine.cc:272: avcodec_close(codec_context_); ffmpeg_demuxer.cc also closes the codec context, how can we avoid calling it twise? Maybe we don't need to close here and let ffmpeg_demuxer.cc closes. We can do another patch later to correct the codec close. http://codereview.chromium.org/3014059/diff/8002/41007#newcode282 media/filters/ffmpeg_video_decode_engine.cc:282: // return all the This comment is not complete. http://codereview.chromium.org/3014059/diff/8002/41007#newcode287 media/filters/ffmpeg_video_decode_engine.cc:287: event_handler_->OnFillBufferCallback(video_frame); Do you need to mark a flag to the video frame or the renderer might render the frame. http://codereview.chromium.org/3014059/diff/8002/41007#newcode292 media/filters/ffmpeg_video_decode_engine.cc:292: if (pending_input_buffers_ == 0 && pending_output_buffers_ == 0) nit: if (!pending_input_buffers_ && !pending_output_buffers) http://codereview.chromium.org/3014059/diff/8002/41007#newcode297 media/filters/ffmpeg_video_decode_engine.cc:297: CHECK_EQ(pending_input_buffers_, 0); Use DCHECKs here. http://codereview.chromium.org/3014059/diff/8002/41007#newcode305 media/filters/ffmpeg_video_decode_engine.cc:305: output_eos_reached_ = false; An explanation for doing this? http://codereview.chromium.org/3014059/diff/8002/41007#newcode309 media/filters/ffmpeg_video_decode_engine.cc:309: ReadInput(); Add an explanation for doing this. I believe this is because we only have output buffers to write to in direct rendering mode since we cached them. http://codereview.chromium.org/3014059/diff/8002/41007#newcode318 media/filters/ffmpeg_video_decode_engine.cc:318: event_handler_->OnEmptyBufferCallback(buffer); nit: event_handler_->OnEmptyBufferCallback(scoped_refptr<Buffer>()); or event event_handler_->OnEmptyBufferCallback(NULL); ? http://codereview.chromium.org/3014059/diff/8002/41008 File media/filters/ffmpeg_video_decode_engine.h (right): http://codereview.chromium.org/3014059/diff/8002/41008#newcode63 media/filters/ffmpeg_video_decode_engine.h:63: int32 pending_input_buffers_; is pending_input_buffers_ for the input pin and pending_output_buffers_ for the output pin? The comments seem to be inaccurate. http://codereview.chromium.org/3014059/diff/8002/41008#newcode64 media/filters/ffmpeg_video_decode_engine.h:64: int32 pending_output_buffers_; Also use int is okay, shouldn't use int32 unless necessary. http://codereview.chromium.org/3014059/diff/8002/41008#newcode70 media/filters/ffmpeg_video_decode_engine.h:70: std::deque<scoped_refptr<VideoFrame> > frame_queue_available_; Free as in the buffers are empty and are available to be filled? http://codereview.chromium.org/3014059/diff/8002/41009 File media/filters/ffmpeg_video_decoder.cc (right): http://codereview.chromium.org/3014059/diff/8002/41009#newcode162 media/filters/ffmpeg_video_decoder.cc:162: state_ = kFlushing; This state doesn't seem to do anything? http://codereview.chromium.org/3014059/diff/8002/41009#newcode286 media/filters/ffmpeg_video_decoder.cc:286: CHECK_NE(state_, kStopped); Change it to DCHECK? It's DCHECK elsewhere too. http://codereview.chromium.org/3014059/diff/8002/41009#newcode295 media/filters/ffmpeg_video_decoder.cc:295: // Fall through, because we still need to keep record of this frame. Do you mean return here? decode_engine_->FillThisBuffer is still being called. http://codereview.chromium.org/3014059/diff/8002/41009#newcode317 media/filters/ffmpeg_video_decoder.cc:317: // When in kFlushCodec, any errored decode, or a 0-lengthed frame, I don't think this comments apply any more, please remove it. http://codereview.chromium.org/3014059/diff/8002/41011 File media/filters/ffmpeg_video_decoder_unittest.cc (right): http://codereview.chromium.org/3014059/diff/8002/41011#newcode193 media/filters/ffmpeg_video_decoder_unittest.cc:193: remove this empty line? http://codereview.chromium.org/3014059/diff/8002/41014 File media/filters/video_renderer_base.cc (left): http://codereview.chromium.org/3014059/diff/8002/41014#oldcode352 media/filters/video_renderer_base.cc:352: if (state_ == kStopped || !current_frame_.get() || Why removing state_ == kStopped? http://codereview.chromium.org/3014059/diff/8002/41014 File media/filters/video_renderer_base.cc (right): http://codereview.chromium.org/3014059/diff/8002/41014#newcode126 media/filters/video_renderer_base.cc:126: CHECK_EQ(pending_reads_, 0u); Use DCHECK instead. http://codereview.chromium.org/3014059/diff/8002/41014#newcode188 media/filters/video_renderer_base.cc:188: if (!decoder_->ProvidesBuffer() && !uses_egl_image()) { Is uses_egl_image() needed anymore? Shouldn't decoder_->ProvidesBuffer() cover the use of uses_egl_image internally? http://codereview.chromium.org/3014059/diff/8002/41014#newcode269 media/filters/video_renderer_base.cc:269: if (remaining_time > kIdleTimeDelta) We need more explanations for these branches. Add some more comments here. http://codereview.chromium.org/3014059/diff/8002/41014#newcode271 media/filters/video_renderer_base.cc:271: if (remaining_time.InMicroseconds() < 0 && frames_queue_ready_.empty()) I believe this means that we are late showing the frame but we have no frames to show. Why would this happen? Also add comments to the top of these branches so we know what these branches are for. http://codereview.chromium.org/3014059/diff/8002/41014#newcode381 media/filters/video_renderer_base.cc:381: CHECK_NE(state_, kStopped); DCHECK here. http://codereview.chromium.org/3014059/diff/8002/41014#newcode395 media/filters/video_renderer_base.cc:395: if (pending_paint_ == false) { merge this if state with the one above. http://codereview.chromium.org/3014059/diff/8002/41014#newcode453 media/filters/video_renderer_base.cc:453: void VideoRendererBase::ReadInput(scoped_refptr<VideoFrame> frame) { This short method is only called from ScheduleRead_Locked(), I don't think there's a need to separate this out. http://codereview.chromium.org/3014059/diff/8002/41014#newcode498 media/filters/video_renderer_base.cc:498: CHECK_EQ(frames_queue_done_.size(), 0u); Use DCHECKs instead of CHECKs here in this method. http://codereview.chromium.org/3014059/diff/8002/41015 File media/filters/video_renderer_base.h (right): http://codereview.chromium.org/3014059/diff/8002/41015#newcode177 media/filters/video_renderer_base.h:177: // Keeps track of our pending buffers. We *must* have no pending buffers Do you mean no pending reads? http://codereview.chromium.org/3014059/diff/8002/41015#newcode180 media/filters/video_renderer_base.h:180: // decoder provides buffer, |pending_reads_| is always negative and if you mean non-positive right? negative excludes zero. http://codereview.chromium.org/3014059/diff/8002/41015#newcode181 media/filters/video_renderer_base.h:181: // renderer provides buffer, |pending_reads_| is always positive. you mean non-negative right? positive excludes zero. http://codereview.chromium.org/3014059/diff/8002/41015#newcode183 media/filters/video_renderer_base.h:183: size_t pending_reads_; This should be int if you want to make it negative.
PHAL http://codereview.chromium.org/3014059/diff/8002/41006 File media/filters/ffmpeg_video_allocator.h (left): http://codereview.chromium.org/3014059/diff/8002/41006#oldcode30 media/filters/ffmpeg_video_allocator.h:30: ~RefCountedAVFrame() { DCHECK_EQ(usage_count_, 0); } Ideally we can Add Flush(flush_callback) to allocator, call avcodec_flush() , wait ffmpeg-mt return all buffers, call the callback. the we should ganrantee usage count is 0 in destrcutor. but not sure how much delay the ffmpeg-mt will introduce. and also I remember when I write the patch ,ffmpeg-mt behave a little strange in h264 code. it seems that it allocate a frame even before I install the function pointer (which it should not). It had been months, I am not sure if it is still true though. http://codereview.chromium.org/3014059/diff/8002/41007 File media/filters/ffmpeg_video_decode_engine.cc (right): http://codereview.chromium.org/3014059/diff/8002/41007#newcode137 media/filters/ffmpeg_video_decode_engine.cc:137: if (pending_output_buffers_ == 0 && pending_input_buffers_ == 0) On 2010/08/23 21:17:53, Alpha wrote: > nit: if(!pending_output_buffers_ && !pending_input_buffers_) > Done. http://codereview.chromium.org/3014059/diff/8002/41007#newcode156 media/filters/ffmpeg_video_decode_engine.cc:156: if (pending_output_buffers_ == 0 && pending_input_buffers_ == 0) On 2010/08/23 21:17:53, Alpha wrote: > nit: if(!pending_output_buffers_ && !pending_input_buffers_) Done. http://codereview.chromium.org/3014059/diff/8002/41007#newcode158 media/filters/ffmpeg_video_decode_engine.cc:158: } else { On 2010/08/23 21:17:53, Alpha wrote: > nit: else if (!output_eos_reached_) > Done. http://codereview.chromium.org/3014059/diff/8002/41007#newcode272 media/filters/ffmpeg_video_decode_engine.cc:272: avcodec_close(codec_context_); On 2010/08/23 21:17:53, Alpha wrote: > ffmpeg_demuxer.cc also closes the codec context, how can we avoid calling it > twise? Maybe we don't need to close here and let ffmpeg_demuxer.cc closes. We > can do another patch later to correct the codec close. > Done. http://codereview.chromium.org/3014059/diff/8002/41007#newcode282 media/filters/ffmpeg_video_decode_engine.cc:282: // return all the On 2010/08/23 21:17:53, Alpha wrote: > This comment is not complete. Done. http://codereview.chromium.org/3014059/diff/8002/41007#newcode287 media/filters/ffmpeg_video_decode_engine.cc:287: event_handler_->OnFillBufferCallback(video_frame); renderer should have already paused, after that all frames will goto "done" queue instead of "ready" queue. http://codereview.chromium.org/3014059/diff/8002/41007#newcode292 media/filters/ffmpeg_video_decode_engine.cc:292: if (pending_input_buffers_ == 0 && pending_output_buffers_ == 0) On 2010/08/23 21:17:53, Alpha wrote: > nit: if (!pending_input_buffers_ && !pending_output_buffers) > Done. http://codereview.chromium.org/3014059/diff/8002/41007#newcode297 media/filters/ffmpeg_video_decode_engine.cc:297: CHECK_EQ(pending_input_buffers_, 0); On 2010/08/23 21:17:53, Alpha wrote: > Use DCHECKs here. > Done. http://codereview.chromium.org/3014059/diff/8002/41007#newcode305 media/filters/ffmpeg_video_decode_engine.cc:305: output_eos_reached_ = false; On 2010/08/23 21:17:53, Alpha wrote: > An explanation for doing this? > Done. http://codereview.chromium.org/3014059/diff/8002/41007#newcode309 media/filters/ffmpeg_video_decode_engine.cc:309: ReadInput(); we do not want to pass any empty or NULL frame around as request. we still need someone to start preroll and make the pipeline come to action. http://codereview.chromium.org/3014059/diff/8002/41007#newcode318 media/filters/ffmpeg_video_decode_engine.cc:318: event_handler_->OnEmptyBufferCallback(buffer); On 2010/08/23 21:17:53, Alpha wrote: > nit: event_handler_->OnEmptyBufferCallback(scoped_refptr<Buffer>()); or event > event_handler_->OnEmptyBufferCallback(NULL); ? > Done. http://codereview.chromium.org/3014059/diff/8002/41008 File media/filters/ffmpeg_video_decode_engine.h (right): http://codereview.chromium.org/3014059/diff/8002/41008#newcode63 media/filters/ffmpeg_video_decode_engine.h:63: int32 pending_input_buffers_; On 2010/08/23 21:17:53, Alpha wrote: > is pending_input_buffers_ for the input pin and pending_output_buffers_ for the > output pin? The comments seem to be inaccurate. Done. http://codereview.chromium.org/3014059/diff/8002/41008#newcode64 media/filters/ffmpeg_video_decode_engine.h:64: int32 pending_output_buffers_; On 2010/08/23 21:17:53, Alpha wrote: > Also use int is okay, shouldn't use int32 unless necessary. Done. http://codereview.chromium.org/3014059/diff/8002/41008#newcode70 media/filters/ffmpeg_video_decode_engine.h:70: std::deque<scoped_refptr<VideoFrame> > frame_queue_available_; On 2010/08/23 21:17:53, Alpha wrote: > Free as in the buffers are empty and are available to be filled? Done. http://codereview.chromium.org/3014059/diff/8002/41009 File media/filters/ffmpeg_video_decoder.cc (right): http://codereview.chromium.org/3014059/diff/8002/41009#newcode162 media/filters/ffmpeg_video_decoder.cc:162: state_ = kFlushing; used to prevent pts to enter heap. but I had changed the clear of heap to FlushComplete(). http://codereview.chromium.org/3014059/diff/8002/41009#newcode286 media/filters/ffmpeg_video_decoder.cc:286: CHECK_NE(state_, kStopped); On 2010/08/23 21:17:53, Alpha wrote: > Change it to DCHECK? It's DCHECK elsewhere too. > Done. http://codereview.chromium.org/3014059/diff/8002/41009#newcode295 media/filters/ffmpeg_video_decoder.cc:295: // Fall through, because we still need to keep record of this frame. we do not maintain all the frames in this class, we put it in the queue inside engine. so it could be recycled. http://codereview.chromium.org/3014059/diff/8002/41009#newcode317 media/filters/ffmpeg_video_decoder.cc:317: // When in kFlushCodec, any errored decode, or a 0-lengthed frame, I think this is still true. no? http://codereview.chromium.org/3014059/diff/8002/41011 File media/filters/ffmpeg_video_decoder_unittest.cc (right): http://codereview.chromium.org/3014059/diff/8002/41011#newcode193 media/filters/ffmpeg_video_decoder_unittest.cc:193: empty line before comments http://codereview.chromium.org/3014059/diff/8002/41014 File media/filters/video_renderer_base.cc (left): http://codereview.chromium.org/3014059/diff/8002/41014#oldcode352 media/filters/video_renderer_base.cc:352: if (state_ == kStopped || !current_frame_.get() || we had flushed, therefore current_frame_ should be NULL anyway. http://codereview.chromium.org/3014059/diff/8002/41014 File media/filters/video_renderer_base.cc (right): http://codereview.chromium.org/3014059/diff/8002/41014#newcode126 media/filters/video_renderer_base.cc:126: CHECK_EQ(pending_reads_, 0u); On 2010/08/23 21:17:53, Alpha wrote: > Use DCHECK instead. Done. http://codereview.chromium.org/3014059/diff/8002/41014#newcode188 media/filters/video_renderer_base.cc:188: if (!decoder_->ProvidesBuffer() && !uses_egl_image()) { unfortunate no. in gles_renderer. FillBuffer is called in the first paint. ideallly, we should let it allocate buffers when first paint or OnInitialize is called, which comes first. then we make sure that when OnInitialize() is done, frame_queue_done include all the buffer/eglimages. but that need to be refactored or something. http://codereview.chromium.org/3014059/diff/8002/41014#newcode269 media/filters/video_renderer_base.cc:269: if (remaining_time > kIdleTimeDelta) I do not agree with these conditions, in my mind ,we do not need to wait up every 10ms. we only wait up when something happened. such as 1. frame arrival 2. state chagne : such as pause/start/flush 3. play-rate change. that's probably another patch. http://codereview.chromium.org/3014059/diff/8002/41014#newcode271 media/filters/video_renderer_base.cc:271: if (remaining_time.InMicroseconds() < 0 && frames_queue_ready_.empty()) I am afarid that pending_paints will hold one frame too long... then the loop start to spin... http://codereview.chromium.org/3014059/diff/8002/41014#newcode381 media/filters/video_renderer_base.cc:381: CHECK_NE(state_, kStopped); On 2010/08/23 21:17:53, Alpha wrote: > DCHECK here. Done. http://codereview.chromium.org/3014059/diff/8002/41014#newcode395 media/filters/video_renderer_base.cc:395: if (pending_paint_ == false) { On 2010/08/23 21:17:53, Alpha wrote: > merge this if state with the one above. Done. http://codereview.chromium.org/3014059/diff/8002/41014#newcode453 media/filters/video_renderer_base.cc:453: void VideoRendererBase::ReadInput(scoped_refptr<VideoFrame> frame) { look at the gles renderer, you know why I make this separate for now. http://codereview.chromium.org/3014059/diff/8002/41014#newcode498 media/filters/video_renderer_base.cc:498: CHECK_EQ(frames_queue_done_.size(), 0u); On 2010/08/23 21:17:53, Alpha wrote: > Use DCHECKs instead of CHECKs here in this method. > Done. http://codereview.chromium.org/3014059/diff/8002/41015 File media/filters/video_renderer_base.h (right): http://codereview.chromium.org/3014059/diff/8002/41015#newcode177 media/filters/video_renderer_base.h:177: // Keeps track of our pending buffers. We *must* have no pending buffers On 2010/08/23 21:17:53, Alpha wrote: > Do you mean no pending reads? > Done. http://codereview.chromium.org/3014059/diff/8002/41015#newcode180 media/filters/video_renderer_base.h:180: // decoder provides buffer, |pending_reads_| is always negative and if On 2010/08/23 21:17:53, Alpha wrote: > you mean non-positive right? negative excludes zero. Done. http://codereview.chromium.org/3014059/diff/8002/41015#newcode181 media/filters/video_renderer_base.h:181: // renderer provides buffer, |pending_reads_| is always positive. On 2010/08/23 21:17:53, Alpha wrote: > you mean non-negative right? positive excludes zero. Done. http://codereview.chromium.org/3014059/diff/8002/41015#newcode183 media/filters/video_renderer_base.h:183: size_t pending_reads_; On 2010/08/23 21:17:53, Alpha wrote: > This should be int if you want to make it negative. Done.
update all the path to use decoder as owner. add some annotation to the changed code. please take a look. http://codereview.chromium.org/3014059/diff/68001/38004 File media/base/limits.h (right): http://codereview.chromium.org/3014059/diff/68001/38004#newcode19 media/base/limits.h:19: static const size_t kMaxVideoFrames = 4; used in multiple place, so promote to header file. http://codereview.chromium.org/3014059/diff/68001/38005 File media/filters/ffmpeg_video_allocator.h (left): http://codereview.chromium.org/3014059/diff/68001/38005#oldcode30 media/filters/ffmpeg_video_allocator.h:30: ~RefCountedAVFrame() { DCHECK_EQ(usage_count_, 0); } remove this because the way ffmpeg-mt handle release buffer in delayed fashion. I will look into how to put it back. http://codereview.chromium.org/3014059/diff/68001/38006 File media/filters/ffmpeg_video_decode_engine.cc (right): http://codereview.chromium.org/3014059/diff/68001/38006#newcode66 media/filters/ffmpeg_video_decode_engine.cc:66: direct_rendering_ = codec->capabilities & CODEC_CAP_DR1 ? true : false; make sure we use fallback when "use-system-ffmpeg" is enabled. http://codereview.chromium.org/3014059/diff/68001/38006#newcode93 media/filters/ffmpeg_video_decode_engine.cc:93: info.provides_buffers_ = true; as suggested , we always let decoder to provides buffer. in this sense, we should remove providesbuffer(), but how about when we make egl actually works before we do this? now Egl path still work. http://codereview.chromium.org/3014059/diff/68001/38006#newcode99 media/filters/ffmpeg_video_decode_engine.cc:99: frame_queue_available_.clear(); not sure if decoder could be initialize twice. maybe not necessary. http://codereview.chromium.org/3014059/diff/68001/38006#newcode110 media/filters/ffmpeg_video_decode_engine.cc:110: frame_queue_available_.push_back(video_frame); create buffer pool here. http://codereview.chromium.org/3014059/diff/68001/38006#newcode117 media/filters/ffmpeg_video_decode_engine.cc:117: frame_queue_available_.size() == Limits::kMaxVideoFrames) { if we do not have enough buffers , we will report error too. http://codereview.chromium.org/3014059/diff/68001/38006#newcode153 media/filters/ffmpeg_video_decode_engine.cc:153: if (!pending_output_buffers_ && !pending_input_buffers_) try to finish flushing. http://codereview.chromium.org/3014059/diff/68001/38006#newcode156 media/filters/ffmpeg_video_decode_engine.cc:156: DecodeFrame(buffer); otherwise try to decode this buffer. http://codereview.chromium.org/3014059/diff/68001/38006#newcode167 media/filters/ffmpeg_video_decode_engine.cc:167: allocator_->DisplayDone(codec_context_, frame); return this frame to available pool after display http://codereview.chromium.org/3014059/diff/68001/38006#newcode173 media/filters/ffmpeg_video_decode_engine.cc:173: OnFlushDone(); try to finish flushing http://codereview.chromium.org/3014059/diff/68001/38006#newcode175 media/filters/ffmpeg_video_decode_engine.cc:175: ReadInput(); if we already deliver EOS to renderer, we stop reading new input. http://codereview.chromium.org/3014059/diff/68001/38006#newcode219 media/filters/ffmpeg_video_decode_engine.cc:219: event_handler_->OnFillBufferCallback(video_frame); when we flushing codec with empty input packet, the first frame_decoded == 0 signal output frame had been drained, we mark the flag. otherwise we read it again. http://codereview.chromium.org/3014059/diff/68001/38006#newcode261 media/filters/ffmpeg_video_decode_engine.cc:261: video_frame = frame_queue_available_.front(); no direct rendering case, copy it over to available frame. available frame is guaranteed, because we issue as much read as available frame, except in line 222. where decoder reorder force us to read more input. http://codereview.chromium.org/3014059/diff/68001/38006#newcode271 media/filters/ffmpeg_video_decode_engine.cc:271: video_frame = allocator_->DecodeDone(codec_context_, av_frame_.get()); get frame from allocator which associate with av_frame_.. http://codereview.chromium.org/3014059/diff/68001/38006#newcode291 media/filters/ffmpeg_video_decode_engine.cc:291: flush_pending_ = true; mark that we are start flushing. http://codereview.chromium.org/3014059/diff/68001/38006#newcode294 media/filters/ffmpeg_video_decode_engine.cc:294: OnFlushDone(); try to finish flushing immediately. http://codereview.chromium.org/3014059/diff/68001/38006#newcode299 media/filters/ffmpeg_video_decode_engine.cc:299: DCHECK_EQ(pending_output_buffers_, 0); DCHECKs probably not needed http://codereview.chromium.org/3014059/diff/68001/38006#newcode312 media/filters/ffmpeg_video_decode_engine.cc:312: ReadInput(); Seek() is where are start preroll. http://codereview.chromium.org/3014059/diff/68001/38008 File media/filters/ffmpeg_video_decoder.cc (left): http://codereview.chromium.org/3014059/diff/68001/38008#oldcode258 media/filters/ffmpeg_video_decoder.cc:258: // we will repeatedly return empty frame upon renderer's request, this had been moved to a earlier stage in FFmpegVideoDecoder::FillThisBuffer(). http://codereview.chromium.org/3014059/diff/68001/38008 File media/filters/ffmpeg_video_decoder.cc (right): http://codereview.chromium.org/3014059/diff/68001/38008#newcode161 media/filters/ffmpeg_video_decoder.cc:161: state_ = kPausing; mark we are pausing. http://codereview.chromium.org/3014059/diff/68001/38008#newcode178 media/filters/ffmpeg_video_decoder.cc:178: FlushBuffers(); flush buffer to owner. http://codereview.chromium.org/3014059/diff/68001/38008#newcode193 media/filters/ffmpeg_video_decoder.cc:193: pts_heap_.Pop(); move here, because after flush down, we should have no out-going request to demux. http://codereview.chromium.org/3014059/diff/68001/38008#newcode311 media/filters/ffmpeg_video_decoder.cc:311: fill_buffer_done_callback()->Run(empty_frame); this buffer do not need to be counted, it is not "real" buffer. http://codereview.chromium.org/3014059/diff/68001/38008#newcode313 media/filters/ffmpeg_video_decoder.cc:313: // Fall through, because we still need to keep record of this frame. the input frame is a real frame, which need to be store somewhere. http://codereview.chromium.org/3014059/diff/68001/38008#newcode323 media/filters/ffmpeg_video_decoder.cc:323: DCHECK_NE(state_, kStopped); upon stop, we will not have any buffer exchagne. http://codereview.chromium.org/3014059/diff/68001/38008#newcode327 media/filters/ffmpeg_video_decoder.cc:327: frame_queue_flushed_.push_back(video_frame); engine are ignorant to pause and flush, we will cache them until the right time to flush it. http://codereview.chromium.org/3014059/diff/68001/38008#newcode358 media/filters/ffmpeg_video_decoder.cc:358: DCHECK_NE(state_, kStopped); same. http://codereview.chromium.org/3014059/diff/68001/38008#newcode424 media/filters/ffmpeg_video_decoder.cc:424: decode_engine_->FillThisBuffer(video_frame); depends on who own the buffers, we either return it to renderer or return it to engine. http://codereview.chromium.org/3014059/diff/68001/38009 File media/filters/ffmpeg_video_decoder.h (left): http://codereview.chromium.org/3014059/diff/68001/38009#oldcode121 media/filters/ffmpeg_video_decoder.h:121: // Tracks the number of asynchronous reads issued from renderer. remove this because we had maintain this in decode engine http://codereview.chromium.org/3014059/diff/68001/38009 File media/filters/ffmpeg_video_decoder.h (right): http://codereview.chromium.org/3014059/diff/68001/38009#newcode132 media/filters/ffmpeg_video_decoder.h:132: std::deque<scoped_refptr<VideoFrame> > frame_queue_flushed_; use to honor the request that after receive pause() we will never send a outbounding frame/packet. because Pause is not part of engine interface, engine do not have this knowledge of pause; we could not return the frame to engine either in pause state, otherwise it loop unbounded. we will flush this buffer when we enter flush state. http://codereview.chromium.org/3014059/diff/68001/38010 File media/filters/omx_video_decode_engine.cc (left): http://codereview.chromium.org/3014059/diff/68001/38010#oldcode30 media/filters/omx_video_decode_engine.cc:30: mostly not used. http://codereview.chromium.org/3014059/diff/68001/38010#oldcode279 media/filters/omx_video_decode_engine.cc:279: see header file http://codereview.chromium.org/3014059/diff/68001/38010#oldcode304 media/filters/omx_video_decode_engine.cc:304: return; moved to OmxVideoDecodeEngine::FillBufferDoneTask http://codereview.chromium.org/3014059/diff/68001/38010#oldcode308 media/filters/omx_video_decode_engine.cc:308: frame = static_cast<VideoFrame*>(buffer->pAppPrivate); two path had been merged, because we both recycle it. http://codereview.chromium.org/3014059/diff/68001/38010#oldcode612 media/filters/omx_video_decode_engine.cc:612: return; merged two path. http://codereview.chromium.org/3014059/diff/68001/38010 File media/filters/omx_video_decode_engine.cc (right): http://codereview.chromium.org/3014059/diff/68001/38010#newcode150 media/filters/omx_video_decode_engine.cc:150: omx_buffer->nFlags &= ~OMX_BUFFERFLAG_EOS; after EOS, some buffer already has this flag on, you need to clear them, otherwise seek do not work. http://codereview.chromium.org/3014059/diff/68001/38010#newcode169 media/filters/omx_video_decode_engine.cc:169: return; flush in a state other than executing is not defined, we just callback. http://codereview.chromium.org/3014059/diff/68001/38010#newcode187 media/filters/omx_video_decode_engine.cc:187: flush_pending_ = true; mark the flag to start flushing later. http://codereview.chromium.org/3014059/diff/68001/38010#newcode192 media/filters/omx_video_decode_engine.cc:192: DCHECK_EQ(input_pending_request_, 0); only start flushing when we do not have any pending read to demux. http://codereview.chromium.org/3014059/diff/68001/38010#newcode241 media/filters/omx_video_decode_engine.cc:241: if (port == input_port_) { we now flush port one by one , instead of OMX_ALL, to make the flow more predictable. http://codereview.chromium.org/3014059/diff/68001/38010#newcode272 media/filters/omx_video_decode_engine.cc:272: InitialReadBuffer(); populate the buffers to start preroll http://codereview.chromium.org/3014059/diff/68001/38010#newcode295 media/filters/omx_video_decode_engine.cc:295: if (kClientRunning == client_state_ || kClientFlushing == client_state_) { probably we will never meet kClientRunning if we use new pipeline. http://codereview.chromium.org/3014059/diff/68001/38010#newcode323 media/filters/omx_video_decode_engine.cc:323: if (kClientFlushing == client_state_ && !uses_egl_image_) return; keep the frame to ourself if we are flushing and we are the owner http://codereview.chromium.org/3014059/diff/68001/38010#newcode611 media/filters/omx_video_decode_engine.cc:611: } do not drop the frame in any case. http://codereview.chromium.org/3014059/diff/68001/38010#newcode802 media/filters/omx_video_decode_engine.cc:802: } delay free up buffer if some are still used in compoenent. http://codereview.chromium.org/3014059/diff/68001/38010#newcode839 media/filters/omx_video_decode_engine.cc:839: // TODO(jiesun): Make sure this works. REMOVE ME http://codereview.chromium.org/3014059/diff/68001/38010#newcode901 media/filters/omx_video_decode_engine.cc:901: buffer->pAppPrivate = video_frame.get(); buffer pool for system memory path. http://codereview.chromium.org/3014059/diff/68001/38010#newcode1045 media/filters/omx_video_decode_engine.cc:1045: } after output eos, we do not accept output request. http://codereview.chromium.org/3014059/diff/68001/38010#newcode1086 media/filters/omx_video_decode_engine.cc:1086: state need to be reset at each seek http://codereview.chromium.org/3014059/diff/68001/38010#newcode1227 media/filters/omx_video_decode_engine.cc:1227: } still need to return buffers, http://codereview.chromium.org/3014059/diff/68001/38011 File media/filters/omx_video_decode_engine.h (left): http://codereview.chromium.org/3014059/diff/68001/38011#oldcode9 media/filters/omx_video_decode_engine.h:9: #include <list> not used. http://codereview.chromium.org/3014059/diff/68001/38011#oldcode23 media/filters/omx_video_decode_engine.h:23: class MessageLoop; included in video_decode_engine.h http://codereview.chromium.org/3014059/diff/68001/38011#oldcode26 media/filters/omx_video_decode_engine.h:26: struct AVStream; remove ffmpeg types. http://codereview.chromium.org/3014059/diff/68001/38011#oldcode30 media/filters/omx_video_decode_engine.h:30: class Buffer; included in video_decode_engine.h http://codereview.chromium.org/3014059/diff/68001/38011#oldcode85 media/filters/omx_video_decode_engine.h:85: const OmxConfigurator::MediaFormat& output_format); defined in event handler interface callback. but use differnt parameters, need to refactor this. http://codereview.chromium.org/3014059/diff/68001/38011#oldcode240 media/filters/omx_video_decode_engine.h:240: std::queue<OMX_BUFFERHEADERTYPE*> available_output_frames_; merged two path to use output_frames_ , so this is not used anymore. http://codereview.chromium.org/3014059/diff/68001/38011#oldcode245 media/filters/omx_video_decode_engine.h:245: bool input_port_enabled_; not acutally used. http://codereview.chromium.org/3014059/diff/68001/38011 File media/filters/omx_video_decode_engine.h (right): http://codereview.chromium.org/3014059/diff/68001/38011#newcode177 media/filters/omx_video_decode_engine.h:177: used to buffer recycle system memory based OpenMax path, create a media::VideoFrame out of a OMX_BUFFERHEADTYPE. http://codereview.chromium.org/3014059/diff/68001/38011#newcode196 media/filters/omx_video_decode_engine.h:196: int output_buffers_at_component_; used for identify flushing conditions http://codereview.chromium.org/3014059/diff/68001/38011#newcode197 media/filters/omx_video_decode_engine.h:197: int output_pending_request_; actually count the output side of buffer exchange . http://codereview.chromium.org/3014059/diff/68001/38011#newcode225 media/filters/omx_video_decode_engine.h:225: bool need_free_output_buffers_; make resource reclaim more reasonable for output buffers. http://codereview.chromium.org/3014059/diff/68001/38012 File media/filters/video_renderer_base.cc (left): http://codereview.chromium.org/3014059/diff/68001/38012#oldcode27 media/filters/video_renderer_base.cc:27: static const size_t kMaxFrames = 4; moved to header #include "media/base/limits.h" http://codereview.chromium.org/3014059/diff/68001/38012#oldcode130 media/filters/video_renderer_base.cc:130: // TODO(jiesun): move this to flush. no need here with new pipeline. http://codereview.chromium.org/3014059/diff/68001/38012#oldcode330 media/filters/video_renderer_base.cc:330: if (uses_egl_image() && merged two paths. http://codereview.chromium.org/3014059/diff/68001/38012#oldcode352 media/filters/video_renderer_base.cc:352: if (state_ == kStopped || !current_frame_.get() || stop is not needed, because before stop, we had flushed, current frame is NULL. http://codereview.chromium.org/3014059/diff/68001/38012#oldcode360 media/filters/video_renderer_base.cc:360: state_ == kFlushing || state_ == kEnded); test the inverse because we added states. http://codereview.chromium.org/3014059/diff/68001/38012#oldcode392 media/filters/video_renderer_base.cc:392: // TODO(jiesun): Remove this when flush before stop landed! will not happen in new pipeline http://codereview.chromium.org/3014059/diff/68001/38012#oldcode414 media/filters/video_renderer_base.cc:414: DCHECK(seek_callback_.get()); we could get here before Seek is called. http://codereview.chromium.org/3014059/diff/68001/38012 File media/filters/video_renderer_base.cc (right): http://codereview.chromium.org/3014059/diff/68001/38012#newcode81 media/filters/video_renderer_base.cc:81: scoped_ptr<FilterCallback> c(callback); flushed => prerolled => kplaying http://codereview.chromium.org/3014059/diff/68001/38012#newcode88 media/filters/video_renderer_base.cc:88: DCHECK(state_ != kUninitialized || state_ == kError); pause could be called any time as in new pipeline. except uninitialized. http://codereview.chromium.org/3014059/diff/68001/38012#newcode105 media/filters/video_renderer_base.cc:105: DCHECK_EQ(pending_reads_, 0); because flush before stop http://codereview.chromium.org/3014059/diff/68001/38012#newcode135 media/filters/video_renderer_base.cc:135: DCHECK(kPrerolled == state_ || kFlushed == state_ || kSeeking == state_); race condition between filters to receive SeekTask(). we could receive buffer from decoder before seek() is called on us. so we do: kFlushed => (receive one buffer ) => kSeeking => ( enought buffer) => kPrerolled. ) http://codereview.chromium.org/3014059/diff/68001/38012#newcode137 media/filters/video_renderer_base.cc:137: if (state_ == kPrerolled) { already prerolled, we could callback immediately. http://codereview.chromium.org/3014059/diff/68001/38012#newcode141 media/filters/video_renderer_base.cc:141: state_ = kSeeking; otherwise we are either kFlushed or kSeeking ( but without enough buffers), we save the callback and callback later. http://codereview.chromium.org/3014059/diff/68001/38012#newcode143 media/filters/video_renderer_base.cc:143: seek_timestamp_ = time; MOVE ME OUT OF THE ELSE. http://codereview.chromium.org/3014059/diff/68001/38012#newcode182 media/filters/video_renderer_base.cc:182: state_ = kFlushed; kPaused is now used for Pause(). since we had a initial Seek, we consider ourself flushed, because we have not populate buffers yet. http://codereview.chromium.org/3014059/diff/68001/38012#newcode354 media/filters/video_renderer_base.cc:354: state_ = kSeeking; the first time we receive a video frame after kFlushed, ( either at initial time , or after a sequence of seek) we will consider we are seeking now, this is due to decoder reach seek first. http://codereview.chromium.org/3014059/diff/68001/38012#newcode358 media/filters/video_renderer_base.cc:358: if (frame.get() && !frame->IsEndOfStream()) this is a real buffer, we maintain the count of buffers. http://codereview.chromium.org/3014059/diff/68001/38012#newcode363 media/filters/video_renderer_base.cc:363: if (state_ == kPaused || state_ == kFlushing) { in these state, we will just keep record of video frames. http://codereview.chromium.org/3014059/diff/68001/38012#newcode369 media/filters/video_renderer_base.cc:369: if (state_ == kFlushing && pending_paint_ == false) excluding kPause, because in pause state, we will never transfer out-bounding buffer. http://codereview.chromium.org/3014059/diff/68001/38012#newcode382 media/filters/video_renderer_base.cc:382: DCHECK(frames_queue_ready_.size() <= Limits::kMaxVideoFrames); do not use DCHECK_LE because weired Link error. http://codereview.chromium.org/3014059/diff/68001/38012#newcode392 media/filters/video_renderer_base.cc:392: state_ = kPrerolled; remove multiple meaning of kPaused state. http://codereview.chromium.org/3014059/diff/68001/38012#newcode418 media/filters/video_renderer_base.cc:418: void VideoRendererBase::ReadInput(scoped_refptr<VideoFrame> frame) { add this function ( protected ) to be access by derived, see gles_video_renderer for details. http://codereview.chromium.org/3014059/diff/68001/38012#newcode468 media/filters/video_renderer_base.cc:468: DCHECK(frames_queue_done_.size() == Limits::kMaxVideoFrames); check all buffers are return to owners. http://codereview.chromium.org/3014059/diff/68001/38013 File media/filters/video_renderer_base.h (right): http://codereview.chromium.org/3014059/diff/68001/38013#newcode161 media/filters/video_renderer_base.h:161: kPrerolled, we had reuse kPaused in different places. this cause some kind of race condition. http://codereview.chromium.org/3014059/diff/68001/38013#newcode185 media/filters/video_renderer_base.h:185: int pending_reads_; the name is no longer valid. it maybe negative depends on who provides buffer. http://codereview.chromium.org/3014059/diff/68001/38014 File media/filters/video_renderer_base_unittest.cc (left): http://codereview.chromium.org/3014059/diff/68001/38014#oldcode190 media/filters/video_renderer_base_unittest.cc:190: EXPECT_EQ(0u, read_queue_.size()); now decoder is the one who populate the video frames. http://codereview.chromium.org/3014059/diff/68001/38014 File media/filters/video_renderer_base_unittest.cc (right): http://codereview.chromium.org/3014059/diff/68001/38014#newcode209 media/filters/video_renderer_base_unittest.cc:209: honor new pipeline state transition.
Looking pretty good to me. My biggest suggestion is to comment the code using your annotations because they are very helpful to understand the code. http://codereview.chromium.org/3014059/diff/68001/38004 File media/base/limits.h (right): http://codereview.chromium.org/3014059/diff/68001/38004#newcode18 media/base/limits.h:18: // total number of video frames which are populating in the pipeline. nit: should be Total. Also empty line before the comments. http://codereview.chromium.org/3014059/diff/68001/38005 File media/filters/ffmpeg_video_allocator.h (left): http://codereview.chromium.org/3014059/diff/68001/38005#oldcode30 media/filters/ffmpeg_video_allocator.h:30: ~RefCountedAVFrame() { DCHECK_EQ(usage_count_, 0); } On 2010/08/27 00:09:27, jiesun wrote: > remove this because the way ffmpeg-mt handle release buffer in delayed fashion. > I will look into how to put it back. Sounds good. Add a TODO here. http://codereview.chromium.org/3014059/diff/68001/38006 File media/filters/ffmpeg_video_decode_engine.cc (right): http://codereview.chromium.org/3014059/diff/68001/38006#newcode93 media/filters/ffmpeg_video_decode_engine.cc:93: info.provides_buffers_ = true; On 2010/08/27 00:09:27, jiesun wrote: > as suggested , we always let decoder to provides buffer. > in this sense, we should remove providesbuffer(), but how about when we make egl > actually works before we do this? now Egl path still work. It's fine leaving the providesbuffer(), that can be fixed later. http://codereview.chromium.org/3014059/diff/68001/38006#newcode117 media/filters/ffmpeg_video_decode_engine.cc:117: frame_queue_available_.size() == Limits::kMaxVideoFrames) { On 2010/08/27 00:09:27, jiesun wrote: > if we do not have enough buffers , we will report error too. This comment is really good, should put it to the code. http://codereview.chromium.org/3014059/diff/68001/38006#newcode153 media/filters/ffmpeg_video_decode_engine.cc:153: if (!pending_output_buffers_ && !pending_input_buffers_) On 2010/08/27 00:09:27, jiesun wrote: > try to finish flushing. This comment is really good, should put it to the code. http://codereview.chromium.org/3014059/diff/68001/38006#newcode156 media/filters/ffmpeg_video_decode_engine.cc:156: DecodeFrame(buffer); On 2010/08/27 00:09:27, jiesun wrote: > otherwise try to decode this buffer. This comment is really good, should put it to the code. http://codereview.chromium.org/3014059/diff/68001/38006#newcode161 media/filters/ffmpeg_video_decode_engine.cc:161: CHECK(frame.get() && !frame->IsEndOfStream()); USe DCHECK instead of CHECK. http://codereview.chromium.org/3014059/diff/68001/38006#newcode167 media/filters/ffmpeg_video_decode_engine.cc:167: allocator_->DisplayDone(codec_context_, frame); On 2010/08/27 00:09:27, jiesun wrote: > return this frame to available pool after display This comment is really good, should put it to the code. http://codereview.chromium.org/3014059/diff/68001/38006#newcode173 media/filters/ffmpeg_video_decode_engine.cc:173: OnFlushDone(); On 2010/08/27 00:09:27, jiesun wrote: > try to finish flushing This comment is really good, should put it to the code. http://codereview.chromium.org/3014059/diff/68001/38006#newcode175 media/filters/ffmpeg_video_decode_engine.cc:175: ReadInput(); On 2010/08/27 00:09:27, jiesun wrote: > if we already deliver EOS to renderer, we stop reading new input. This comment is really good, should put it to the code. http://codereview.chromium.org/3014059/diff/68001/38006#newcode219 media/filters/ffmpeg_video_decode_engine.cc:219: event_handler_->OnFillBufferCallback(video_frame); On 2010/08/27 00:09:27, jiesun wrote: > when we flushing codec with empty input packet, the first frame_decoded == 0 > signal output frame had been > drained, we mark the flag. otherwise we read it again. This comment is really good, should put it to the code. http://codereview.chromium.org/3014059/diff/68001/38006#newcode260 media/filters/ffmpeg_video_decode_engine.cc:260: CHECK(frame_queue_available_.size()); Use DCHECK instead of CHECK. http://codereview.chromium.org/3014059/diff/68001/38006#newcode271 media/filters/ffmpeg_video_decode_engine.cc:271: video_frame = allocator_->DecodeDone(codec_context_, av_frame_.get()); On 2010/08/27 00:09:27, jiesun wrote: > get frame from allocator which associate with av_frame_.. Awsome, we should keep this comment in the code. http://codereview.chromium.org/3014059/diff/68001/38006#newcode294 media/filters/ffmpeg_video_decode_engine.cc:294: OnFlushDone(); On 2010/08/27 00:09:27, jiesun wrote: > try to finish flushing immediately. Keep this comment, saying that if there's no pending i/o we are done flushing. http://codereview.chromium.org/3014059/diff/68001/38006#newcode299 media/filters/ffmpeg_video_decode_engine.cc:299: DCHECK_EQ(pending_output_buffers_, 0); On 2010/08/27 00:09:27, jiesun wrote: > DCHECKs probably not needed They are good to have. http://codereview.chromium.org/3014059/diff/68001/38006#newcode309 media/filters/ffmpeg_video_decode_engine.cc:309: // TODO(jiesun): move kMaxFrame outside of video_render_base.cc. You've done this already so remove this todo. http://codereview.chromium.org/3014059/diff/68001/38006#newcode312 media/filters/ffmpeg_video_decode_engine.cc:312: ReadInput(); On 2010/08/27 00:09:27, jiesun wrote: > Seek() is where are start preroll. Keep this comment, say we are going to preroll. http://codereview.chromium.org/3014059/diff/68001/38008 File media/filters/ffmpeg_video_decoder.cc (right): http://codereview.chromium.org/3014059/diff/68001/38008#newcode327 media/filters/ffmpeg_video_decoder.cc:327: frame_queue_flushed_.push_back(video_frame); Does this only happens when the renderer provides buffer? We should have already called Flush() to the decode engine, it shouldn't return us anything. If true please put a DCHECK here, e.g. DCHECK_FALSE(ProvidesBuffer()); My guess is that we receive OnFillBufferCallback even for ffmpeg video decode engine because the engine can post a task to call OnFillBufferCallback, so we still need to give it back to the engine. http://codereview.chromium.org/3014059/diff/68001/38012 File media/filters/video_renderer_base.cc (right): http://codereview.chromium.org/3014059/diff/68001/38012#newcode135 media/filters/video_renderer_base.cc:135: DCHECK(kPrerolled == state_ || kFlushed == state_ || kSeeking == state_); This comment is really good, can you put it to the code? http://codereview.chromium.org/3014059/diff/68001/38012#newcode141 media/filters/video_renderer_base.cc:141: state_ = kSeeking; On 2010/08/27 00:09:27, jiesun wrote: > otherwise we are either kFlushed or kSeeking ( but without enough buffers), we > save the callback and callback later. Keep this coment in the code too. http://codereview.chromium.org/3014059/diff/68001/38012#newcode182 media/filters/video_renderer_base.cc:182: state_ = kFlushed; On 2010/08/27 00:09:27, jiesun wrote: > kPaused is now used for Pause(). > since we had a initial Seek, we consider ourself flushed, because we have not > populate buffers yet. Keep this comment in the code too. http://codereview.chromium.org/3014059/diff/68001/38012#newcode354 media/filters/video_renderer_base.cc:354: state_ = kSeeking; On 2010/08/27 00:09:27, jiesun wrote: > the first time we receive a video frame after kFlushed, ( either at initial time > , or after a sequence of seek) we will consider we are seeking now, this is due > to decoder reach seek first. Can we not transition the state but just keep the buffer? It doesn't seem right to do this without an explicit signal like seek(). http://codereview.chromium.org/3014059/diff/68001/38012#newcode369 media/filters/video_renderer_base.cc:369: if (state_ == kFlushing && pending_paint_ == false) On 2010/08/27 00:09:27, jiesun wrote: > excluding kPause, because in pause state, we will never transfer out-bounding > buffer. Please put this comment in the code. http://codereview.chromium.org/3014059/diff/68001/38012#newcode382 media/filters/video_renderer_base.cc:382: DCHECK(frames_queue_ready_.size() <= Limits::kMaxVideoFrames); On 2010/08/27 00:09:27, jiesun wrote: > do not use DCHECK_LE because weired Link error. It needs a static cast. http://codereview.chromium.org/3014059/diff/68001/38012#newcode420 media/filters/video_renderer_base.cc:420: CHECK(frame.get() && !frame->IsEndOfStream()); Use DCHECK instead of CHECK. http://codereview.chromium.org/3014059/diff/68001/38013 File media/filters/video_renderer_base.h (right): http://codereview.chromium.org/3014059/diff/68001/38013#newcode161 media/filters/video_renderer_base.h:161: kPrerolled, Please put some comments here why how and why we split kPaused into kPrerolled and kFlushed.
PHAL. 1. move the annotation to comments. 2. add state diagram. 3. rebase. http://codereview.chromium.org/3014059/diff/68001/38004 File media/base/limits.h (right): http://codereview.chromium.org/3014059/diff/68001/38004#newcode18 media/base/limits.h:18: // total number of video frames which are populating in the pipeline. On 2010/08/27 23:41:31, Alpha wrote: > nit: should be Total. Also empty line before the comments. > Done. http://codereview.chromium.org/3014059/diff/68001/38005 File media/filters/ffmpeg_video_allocator.h (left): http://codereview.chromium.org/3014059/diff/68001/38005#oldcode30 media/filters/ffmpeg_video_allocator.h:30: ~RefCountedAVFrame() { DCHECK_EQ(usage_count_, 0); } On 2010/08/27 23:41:31, Alpha wrote: > On 2010/08/27 00:09:27, jiesun wrote: > > remove this because the way ffmpeg-mt handle release buffer in delayed > fashion. > > I will look into how to put it back. > > Sounds good. Add a TODO here. Done. http://codereview.chromium.org/3014059/diff/68001/38006 File media/filters/ffmpeg_video_decode_engine.cc (right): http://codereview.chromium.org/3014059/diff/68001/38006#newcode161 media/filters/ffmpeg_video_decode_engine.cc:161: CHECK(frame.get() && !frame->IsEndOfStream()); On 2010/08/27 23:41:31, Alpha wrote: > USe DCHECK instead of CHECK. Done. http://codereview.chromium.org/3014059/diff/68001/38006#newcode167 media/filters/ffmpeg_video_decode_engine.cc:167: allocator_->DisplayDone(codec_context_, frame); On 2010/08/27 00:09:27, jiesun wrote: > return this frame to available pool after display Done. http://codereview.chromium.org/3014059/diff/68001/38006#newcode173 media/filters/ffmpeg_video_decode_engine.cc:173: OnFlushDone(); On 2010/08/27 23:41:31, Alpha wrote: > On 2010/08/27 00:09:27, jiesun wrote: > > try to finish flushing > > This comment is really good, should put it to the code. Done. http://codereview.chromium.org/3014059/diff/68001/38006#newcode175 media/filters/ffmpeg_video_decode_engine.cc:175: ReadInput(); On 2010/08/27 00:09:27, jiesun wrote: > if we already deliver EOS to renderer, we stop reading new input. Done. http://codereview.chromium.org/3014059/diff/68001/38008 File media/filters/ffmpeg_video_decoder.cc (right): http://codereview.chromium.org/3014059/diff/68001/38008#newcode327 media/filters/ffmpeg_video_decoder.cc:327: frame_queue_flushed_.push_back(video_frame); I think this happened when decoder owns buffer, because engine do not have pause(). it had no idea when to stop outgoing transfer. so it keep posting buffer to renderer. since flush is done in parallel. there are some race condition if renderer or decoder get that command first. assume renderer had return all buffer (count == 0 )and call flush done. but there maybe still one buffer delivered to it in message queue (pending). pause is used to resolve this dilemma. but since engine do not have pause(); decoder had to honor the protocol of stop deliver out bounding transfer... once every filter enter pause() and pipeline know the following stage, the ONLY "data transfer direction" is "to owner". if decoder own buffers, the only transfer direction is renderer=>decoder. therefore after renderer call flush done, it will be sure that no one will deliver it any buffer unless it is seek. http://codereview.chromium.org/3014059/diff/68001/38012 File media/filters/video_renderer_base.cc (right): http://codereview.chromium.org/3014059/diff/68001/38012#newcode141 media/filters/video_renderer_base.cc:141: state_ = kSeeking; On 2010/08/27 23:41:31, Alpha wrote: > On 2010/08/27 00:09:27, jiesun wrote: > > otherwise we are either kFlushed or kSeeking ( but without enough buffers), we > > save the callback and callback later. > > Keep this coment in the code too. > Done. http://codereview.chromium.org/3014059/diff/68001/38012#newcode182 media/filters/video_renderer_base.cc:182: state_ = kFlushed; On 2010/08/27 23:41:31, Alpha wrote: > On 2010/08/27 00:09:27, jiesun wrote: > > kPaused is now used for Pause(). > > since we had a initial Seek, we consider ourself flushed, because we have not > > populate buffers yet. > > Keep this comment in the code too. Done. http://codereview.chromium.org/3014059/diff/68001/38012#newcode354 media/filters/video_renderer_base.cc:354: state_ = kSeeking; we are doing some special logic in this function when we are "seeking", so we had to change it here http://codereview.chromium.org/3014059/diff/85001/86010 File media/filters/video_renderer_base.h (left): http://codereview.chromium.org/3014059/diff/85001/86010#oldcode152 media/filters/video_renderer_base.h:152: // State transition Diagram of this class: // [kUninitialized] -------> [kError] // | // | Initialize() // V All frames returned // +------[kFlushed]<----------------------[kFlushing] // | | Seek() or upon ^ // | V got first frame | // | [kSeeking] | Flush() // | | | // | V Got enough frames | // | [kPrerolled]---------------------->[kPaused] // | | Pause() ^ // | V Play() | // | [kPlaying]---------------------------| // | | Pause() ^ // | V Receive EOF frame. | Pause() // | [kEnded]-----------------------------+ // | ^ // | | // +-----> [kStopped] [Any state other than] // [kUninitialized/kError]
LGTM. http://codereview.chromium.org/3014059/diff/68001/38008 File media/filters/ffmpeg_video_decoder.cc (right): http://codereview.chromium.org/3014059/diff/68001/38008#newcode327 media/filters/ffmpeg_video_decoder.cc:327: frame_queue_flushed_.push_back(video_frame); On 2010/08/30 20:14:56, jiesun wrote: > I think this happened when decoder owns buffer, because engine do not have > pause(). it had no idea when to stop outgoing transfer. > > so it keep posting buffer to renderer. since flush is done in parallel. there > are some race condition if renderer or decoder get that command first. > assume renderer had return all buffer (count == 0 )and call flush done. but > there maybe still one buffer delivered to it in message queue (pending). > > pause is used to resolve this dilemma. but since engine do not have pause(); > decoder had to honor the protocol of stop deliver out bounding transfer... once > every filter enter pause() and pipeline know the following stage, the ONLY "data > transfer direction" is "to owner". if decoder own buffers, the only transfer > direction is renderer=>decoder. therefore after renderer call flush done, it > will be sure that no one will deliver it any buffer unless it is seek. > > I see. Do you think it makes sense to add Pause() to the video decode engine. It may be nicer for the VideoDecodeEngine to match the interface of a VideoDecoder since VideoDecoder dosen't need to handle additional states that VideoDecodeEngine doesn't have. We don't need to do this in this patch but something to think about.
I had been trying very hard to match the interface with pepper video interface. (with the exception of Seek() for preroll.). we probably could remove seek() from engine interface too, as long as we preroll in decoder class. OpenMax will break if I do it now. because it assume input buffer count == request count. So probably we need to refactor that later. Most likely not me. One more IMPORTANT note is : ipc_video_decoder maybe need a little adjustment. At least preroll now is done by Decoder now. I wonder if someone is working on that now? I could do that after this.
I don't think anyone is doing the prerolling for ipc_video_decoder except me hooking it up with gles2 context. I won't be fixing the ipc video decode code except for adding small stuff like textures and stuff, so feel free to take it.
http://codereview.chromium.org/3014059/diff/87001/88001 File media/base/limits.h (right): http://codereview.chromium.org/3014059/diff/87001/88001#newcode1 media/base/limits.h:1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. copyright year http://codereview.chromium.org/3014059/diff/87001/88003 File media/filters/ffmpeg_video_decode_engine.cc (right): http://codereview.chromium.org/3014059/diff/87001/88003#newcode67 media/filters/ffmpeg_video_decode_engine.cc:67: DLOG(INFO) << "direct rendering is used"; unindent this block by 1 space http://codereview.chromium.org/3014059/diff/87001/88003#newcode97 media/filters/ffmpeg_video_decode_engine.cc:97: // If we do not have enough buffers , we will report error too. nit: buffers , -> buffers, http://codereview.chromium.org/3014059/diff/87001/88003#newcode98 media/filters/ffmpeg_video_decode_engine.cc:98: bool buffer_allocated = true;; nit: remove extra ; http://codereview.chromium.org/3014059/diff/87001/88003#newcode190 media/filters/ffmpeg_video_decode_engine.cc:190: // If we already deliver EOS to renderer, we stop reading new input. nit: unindent this block by 2 spaces http://codereview.chromium.org/3014059/diff/87001/88003#newcode324 media/filters/ffmpeg_video_decode_engine.cc:324: void FFmpegVideoDecodeEngine::OnFlushDone() { I'd make this: bool TryToFinishPendingFlush() { DCHECK(flush_pending_); if (!pending_input_buffers_ && !pending_output_buffers_) { flush_pending_ = false; event_handler_->OnFlushComplete(); return true; } return false; } ...then call TryToFinishPendingFlush() instead of duplicating the code + comments inside Flush(), EmptyThisBuffer() and FillThisBuffer() http://codereview.chromium.org/3014059/diff/87001/88006 File media/filters/ffmpeg_video_decoder.h (right): http://codereview.chromium.org/3014059/diff/87001/88006#newcode96 media/filters/ffmpeg_video_decoder.h:96: // Flush the output buffers that we had holded in Paused state. had holded -> had held http://codereview.chromium.org/3014059/diff/87001/88009 File media/filters/video_renderer_base.cc (right): http://codereview.chromium.org/3014059/diff/87001/88009#newcode135 media/filters/video_renderer_base.cc:135: // There is a race condition between filters to receive SeekTask(). Do you know if we can fix this rather than implement a workaround inside the filters? http://codereview.chromium.org/3014059/diff/87001/88010 File media/filters/video_renderer_base.h (right): http://codereview.chromium.org/3014059/diff/87001/88010#newcode207 media/filters/video_renderer_base.h:207: // We use size_t since we compare against std::deque::size(). size_t comment it out of date
thanks for reviews. I add some comments, PHAL. http://codereview.chromium.org/3014059/diff/87001/88001 File media/base/limits.h (right): http://codereview.chromium.org/3014059/diff/87001/88001#newcode1 media/base/limits.h:1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. On 2010/08/31 02:43:02, scherkus wrote: > copyright year Done. http://codereview.chromium.org/3014059/diff/87001/88003 File media/filters/ffmpeg_video_decode_engine.cc (right): http://codereview.chromium.org/3014059/diff/87001/88003#newcode67 media/filters/ffmpeg_video_decode_engine.cc:67: DLOG(INFO) << "direct rendering is used"; On 2010/08/31 02:43:02, scherkus wrote: > unindent this block by 1 space Done. http://codereview.chromium.org/3014059/diff/87001/88003#newcode97 media/filters/ffmpeg_video_decode_engine.cc:97: // If we do not have enough buffers , we will report error too. On 2010/08/31 02:43:02, scherkus wrote: > nit: buffers , -> buffers, Done. http://codereview.chromium.org/3014059/diff/87001/88003#newcode98 media/filters/ffmpeg_video_decode_engine.cc:98: bool buffer_allocated = true;; On 2010/08/31 02:43:02, scherkus wrote: > nit: remove extra ; Done. http://codereview.chromium.org/3014059/diff/87001/88003#newcode190 media/filters/ffmpeg_video_decode_engine.cc:190: // If we already deliver EOS to renderer, we stop reading new input. On 2010/08/31 02:43:02, scherkus wrote: > nit: unindent this block by 2 spaces Done. http://codereview.chromium.org/3014059/diff/87001/88003#newcode324 media/filters/ffmpeg_video_decode_engine.cc:324: void FFmpegVideoDecodeEngine::OnFlushDone() { On 2010/08/31 02:43:02, scherkus wrote: > I'd make this: > bool TryToFinishPendingFlush() { > DCHECK(flush_pending_); > if (!pending_input_buffers_ && !pending_output_buffers_) { > flush_pending_ = false; > event_handler_->OnFlushComplete(); > return true; > } > return false; > } > > ...then call TryToFinishPendingFlush() instead of duplicating the code + > comments inside Flush(), EmptyThisBuffer() and FillThisBuffer() Done. http://codereview.chromium.org/3014059/diff/87001/88009 File media/filters/video_renderer_base.cc (right): http://codereview.chromium.org/3014059/diff/87001/88009#newcode135 media/filters/video_renderer_base.cc:135: // There is a race condition between filters to receive SeekTask(). Good question! I had thought some solution for this. neither one is ideal. Because we serialize Seek in pipeline and we perform all operation from upper stream to down stream ( except flush is parallel now). the last filter receive the Seek() is renderer. "Preroll" (scheduling some buffer exchange) is where pipeline are starting to populate with buffers. If renderer start preroll, we are sure in this point every filters receive Seek(). that's how current code works. The problem is that we need to decouple two concepts 1. the request to preroll 2. buffer pool availability. We use 1 to resolve the start/stop/seek race condition during state change. we use 2 to mark display had been done and correctly recycle buffers. We use both to flow control. We use a single interface to achieve both. So In my patch, I removed 1 as a method of flow control. use 2 as single flow control method. let decoder to "preroll" which causing the problem. 1. One way to resolve this could be seperate interface for (1)/(2). say: after last filter received seek() it call it upstreams Preroll() to start. 2. Another hacky way is use empty frame request as flow control, discount them when maintaining the buffer count. flow control will be a little complex, because we use both requests and buffer availabilities to control it. these parameters(buffer count vs request count) are potential different because it comes from different filters, making it necessary to queue both and match each other, which is not clean. (but doable) 3. More complex way (from this pipeline) is illustrate here http://msdn.microsoft.com/en-us/library/ms783675.aspx. If we let demux to preroll. (A push model) and reverse the Seek/Start order, ( from renderer to demuxer) , then we are gurantee no buffer exchange before demuxer start to preroll. Audio path is not working this way, therefore I do not think it is an option for me. This way, we drop all the request model, and use buffer available as single way to control flow. how do you think? http://codereview.chromium.org/3014059/diff/87001/88010 File media/filters/video_renderer_base.h (right): http://codereview.chromium.org/3014059/diff/87001/88010#newcode207 media/filters/video_renderer_base.h:207: // We use size_t since we compare against std::deque::size(). On 2010/08/31 02:43:02, scherkus wrote: > size_t comment it out of date Done.
LGTM w/ nits I'll have to dive in a clean up the pipeline/filter relationship at some point (might be doing this next week as part of 20%) http://codereview.chromium.org/3014059/diff/87001/88009 File media/filters/video_renderer_base.cc (right): http://codereview.chromium.org/3014059/diff/87001/88009#newcode135 media/filters/video_renderer_base.cc:135: // There is a race condition between filters to receive SeekTask(). On 2010/08/31 17:36:52, jiesun wrote: > Good question! > I had thought some solution for this. neither one is ideal. > Because we serialize Seek in pipeline and we perform all operation from upper > stream to down stream ( except flush is parallel now). the last filter receive > the Seek() is renderer. "Preroll" (scheduling some buffer exchange) is where > pipeline are starting to populate with buffers. If renderer start preroll, we > are sure in this point every filters receive Seek(). that's how current code > works. > The problem is that we need to decouple two concepts > 1. the request to preroll > 2. buffer pool availability. > We use 1 to resolve the start/stop/seek race condition during state change. we > use 2 to mark display had been done and correctly recycle buffers. > We use both to flow control. > We use a single interface to achieve both. > So In my patch, I removed 1 as a method of flow control. use 2 as single flow > control method. let decoder to "preroll" which causing the problem. > > 1. One way to resolve this could be seperate interface for (1)/(2). say: after > last filter received seek() it call it upstreams Preroll() to start. > 2. Another hacky way is use empty frame request as flow control, discount them > when maintaining the buffer count. flow control will be a little complex, > because we use both requests and buffer availabilities to control it. these > parameters(buffer count vs request count) are potential different because it > comes from different filters, making it necessary to queue both and match each > other, which is not clean. (but doable) > 3. More complex way (from this pipeline) is illustrate here > http://msdn.microsoft.com/en-us/library/ms783675.aspx. If we let demux to > preroll. (A push model) and reverse the Seek/Start order, ( from renderer to > demuxer) , then we are gurantee no buffer exchange before demuxer start to > preroll. > Audio path is not working this way, therefore I do not think it is an option for > me. This way, we drop all the request model, and use buffer available as single > way to control flow. > > how do you think? hrmm sounds like something we'll have to clean up later could you file a bug that includes this summary + link to it from this code? this is some good insight we should keep track of http://codereview.chromium.org/3014059/diff/102001/103003 File media/filters/ffmpeg_video_decode_engine.cc (right): http://codereview.chromium.org/3014059/diff/102001/103003#newcode158 media/filters/ffmpeg_video_decode_engine.cc:158: TryToFinishPendingFlush(); // Try to finish flushing and notify pipeline. nit: remove this comment -- reason why we went with the new name :) http://codereview.chromium.org/3014059/diff/102001/103003#newcode179 media/filters/ffmpeg_video_decode_engine.cc:179: TryToFinishPendingFlush(); // Try to finish flushing and notify pipeline. ditto http://codereview.chromium.org/3014059/diff/102001/103003#newcode307 media/filters/ffmpeg_video_decode_engine.cc:307: TryToFinishPendingFlush(); // Try to finish flushing and notify pipeline. ditto http://codereview.chromium.org/3014059/diff/102001/103003#newcode310 media/filters/ffmpeg_video_decode_engine.cc:310: bool FFmpegVideoDecodeEngine::TryToFinishPendingFlush() { actually I'm sorry.. I wrote that comment without fully thinking this through could you make this void? it looks like the return value will never be used so might as well not confuse other people |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
