Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7044008:
Initial implementation of stream switching in AdaptiveDemuxer. (Closed)
|
Created:
9 years, 7 months ago by acolwell GONE FROM CHROMIUM Modified:
9 years, 6 months ago CC:
chromium-reviews, hclam+watch_chromium.org, sjl, ddorwin+watch_chromium.org, fischman+watch_chromium.org, acolwell GONE FROM CHROMIUM, annacc, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInitial implementation of stream switching in AdaptiveDemuxer.
BUG=82167
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86774
Patch Set 1 : _ #
Total comments: 26
Patch Set 2 : Added documentation & a little cleanup #
Total comments: 14
Patch Set 3 : Addressed CR comments #
Total comments: 4
Patch Set 4 : Fix TODOs #
Total comments: 47
Patch Set 5 : Fixes for Ami's comments #Patch Set 6 : Use RepeatingTimer #
Total comments: 6
Patch Set 7 : Addressing final CR concerns and fixing Lint errors. #
Messages
Total messages: 12 (0 generated)
Pausing CR per offline convo. http://codereview.chromium.org/7044008/diff/3001/media/base/pipeline_status.h File media/base/pipeline_status.h (right): http://codereview.chromium.org/7044008/diff/3001/media/base/pipeline_status.h... media/base/pipeline_status.h:38: typedef base::Callback<void(PipelineStatus)> PipelineStatusCB; Does filters.h need a separate FilterStatusCB? Use this type in base/mock_callback.{h,cc} instead of the explicit type? http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... File media/filters/adaptive_demuxer.cc (right): http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.cc:247: // XXAJC Figure out what to do if this happens during a stream switch. Boom. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... File media/filters/adaptive_demuxer.h (right): http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:28: typedef base::Callback<void(PipelineStatus, base::TimeDelta)> SeekHelperCB; Any reason these need to be outside in namespace scope? http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:28: typedef base::Callback<void(PipelineStatus, base::TimeDelta)> SeekHelperCB; Doco these typedefs. Their naming is leaving me scratching my head, since they're both Callback<>s, so I expect them both to end with CB, but the only difference between their names is the presence of the trailing CB. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:29: typedef base::Callback<void(const base::TimeDelta&, SeekHelperCB)> SeekHelper; TimeDelta is an int64; no need to pass by const&. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:70: // outstanding on | streams_[current_stream_index_] |. Inward-facing spaces around |'s are unnecessary (here and elsewhere). http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:85: // The number of outstanding Read()'s on streams_[current_stream_index_]. ||'ify code in comments (here and elsewhere). http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:95: // The index of the stream we are switching to. Document value while not switching (or that value only makes sense when !switch_cb_.is_null())? http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:98: // Helper object used to seek streams_|switch_index_| to a specific time. brackets instead of pipes http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:101: // The timestamp of the last buffered returned via a Read() callback. s/buffered/buffer/ http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:119: // Change the to a different video stream. English. I think this is the first time you/we enforce that the audio stream can't change during playback. A bunch of the .cc code can be simplified if this is the case, I think. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:122: // Get the ID of the current video stream. This and the next method: are they really necessary? http://codereview.chromium.org/7044008/diff/3001/media/filters/video_renderer... File media/filters/video_renderer_base.cc (right): http://codereview.chromium.org/7044008/diff/3001/media/filters/video_renderer... media/filters/video_renderer_base.cc:320: current_frame_->height() != height_) { I don't imagine we have any test coverage for the case of frame size changing mid-stream. Easy to add? http://codereview.chromium.org/7044008/diff/3001/media/video/ffmpeg_video_dec... File media/video/ffmpeg_video_decode_engine.cc (right): http://codereview.chromium.org/7044008/diff/3001/media/video/ffmpeg_video_dec... media/video/ffmpeg_video_decode_engine.cc:308: extra newline
New patch uploaded with far more documentation. http://codereview.chromium.org/7044008/diff/3001/media/base/pipeline_status.h File media/base/pipeline_status.h (right): http://codereview.chromium.org/7044008/diff/3001/media/base/pipeline_status.h... media/base/pipeline_status.h:38: typedef base::Callback<void(PipelineStatus)> PipelineStatusCB; On 2011/05/19 20:27:37, Ami Fischman wrote: > Does filters.h need a separate FilterStatusCB? No. > Use this type in base/mock_callback.{h,cc} instead of the explicit type? Yes. I'll put this in a separate CL. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... File media/filters/adaptive_demuxer.h (right): http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:28: typedef base::Callback<void(PipelineStatus, base::TimeDelta)> SeekHelperCB; On 2011/05/19 20:27:37, Ami Fischman wrote: > Any reason these need to be outside in namespace scope? Done. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:28: typedef base::Callback<void(PipelineStatus, base::TimeDelta)> SeekHelperCB; On 2011/05/19 20:27:37, Ami Fischman wrote: > Doco these typedefs. > Their naming is leaving me scratching my head, since they're both Callback<>s, > so I expect them both to end with CB, but the only difference between their > names is the presence of the trailing CB. Done. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:29: typedef base::Callback<void(const base::TimeDelta&, SeekHelperCB)> SeekHelper; On 2011/05/19 20:27:37, Ami Fischman wrote: > TimeDelta is an int64; no need to pass by const&. Done. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:70: // outstanding on | streams_[current_stream_index_] |. On 2011/05/19 20:27:37, Ami Fischman wrote: > Inward-facing spaces around |'s are unnecessary (here and elsewhere). Done. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:95: // The index of the stream we are switching to. On 2011/05/19 20:27:37, Ami Fischman wrote: > Document value while not switching (or that value only makes sense when > !switch_cb_.is_null())? Done. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:98: // Helper object used to seek streams_|switch_index_| to a specific time. On 2011/05/19 20:27:37, Ami Fischman wrote: > brackets instead of pipes Done. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:101: // The timestamp of the last buffered returned via a Read() callback. On 2011/05/19 20:27:37, Ami Fischman wrote: > s/buffered/buffer/ Done. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:119: // Change the to a different video stream. On 2011/05/19 20:27:37, Ami Fischman wrote: > English. > I think this is the first time you/we enforce that the audio stream can't change > during playback. A bunch of the .cc code can be simplified if this is the case, > I think. Done. For now I'm constraining code to video only. I envision adding audio switching in the not too distant future. I just didn't want to handle switching both streams in this initial CL. http://codereview.chromium.org/7044008/diff/3001/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:122: // Get the ID of the current video stream. On 2011/05/19 20:27:37, Ami Fischman wrote: > This and the next method: are they really necessary? These were added so a StreamSwitchManager object could initiate switches. I'm sure this interface will change over time, but it was an initial attempt to separate demuxer & stream switching logic from switch decision logic. http://codereview.chromium.org/7044008/diff/3001/media/filters/video_renderer... File media/filters/video_renderer_base.cc (right): http://codereview.chromium.org/7044008/diff/3001/media/filters/video_renderer... media/filters/video_renderer_base.cc:320: current_frame_->height() != height_) { On 2011/05/19 20:27:37, Ami Fischman wrote: > I don't imagine we have any test coverage for the case of frame size changing > mid-stream. Easy to add? Removing the frame size changes for now. It was a quick hack and I didn't fully explore the ramifications of changing the frame size mid-stream. http://codereview.chromium.org/7044008/diff/3001/media/video/ffmpeg_video_dec... File media/video/ffmpeg_video_decode_engine.cc (right): http://codereview.chromium.org/7044008/diff/3001/media/video/ffmpeg_video_dec... media/video/ffmpeg_video_decode_engine.cc:308: On 2011/05/19 20:27:37, Ami Fischman wrote: > extra newline Done.
http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... File media/filters/adaptive_demuxer.cc (right): http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.cc:21: nit: remove blank line http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.cc:24: ~StreamSwitchManager(); virtual http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.cc:100: base::AutoLock auto_lock(lock_); what are we locking ourselves from? which threads are running through these objects? http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.cc:227: read_cb_queue_.push_back(read_callback); want to raise it again since it seems like it might solve a lot of pain... what if we had a push model for demuxer -> decoder? it'd do away with a lot of this silly queueing stuff alternatively, what if we only allowed one outstanding read at any time? could simplify things! http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.cc:269: // XXAJC Figure out what to do if this happens during a stream switch. should these be TODOs / NOTIMPLEMENTED ?
http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... File media/filters/adaptive_demuxer.cc (right): http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.cc:21: On 2011/05/23 18:26:29, scherkus wrote: > nit: remove blank line Done. http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.cc:24: ~StreamSwitchManager(); On 2011/05/23 18:26:29, scherkus wrote: > virtual Done. http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.cc:100: base::AutoLock auto_lock(lock_); On 2011/05/23 18:26:29, scherkus wrote: > what are we locking ourselves from? > > which threads are running through these objects? The pipeline & ffmpeg_demuxer threads were accessing this code. I've made modifications so that the lock isn't needed and this class always runs on the pipeline thread. http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.cc:227: read_cb_queue_.push_back(read_callback); On 2011/05/23 18:26:29, scherkus wrote: > want to raise it again since it seems like it might solve a lot of pain... > > what if we had a push model for demuxer -> decoder? > > it'd do away with a lot of this silly queueing stuff It wouldn't remove queuing. It would just move the queue to the decoder. The push vs pull discussion should happen outside of this forum. > > alternatively, what if we only allowed one outstanding read at any time? That could simplify this code slightly. I'm not sure why the multiple Read()s were allowed in the first place. I could attack that in a follow-up CL. Doing it before hand wouldn't significantly change this CL. It would just change pending_reads to a bool and change the CB queue to a single CB. I don't think that is enough to defer this CL. > > could simplify things! http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.cc:269: // XXAJC Figure out what to do if this happens during a stream switch. On 2011/05/23 18:26:29, scherkus wrote: > should these be TODOs / NOTIMPLEMENTED ? Done.
misc rambly thoughts + nit about TODO is this CL still on hold? http://codereview.chromium.org/7044008/diff/13001/media/filters/adaptive_demu... File media/filters/adaptive_demuxer.cc (right): http://codereview.chromium.org/7044008/diff/13001/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:147: if (MessageLoop::current() != message_loop_) { nothing actionable right now but some food for thought: ajwong and I have been talking about is how to remove boilerplate code such as this Imagine a world where every method starts with: DCHECK_EQ(MessageLoop::current(), message_loop_); ...and reposting is forbidden (i.e., callee is responsible for making sure the method was called on the right thread) it'll work as long as we don't expect our callbacks to by synchronous -- which I think is alright since most of the callbacks today do repost! http://codereview.chromium.org/7044008/diff/13001/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:259: // TODO Figure out what to do if this happens during a stream switch. TODO -> TODO(acolwell):
Fixed TODOs This isn't on hold anymore. It is ready for review. http://codereview.chromium.org/7044008/diff/13001/media/filters/adaptive_demu... File media/filters/adaptive_demuxer.cc (right): http://codereview.chromium.org/7044008/diff/13001/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:147: if (MessageLoop::current() != message_loop_) { On 2011/05/23 23:17:06, scherkus wrote: > nothing actionable right now but some food for thought: ajwong and I have been > talking about is how to remove boilerplate code such as this > > Imagine a world where every method starts with: > DCHECK_EQ(MessageLoop::current(), message_loop_); > > ...and reposting is forbidden (i.e., callee is responsible for making sure the > method was called on the right thread) > > it'll work as long as we don't expect our callbacks to by synchronous -- which I > think is alright since most of the callbacks today do repost! I was going to comment on this when I made the change. It is annoying right now that I have to know that another object has a different thread and can call me on it. It would be nice if say FFmpeg demuxer would wrap callbacks I hand it so that it would always call me back on the thread that I called Read() from. It would change the semantics from "I could call you on a different thread" to "I'll call you back on the thread you called me on." I think the later is WAY better. It is also much easier to do with the new callback system. http://codereview.chromium.org/7044008/diff/13001/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:259: // TODO Figure out what to do if this happens during a stream switch. On 2011/05/23 23:17:06, scherkus wrote: > TODO -> TODO(acolwell): Done.
Thanks for the art. It helped a lot! http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... File media/filters/adaptive_demuxer.h (right): http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:41: // than the requested time. s/than// http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:44: // Wraps a function that performs a seek on a specified stream. The s/stream/demuxer/ This was a core piece of my confusion earlier about what you were doing. Basically call out that Demuxer supports Seek but DemuxerStream doesn't. That's the real reason for all this. http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:46: // SeekFunctionCBparameter specifies the callback we want called when the seek s/CBp/CB p/ http://codereview.chromium.org/7044008/diff/8004/media/filters/adaptive_demux... media/filters/adaptive_demuxer.h:48: typedef base::Callback<void(base::TimeDelta, SeekFunctionCB)> SeekFunction; WDYT of SeekFunctionCB -> SeekDoneCB SeekFunction -> SeekStartFunction/SeekStartCB ? http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... File media/filters/adaptive_demuxer.cc (right): http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:25: void Init(AdaptiveDemuxer* demuxer); FWIW, could be ctor arg. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:76: StreamSwitchManager::~StreamSwitchManager() {} What happens to an outstanding posted task whose time comes, after this manager (and its demuxer) have been shut down / deallocated? Does the task hold a reference preventing refcounting from tearing these down? Is that enough in the face of global teardown (i.e. are we safe from crashing at exit time)? http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:128: if (!switch_pending_) { reverse the test and early-return lets you de-indent body. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:134: if (new_id_index != current_id_index_) { Adding an if (video_ids_.size() < 2) return; to the top of the function lets you de-indent the rest (and incidentally fix the no-impact buglet of starting the timer every 5s even when there's a single or no video streams). http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:150: NewRunnableMethod(this,&StreamSwitchManager::OnSwitchDone, status)); return after this! http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:257: last_buffer_timestamp_ = kNoTimestamp; I'm surprised this doesn't store the seek target time. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:272: // \|/ You might enjoy v and ^ as arrow-heads for ASCII art. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:318: // current stream. Does this mean that if the switched-to stream stalls loading, it'll cause the current stream (and the user-visible state) to pause? http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:350: if (!error_cb.is_null()) { I think you can lose error_cb by initializing error_status to OK and here asking if (error_status != OK) { switch_cb.Run(error_status); return; } ? http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:383: if (start_switch) Is this thread-safe? Just b/c there were no pending read under the lock above, doesn't mean someone else didn't come in and queue up some reads under you, does it? ISTM you need to test & set the switch_pending state under lock. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:394: return (pending_reads_ == 0); Seems strange that this doesn't inspect switch_cb_ at all. Maybe I expect it to be if (pending_reads_ == 0) { DCHECK(switch_cb_.is_null()); return true; } return false; ? http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:445: switch_cb_.Reset(); swap instead of =+Reset()? http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:461: if (stream) { Do we really allow switching to a NULL stream index? Pending reads will never fire if so... http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:577: stream->ChangeCurrentStream( Once you set switch_pending_=true under lock above, isn't it safe to simply use video_stream_ here? http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:590: base::AutoLock auto_lock(lock_); Is the return value from this function really something that can vary during the lifetime of the AdaptiveDemuxer? (I'm wondering about the need for the lock; if you remove it, please document why in a comment). http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:610: current_video_demuxer_index_ = new_stream_index; Whee! http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:842: seek_cb.Run(status, base::TimeDelta()); The ASCII art in this file (and this sort of error handling) looks to me like a great TOC of a test suite :) http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:881: seek_cb.Run(PIPELINE_OK, seek_point); FWIW method body can just be seek_cb.Run(status, seek_point); so I think this method can be elided entirely (just Bind the expression above in place of Binding this method). http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... File media/filters/adaptive_demuxer.h (right): http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.h:36: // AdaptiveDemuxerStream::ChangeCurrentStream(). Add a pointer to the ascii art below?
few qs http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... File media/filters/adaptive_demuxer.cc (right): http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:57: bool switch_timer_running_; may want to consider using OneShotTimer() from base/timer.h instead of hand-rolling one http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:272: // \|/ On 2011/05/24 06:14:12, Ami Fischman wrote: > You might enjoy v and ^ as arrow-heads for ASCII art. hahah I was going to make that same comment but wondered if that was going overboard http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:322: const PipelineStatusCB& switch_cb) { q: what's our goal w/ PipelineStatus + respective callbacks? would we be better off with AdaptiveStreamingStatus error codes or are we fine stashing everything into PipelineStatus? I'm indifferent. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:819: // yet. Seek to the specified time to force index data to be loaded. OOC does this happen due to deferred CUES fetching?
Addressed Ami's & Andrew's comments. Fixed everything but adding unit tests. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... File media/filters/adaptive_demuxer.cc (right): http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:25: void Init(AdaptiveDemuxer* demuxer); On 2011/05/24 06:14:12, Ami Fischman wrote: > FWIW, could be ctor arg. Done. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:57: bool switch_timer_running_; On 2011/05/24 21:38:37, scherkus wrote: > may want to consider using OneShotTimer() from base/timer.h instead of > hand-rolling one Done. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:76: StreamSwitchManager::~StreamSwitchManager() {} On 2011/05/24 06:14:12, Ami Fischman wrote: > What happens to an outstanding posted task whose time comes, after this manager > (and its demuxer) have been shut down / deallocated? Does the task hold a > reference preventing refcounting from tearing these down? Is that enough in the > face of global teardown (i.e. are we safe from crashing at exit time)? The task holds a reference that prevents destruction. I've made sure that both methods that are referenced by PostXXTask() (OnSwitchDone && OnSwitchTimer) can safely run if the demuxer has been destroyed. Stop() sets demuxer_ to NULL and OnSwitchTimer() doesn't allow a stream switch and doesn't reschedule the timer if demuxer_ is NULL. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:128: if (!switch_pending_) { On 2011/05/24 06:14:12, Ami Fischman wrote: > reverse the test and early-return lets you de-indent body. Done. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:134: if (new_id_index != current_id_index_) { On 2011/05/24 06:14:12, Ami Fischman wrote: > Adding an if (video_ids_.size() < 2) return; to the top of the function lets you > de-indent the rest (and incidentally fix the no-impact buglet of starting the > timer every 5s even when there's a single or no video streams). Done. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:150: NewRunnableMethod(this,&StreamSwitchManager::OnSwitchDone, status)); On 2011/05/24 06:14:12, Ami Fischman wrote: > return after this! Good catch. Done http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:257: last_buffer_timestamp_ = kNoTimestamp; On 2011/05/24 06:14:12, Ami Fischman wrote: > I'm surprised this doesn't store the seek target time. Good catch. Updated the method signature to pass the seek target time. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:272: // \|/ On 2011/05/24 06:14:12, Ami Fischman wrote: > You might enjoy v and ^ as arrow-heads for ASCII art. oh yeah.. duh. Done http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:318: // current stream. On 2011/05/24 06:14:12, Ami Fischman wrote: > Does this mean that if the switched-to stream stalls loading, it'll cause the > current stream (and the user-visible state) to pause? Yes. In follow up CLs this will likely be tuned to minimize user-visible impact. We'll likely seek the switched-to stream a few seconds into the future and buffer a little data on that stream before doing the actual switch. I just wanted to keep the initial CL as simple as possible. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:350: if (!error_cb.is_null()) { On 2011/05/24 06:14:12, Ami Fischman wrote: > I think you can lose error_cb by initializing error_status to OK and here asking > if (error_status != OK) { > switch_cb.Run(error_status); > return; > } > ? Removed error_cb & error_status per our discussion. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:383: if (start_switch) On 2011/05/24 06:14:12, Ami Fischman wrote: > Is this thread-safe? Just b/c there were no pending read under the lock above, > doesn't mean someone else didn't come in and queue up some reads under you, does > it? > ISTM you need to test & set the switch_pending state under lock. I'm pretty sure this is safe. switch_pending state (ie !switch_cb_.is_null()) only gets changed in ChangeCurrentStream() & OnSwitchSeekDone(). Once the switch_pending state is set in ChangeCurrentStream(), Read() will not make streams_[current_stream_index_]->Read() calls. We don't have to worry about the the pending state changing outside of lock and since we are deferring Reads() we don't have to worry about pending_reads_ being modified. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:394: return (pending_reads_ == 0); On 2011/05/24 06:14:12, Ami Fischman wrote: > Seems strange that this doesn't inspect switch_cb_ at all. > Maybe I expect it to be > if (pending_reads_ == 0) { > DCHECK(switch_cb_.is_null()); > return true; > } > return false; > ? Done. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:445: switch_cb_.Reset(); On 2011/05/24 06:14:12, Ami Fischman wrote: > swap instead of =+Reset()? no swap() method. :( http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:461: if (stream) { On 2011/05/24 06:14:12, Ami Fischman wrote: > Do we really allow switching to a NULL stream index? > Pending reads will never fire if so... No. Removed if() and added DCHECK http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:577: stream->ChangeCurrentStream( On 2011/05/24 06:14:12, Ami Fischman wrote: > Once you set switch_pending_=true under lock above, isn't it safe to simply use > video_stream_ here? Yes I think so. Removed stream variable. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:590: base::AutoLock auto_lock(lock_); On 2011/05/24 06:14:12, Ami Fischman wrote: > Is the return value from this function really something that can vary during the > lifetime of the AdaptiveDemuxer? > (I'm wondering about the need for the lock; if you remove it, please document > why in a comment). This info shouldn't change during playback. I've removed this method and moved the code to the constructor. This was only needed by StreamSwitchManager. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:819: // yet. Seek to the specified time to force index data to be loaded. On 2011/05/24 21:38:37, scherkus wrote: > OOC does this happen due to deferred CUES fetching? Yes. It is a temporary hack until we have a WebM demuxer or we expose a better way to get index data via FFmpeg. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:881: seek_cb.Run(PIPELINE_OK, seek_point); On 2011/05/24 06:14:12, Ami Fischman wrote: > FWIW method body can just be > seek_cb.Run(status, seek_point); > so I think this method can be elided entirely (just Bind the expression above in > place of Binding this method). Remove all the logic in the function so now it just has seek_cb.Run(status,seek_point). I couldn't figure out the appropriate Bind magic to eliminate the need for this function. base::Bind(&AdaptiveDemuxerStream::SeekFunctionCB::Run, base::Unretained(&seek_cb), seek_point) compiles, but using the reference of a local variable is obviously uncool. I think Callback needs some sort of Apply() template method that converts an N-ary Callback into an (N-M)-ary Callback. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... File media/filters/adaptive_demuxer.h (right): http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.h:36: // AdaptiveDemuxerStream::ChangeCurrentStream(). On 2011/05/24 06:14:12, Ami Fischman wrote: > Add a pointer to the ascii art below? Done.
Pretty much LGTM. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... File media/filters/adaptive_demuxer.cc (right): http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:318: // current stream. On 2011/05/24 23:45:16, acolwell wrote: > On 2011/05/24 06:14:12, Ami Fischman wrote: > > Does this mean that if the switched-to stream stalls loading, it'll cause the > > current stream (and the user-visible state) to pause? > Yes. > > In follow up CLs this will likely be tuned to minimize user-visible impact. > We'll likely seek the switched-to stream a few seconds into the future and > buffer a little data on that stream before doing the actual switch. > > I just wanted to keep the initial CL as simple as possible. Sounds reasonable. Add TODO? http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:445: switch_cb_.Reset(); On 2011/05/24 23:45:16, acolwell wrote: > On 2011/05/24 06:14:12, Ami Fischman wrote: > > swap instead of =+Reset()? > no swap() method. :( new-style callbacks are assignable, so std::swap() should do you. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:881: seek_cb.Run(PIPELINE_OK, seek_point); On 2011/05/24 23:45:16, acolwell wrote: > On 2011/05/24 06:14:12, Ami Fischman wrote: > > FWIW method body can just be > > seek_cb.Run(status, seek_point); > > so I think this method can be elided entirely (just Bind the expression above > in > > place of Binding this method). > Remove all the logic in the function so now it just has > seek_cb.Run(status,seek_point). I couldn't figure out the appropriate Bind magic > to eliminate the need for this function. > base::Bind(&AdaptiveDemuxerStream::SeekFunctionCB::Run, > base::Unretained(&seek_cb), > seek_point) compiles, but using the reference of a > local variable is obviously uncool. I think Callback needs some sort of Apply() > template method that converts an N-ary Callback into an (N-M)-ary Callback. Re: Apply: that's what Bind *is*. Re: ref-of-local-var: it's only a problem b/c you're using Unretained. Does dropping that make your life better? http://codereview.chromium.org/7044008/diff/13002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:462: // Make the Read() calls that were deferred during the stream switch. Unnecessary? http://codereview.chromium.org/7044008/diff/13002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:488: DCHECK_LT(current_audio_demuxer_index_, static_cast<int>(demuxers_.size())); reset() more idiomatic for scoped_refptr? http://codereview.chromium.org/7044008/diff/13002/media/filters/adaptive_demu... File media/filters/adaptive_demuxer.h (right): http://codereview.chromium.org/7044008/diff/13002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.h:60: void OnAdaptiveDemuxerSeek(base::TimeDelta time); s/time/seek_time/ to match elsewhere?
http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... File media/filters/adaptive_demuxer.cc (right): http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:445: switch_cb_.Reset(); On 2011/05/25 05:39:01, Ami Fischman wrote: > On 2011/05/24 23:45:16, acolwell wrote: > > On 2011/05/24 06:14:12, Ami Fischman wrote: > > > swap instead of =+Reset()? > > no swap() method. :( > > new-style callbacks are assignable, so std::swap() should do you. Done. http://codereview.chromium.org/7044008/diff/12002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:881: seek_cb.Run(PIPELINE_OK, seek_point); On 2011/05/25 05:39:01, Ami Fischman wrote: > Re: Apply: that's what Bind *is*. yeah. I wasn't quite thinking straight here. > Re: ref-of-local-var: it's only a problem b/c you're using Unretained. Does > dropping that make your life better? No. If I remove base::Unretained(&), it complains about seek_cb not being a pointer and that it doesn't support AddRef() and Release(). http://codereview.chromium.org/7044008/diff/13002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:462: // Make the Read() calls that were deferred during the stream switch. On 2011/05/25 05:39:01, Ami Fischman wrote: > Unnecessary? Done. http://codereview.chromium.org/7044008/diff/13002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.cc:488: DCHECK_LT(current_audio_demuxer_index_, static_cast<int>(demuxers_.size())); On 2011/05/25 05:39:01, Ami Fischman wrote: > reset() more idiomatic for scoped_refptr? scoped_refptr doesn't have a reset(). http://codereview.chromium.org/7044008/diff/13002/media/filters/adaptive_demu... File media/filters/adaptive_demuxer.h (right): http://codereview.chromium.org/7044008/diff/13002/media/filters/adaptive_demu... media/filters/adaptive_demuxer.h:60: void OnAdaptiveDemuxerSeek(base::TimeDelta time); On 2011/05/25 05:39:01, Ami Fischman wrote: > s/time/seek_time/ to match elsewhere? Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
