Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
237353007:
Refactor VideoRendererImpl to use VideoFrameScheduler. (Closed)
|
Created:
6 years, 8 months ago by scherkus (not reviewing) Modified:
6 years, 4 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor VideoRendererImpl to use VideoFrameScheduler.
This eliminates the internal thread inside of VideoRendererImpl in favour of using a delegate that handles the actual scheduling of frames. This lets different clients use different schedulers for different purposes:
- Unit tests use a testing scheduler, which makes tests easier to write and are no longer multi-threaded
- Performance tests use a clockless scheduler, which lets tests run as fast as possible without dropping frames
- Applications use the MessageLoop-based scheduler, which lets content/ use the compositor thread when available
BUG=110814
Patch Set 1 : #
Total comments: 5
Patch Set 2 : pretty much done #
Total comments: 80
Patch Set 3 : comments #
Total comments: 6
Patch Set 4 : rebase over split out CLs #
Messages
Total messages: 15 (0 generated)
take a skim over my comments in video_renderer_impl.cc the patch isn't too bad but there are a lot of whacky edge cases that we'll have to think through... https://codereview.chromium.org/237353007/diff/40001/media/filters/video_rend... File media/filters/video_renderer_impl.cc (right): https://codereview.chromium.org/237353007/diff/40001/media/filters/video_rend... media/filters/video_renderer_impl.cc:79: // XXX what about scheduled_frames_? I'm pretty sure we may have to wait for all frames to return from scheduled_frames_ before firing the Flush() callback since we may accidentally return a "flushed" frame in OnVideoFrameFinished() back into unscheduled_frames_ Need to think about this some more... https://codereview.chromium.org/237353007/diff/40001/media/filters/video_rend... media/filters/video_renderer_impl.cc:121: // Changing from a zero to a non-zero playback rate can trigger ended. old code would have us fire ended even if playback_rate_ == 0.0f ... but is that right?! I changed it to require being >0.0f ... but I can see how that might be artificial constraint (remember: *inside* the Pipeline we use a rate of 0.0f to implement WMPI::pause()) https://codereview.chromium.org/237353007/diff/40001/media/filters/video_rend... media/filters/video_renderer_impl.cc:341: case kPrerolled: it also turns out that after hitting kPrerolled and having space inside ready_frames_ ... we wouldn't even do the extra read! silly :( https://codereview.chromium.org/237353007/diff/40001/media/filters/video_rend... media/filters/video_renderer_impl.cc:384: ScheduleFirstFrameForImmediateDisplay(); FYI here's the bit where we only display that first frame *AFTER* getting to preroll state I might fix this before landing this CL... https://codereview.chromium.org/237353007/diff/40001/media/filters/video_rend... media/filters/video_renderer_impl.cc:451: if (unscheduled_frames_.empty() && received_end_of_stream_) { I'm pretty sure this will fire ended too soon. We chatted about perhaps using the clock ... alternatively this could be done here as a PostDelayTask() on the current message loop. Thoughts?
this is ready for review (!!!) acolwell, xhwang: can you both take a look? this CL could use some extra eyes
overall looks pretty good. Here is a first round of comments. https://codereview.chromium.org/237353007/diff/120001/content/renderer/media/... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/237353007/diff/120001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:1224: media_loop_, compositor_runner_, scheduler_)), Any reason we can't simply have the proxy own the scheduler_ instead of this class? https://codereview.chromium.org/237353007/diff/120001/media/filters/clockless... File media/filters/clockless_video_frame_scheduler.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/clockless... media/filters/clockless_video_frame_scheduler.cc:24: done_cb.Run(frame, DISPLAYED); Shouldn't this be a PostTask() to avoid reentrancy issues? https://codereview.chromium.org/237353007/diff/120001/media/filters/pipeline_... File media/filters/pipeline_integration_test_base.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/pipeline_... media/filters/pipeline_integration_test_base.cc:249: scoped_ptr<VideoFrameScheduler>(new ClocklessVideoFrameScheduler( nit: Shouldn't we only be using this scheduler if clockless_playback_ is set? https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... File media/filters/test_video_frame_scheduler.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.cc:35: if (!should_reset_) Why? It seems like this could result in a violation of the VideoFrameScheduler interface contract. https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.cc:63: remaining_frames.push_back(scheduled_frames_[i]); You have to do this because you aren't using a priority queue here? Is that just because you want to be able to verify ScheduleVideoFrame() call order? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_fra... File media/filters/video_frame_scheduler.h (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/video_fra... media/filters/video_frame_scheduler.h:39: virtual void Reset() = 0; nit: You might want add a note here warning people that all |done_cb|s will be dispatched within this call and callers should be careful about reentrancy issues. If that is not the intended semantics then I think we should make sure that all implementations do not dispatch the CBs within this call for consistant behavior. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... File media/filters/video_renderer_impl.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:55: DCHECK(state_ != kUninitialized || state_ == kError); nit: Add != kPausing ? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:73: // stream and needs to drain it before flushing it. nit: Is this comment still correct? It confuses me. We need to do this because frames could have arrived in the course of pausing right? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:390: base::TimeTicks current_wall_ticks = tick_clock_->NowTicks(); hmm.. ISTM that pipeline's Clock should be the sole source of timestamp -> wall clock mapping. This essentially creates a second mapping that could potentially introduce drift. Perhaps we need to change get_time_cb_'s signature so we can get the correct wall clock time from pipeline's Clock. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:430: scheduled_frames_.erase(iter); nit: An reason not to do this before the switch since you do it in all branches and you could simply push |frame| in the RESET case. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:444: PipelineStatistics stats; nit: Move this below w/ a bool guard so we don't have to duplicate the code in the switch and we only have to worry about one location where the CB is dispatched? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... File media/filters/video_renderer_impl_unittest.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:226: // Arbitrary value. Has to be larger to cover any timestamp used in tests. nit: s/larger/large/? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:357: EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[0].wall_ticks); String format verification for these too? Either that or a helper function to make this less cluttered looking. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:392: scheduler_->set_should_reset(false); This is for simulating the proxy VFS case right? Any reason not to simply use that class for this test so it is clearer why the pause would get delayed? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:414: EXPECT_EQ(4u, scheduler_->scheduled_frames().size()); ditto. more concise expectations would be nice.
I like this CL. Here are my questions and comments. https://codereview.chromium.org/237353007/diff/120001/content/renderer/media/... File content/renderer/media/webmediaplayer_impl.h (right): https://codereview.chromium.org/237353007/diff/120001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.h:340: scheduler_; // Deleted on |compositor_runner_|. nit: I am not a fan of this format. Is this done by clang-format? Can we just put the comment on the previous line? https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... File media/filters/test_video_frame_scheduler.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.cc:36: return; The API says Reset() should "Causes the scheduler to release all previously scheduled frames. Frames will be returned as RESET." According to this API, caller should only need to call Reset() once to reset the frame scheduler. Therefore, I think the following call sequence violates this API: set_should_reset(false); Reset(); set_should_reset(false); Reset(); Instead, we could probably do something like: HoldReset(); Reset(); // Reset will be pending. SatisfyReset(); https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.cc:43: } Can we just use: RunDoneCBForFrames(TimeDelta::Max(), RESET); ? https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.cc:65: } nit: If the frames are sorted we don't have to go through the whole list. https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... File media/filters/test_video_frame_scheduler.h (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.h:38: void DisplayFrames(base::TimeTicks wall_ticks); s/DisplayFrames/DisplayFramesUpTo/ ? https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.h:41: void DropFrames(base::TimeTicks wall_ticks); ditto about "UpTo" https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.h:52: std::vector<ScheduledFrame> scheduled_frames_; Any reason we don't sort the frames by timestamp? i.e. using the priority queue? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... File media/filters/video_renderer_impl.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:310: } nit: I found it's easier to understand if we just expand some ShouldTransitionToXxx() and TransitionToXxx() functions. But maybe that's just my problem :) https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:395: playback_rate_; So even if delta < 0 we still ScheduleVideoFrame()? Isn't that frame guaranteed to be dropped? Are we trying to make the code cleaner (less branches)? This may be worth a comment. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:409: void VideoRendererImpl::ScheduleFirstFrameForImmediateDisplay() { DCHECK_EQ(state_, kPrerolled); https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:418: } Probably we can have a helper function like ScheduleNextFrame(base::TimeTicks target_wall_ticks) which can be used by both ScheduleVideoFrames() and ScheduleFirstFrameForImmediateDisplay(). https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:452: unscheduled_frames_.push_back(*iter); ha, It surprised me a bit that the "reset" frames will be rescheduled. It might worth a comment at: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide... https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... File media/filters/video_renderer_impl.h (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.h:81: void ScheduleVideoFrames(); Drop the "Video" part, or s/Video/All? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... File media/filters/video_renderer_impl_unittest.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:223: bool ended_cb_run() { return ended_cb_run_; } nit: make getters const? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:236: base::SimpleTestTickClock* tick_clock_; // Owned by |renderer_|. nit: if you align the comments, why not also align the comment on l.234? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:342: EXPECT_EQ(base::TimeTicks(), Should base::TimeTicks() be tick_clock_->NowTicks()? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:357: EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[0].wall_ticks); ditto about tick_clock_->NowTicks(). Agreed with acolwell that probably we should have a function to use string format to verify these. In that function, we could assert that tick_clock_->NowTicks() is always 0, so the scheduled time should just be the timestamp of the frames. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:381: EXPECT_EQ(0u, scheduler_->scheduled_frames().size()); Also check that "RESET" frames are pushed back to unscheduled_frames_? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:399: scheduler_->Reset(); See comments above about calling scheduler_->Reset() twice. (Pause() calls scheduler_->Reset()). https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:412: hmm, Reset() and SetPlaybackRate() all finishes synchronously. I am a little worried about reentrancy but if it works well than I am in favor of it! https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:415: EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[0].wall_ticks); So even the first frame is rescheduled because Reset() finishes synchronously, but the timers fires asynchronously? It might be nice to note this somewhere. Also, we can test the case where run the message loop first, then reschedule. I guess the first frame will be displayed and not rescheduled? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:444: EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[0].wall_ticks); So this is from the ScheduleFirstFrameForImmediateDisplay() call? Worth a comment. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:451: EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[2].wall_ticks); This make it harder to read, I'd rather see base::TimeDelta::FromMilliseconds(0) :) But a function to check all these would be nice. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:467: scheduler_->DisplayFrames(base::TimeTicks::FromInternalValue(20000)); Write a helper function DisplayFrames(ms) so that we don't need to do the 20000 -> 20 conversion? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:577: Also test that reset doesn't affect stats?
A related question about VideoFrameSchedulerImpl: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide... How accurate is the timer? Can it be late? What if we have a frame scheduled to be displayed at time t. But timer fired at t' > t. Then the frame will become expired and dropped, which doesn't sound right. Am I missing any thing?
there are still two issues I've noticed while testing, but I feel I'd rather get you two to take another look over things issue #1: use after free on WebMediaPlayerImpl::compositor_ when the scheduler runs the display callback (I think VFC might need to be ref counted ... we'll see) issue #2: dealing with repeated displayed frames when changing the playback rate (the lack of explicit coordination between the VFS and VRI combined with a synchronous SetPlaybackRate() API makes this a bit tricky) https://codereview.chromium.org/237353007/diff/120001/content/renderer/media/... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/237353007/diff/120001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:1224: media_loop_, compositor_runner_, scheduler_)), On 2014/04/24 16:43:59, acolwell wrote: > Any reason we can't simply have the proxy own the scheduler_ instead of this > class? nope! done https://codereview.chromium.org/237353007/diff/120001/content/renderer/media/... File content/renderer/media/webmediaplayer_impl.h (right): https://codereview.chromium.org/237353007/diff/120001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.h:340: scheduler_; // Deleted on |compositor_runner_|. On 2014/04/24 18:48:44, xhwang wrote: > nit: I am not a fan of this format. Is this done by clang-format? Can we just > put the comment on the previous line? Done. (it was clang-format) https://codereview.chromium.org/237353007/diff/120001/media/filters/clockless... File media/filters/clockless_video_frame_scheduler.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/clockless... media/filters/clockless_video_frame_scheduler.cc:24: done_cb.Run(frame, DISPLAYED); On 2014/04/24 16:43:59, acolwell wrote: > Shouldn't this be a PostTask() to avoid reentrancy issues? Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/pipeline_... File media/filters/pipeline_integration_test_base.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/pipeline_... media/filters/pipeline_integration_test_base.cc:249: scoped_ptr<VideoFrameScheduler>(new ClocklessVideoFrameScheduler( On 2014/04/24 16:43:59, acolwell wrote: > nit: Shouldn't we only be using this scheduler if clockless_playback_ is set? Ah right ... I guess it depends whether we feel these tests need to run in "real time" versus clockless so they take less time. I'm not sure what sort of coverage we hope we're getting by having artificially slow tests. Hmm... I'll fix this for now but here are some numbers for consideration when running with --single-process-tests --gtest_filter=PipelineIntegration* no clockless: 31s clockless video: 24s (most clips contain audio) clockless audio+video: 8s (!!!) https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... File media/filters/test_video_frame_scheduler.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.cc:35: if (!should_reset_) On 2014/04/24 16:43:59, acolwell wrote: > Why? It seems like this could result in a violation of the VideoFrameScheduler > interface contract. No longer applicable! https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.cc:36: return; On 2014/04/24 18:48:44, xhwang wrote: > The API says Reset() should "Causes the scheduler to release all previously > scheduled frames. Frames will be returned as RESET." > > According to this API, caller should only need to call Reset() once to reset the > frame scheduler. Therefore, I think the following call sequence violates this > API: > > set_should_reset(false); > Reset(); > set_should_reset(false); > Reset(); > > Instead, we could probably do something like: > > HoldReset(); > Reset(); // Reset will be pending. > SatisfyReset(); Ditto -- Reset() has been reworked https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.cc:43: } On 2014/04/24 18:48:44, xhwang wrote: > Can we just use: > > RunDoneCBForFrames(TimeDelta::Max(), RESET); > > ? not anymore now that we have a contract for reentrancy https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.cc:63: remaining_frames.push_back(scheduled_frames_[i]); On 2014/04/24 16:43:59, acolwell wrote: > You have to do this because you aren't using a priority queue here? Is that just > because you want to be able to verify ScheduleVideoFrame() call order? a scheduler doesn't *have* to use a priority queue -- for this particular scheduler using a vector makes it easier for verifying the contents for tests https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... File media/filters/test_video_frame_scheduler.h (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.h:38: void DisplayFrames(base::TimeTicks wall_ticks); On 2014/04/24 18:48:44, xhwang wrote: > s/DisplayFrames/DisplayFramesUpTo/ ? Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.h:41: void DropFrames(base::TimeTicks wall_ticks); On 2014/04/24 18:48:44, xhwang wrote: > ditto about "UpTo" Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/test_vide... media/filters/test_video_frame_scheduler.h:52: std::vector<ScheduledFrame> scheduled_frames_; On 2014/04/24 18:48:44, xhwang wrote: > Any reason we don't sort the frames by timestamp? i.e. using the priority queue? it isn't required -- using a vector preserves the order in which ScheduleVideoFrame() was called you could argue that tests shouldn't have to worry about the order, but I do find it easier to write/read the tests when the ordering is the same (I don't care about this too much, so let me know if you feel otherwise) https://codereview.chromium.org/237353007/diff/120001/media/filters/video_fra... File media/filters/video_frame_scheduler.h (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/video_fra... media/filters/video_frame_scheduler.h:39: virtual void Reset() = 0; On 2014/04/24 16:43:59, acolwell wrote: > nit: You might want add a note here warning people that all |done_cb|s will be > dispatched within this call and callers should be careful about reentrancy > issues. If that is not the intended semantics then I think we should make sure > that all implementations do not dispatch the CBs within this call for consistant > behavior. Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... File media/filters/video_renderer_impl.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:55: DCHECK(state_ != kUninitialized || state_ == kError); On 2014/04/24 16:43:59, acolwell wrote: > nit: Add != kPausing ? kPausing is no more! https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:73: // stream and needs to drain it before flushing it. On 2014/04/24 16:43:59, acolwell wrote: > nit: Is this comment still correct? It confuses me. We need to do this because > frames could have arrived in the course of pausing right? That's my understanding. I feel the comment might be a bit stale... xhwang: you added this in r211288 -- do you recall whether it still might be relevant? https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:310: } On 2014/04/24 18:48:44, xhwang wrote: > nit: I found it's easier to understand if we just expand some > ShouldTransitionToXxx() and TransitionToXxx() functions. But maybe that's just > my problem :) yeah ... the biggest thing I wanted out of these tiny helper functions is to enforce doing the DCHECK() https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:390: base::TimeTicks current_wall_ticks = tick_clock_->NowTicks(); On 2014/04/24 16:43:59, acolwell wrote: > hmm.. ISTM that pipeline's Clock should be the sole source of timestamp -> wall > clock mapping. This essentially creates a second mapping that could potentially > introduce drift. Perhaps we need to change get_time_cb_'s signature so we can > get the correct wall clock time from pipeline's Clock. I agree -- we're not quite done with how we want clocks sorted out but I'd rather leave that for future CLs / investigation. As it stands today, everyone is using base::DefaultTickClock so I don't think we'll run into trouble. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:395: playback_rate_; On 2014/04/24 18:48:44, xhwang wrote: > So even if delta < 0 we still ScheduleVideoFrame()? Isn't that frame guaranteed > to be dropped? Are we trying to make the code cleaner (less branches)? This may > be worth a comment. Done. It's up to the scheduler to decide whether it wants to drop the frame, otherwise we're splitting frame dropping logic between two classes. It would also make it more difficult to implement something like ClocklessVideoFrameScheduler is VideoRendererImpl was making its own scheduling decisions. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:409: void VideoRendererImpl::ScheduleFirstFrameForImmediateDisplay() { On 2014/04/24 18:48:44, xhwang wrote: > DCHECK_EQ(state_, kPrerolled); Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:418: } On 2014/04/24 18:48:44, xhwang wrote: > Probably we can have a helper function like > > ScheduleNextFrame(base::TimeTicks target_wall_ticks) > > which can be used by both ScheduleVideoFrames() and > ScheduleFirstFrameForImmediateDisplay(). I tried it out but didn't like how ScheduleAllFrames() still has to access unscheduled_frames_.front() to calculate wall_ticks and then hope that ScheduleNextFrame() will reference the same frame https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:430: scheduled_frames_.erase(iter); On 2014/04/24 16:43:59, acolwell wrote: > nit: An reason not to do this before the switch since you do it in all branches > and you could simply push |frame| in the RESET case. Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:444: PipelineStatistics stats; On 2014/04/24 16:43:59, acolwell wrote: > nit: Move this below w/ a bool guard so we don't have to duplicate the code in > the switch and we only have to worry about one location where the CB is > dispatched? With change in Reset() behaviour this function cleaned up a bunch! https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:452: unscheduled_frames_.push_back(*iter); On 2014/04/24 18:48:44, xhwang wrote: > ha, It surprised me a bit that the "reset" frames will be rescheduled. It might > worth a comment at: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide... out of curiosity, what did you think "reset" was for? perhaps we can use a different function name? ("release", "return", etc...) anyway Reset()'s behaviour has been changed to no longer call back https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... File media/filters/video_renderer_impl.h (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.h:81: void ScheduleVideoFrames(); On 2014/04/24 18:48:44, xhwang wrote: > Drop the "Video" part, or s/Video/All? Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... File media/filters/video_renderer_impl_unittest.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:223: bool ended_cb_run() { return ended_cb_run_; } On 2014/04/24 18:48:44, xhwang wrote: > nit: make getters const? Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:226: // Arbitrary value. Has to be larger to cover any timestamp used in tests. On 2014/04/24 16:43:59, acolwell wrote: > nit: s/larger/large/? Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:236: base::SimpleTestTickClock* tick_clock_; // Owned by |renderer_|. On 2014/04/24 18:48:44, xhwang wrote: > nit: if you align the comments, why not also align the comment on l.234? Done. (I think clang-format did this) https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:342: EXPECT_EQ(base::TimeTicks(), On 2014/04/24 18:48:44, xhwang wrote: > Should base::TimeTicks() be tick_clock_->NowTicks()? Replaced with string format. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:357: EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[0].wall_ticks); On 2014/04/24 16:43:59, acolwell wrote: > String format verification for these too? Either that or a helper function to > make this less cluttered looking. Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:381: EXPECT_EQ(0u, scheduler_->scheduled_frames().size()); On 2014/04/24 18:48:44, xhwang wrote: > Also check that "RESET" frames are pushed back to unscheduled_frames_? Isn't needed due to change in Reset() behaviour https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:392: scheduler_->set_should_reset(false); On 2014/04/24 16:43:59, acolwell wrote: > This is for simulating the proxy VFS case right? Any reason not to simply use > that class for this test so it is clearer why the pause would get delayed? Removed this test case due to change in Reset() behaviour https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:399: scheduler_->Reset(); On 2014/04/24 18:48:44, xhwang wrote: > See comments above about calling scheduler_->Reset() twice. (Pause() calls > scheduler_->Reset()). Ditto https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:412: On 2014/04/24 18:48:44, xhwang wrote: > hmm, Reset() and SetPlaybackRate() all finishes synchronously. I am a little > worried about reentrancy but if it works well than I am in favor of it! actually this test started failing once I made Reset() post tasks ... which led me to re-think what Reset() should do! https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:414: EXPECT_EQ(4u, scheduler_->scheduled_frames().size()); On 2014/04/24 16:43:59, acolwell wrote: > ditto. more concise expectations would be nice. Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:415: EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[0].wall_ticks); On 2014/04/24 18:48:44, xhwang wrote: > So even the first frame is rescheduled because Reset() finishes synchronously, > but the timers fires asynchronously? It might be nice to note this somewhere. > > Also, we can test the case where run the message loop first, then reschedule. I > guess the first frame will be displayed and not rescheduled? had to change this stuff https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:444: EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[0].wall_ticks); On 2014/04/24 18:48:44, xhwang wrote: > So this is from the ScheduleFirstFrameForImmediateDisplay() call? Worth a > comment. Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:451: EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[2].wall_ticks); On 2014/04/24 18:48:44, xhwang wrote: > This make it harder to read, I'd rather see base::TimeDelta::FromMilliseconds(0) > :) But a function to check all these would be nice. Replaced with string format. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:467: scheduler_->DisplayFrames(base::TimeTicks::FromInternalValue(20000)); On 2014/04/24 18:48:44, xhwang wrote: > Write a helper function DisplayFrames(ms) so that we don't need to do the 20000 > -> 20 conversion? Done. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl_unittest.cc:577: On 2014/04/24 18:48:44, xhwang wrote: > Also test that reset doesn't affect stats? Changed reset so it no longer calls back https://codereview.chromium.org/237353007/diff/140001/media/filters/video_fra... File media/filters/video_frame_scheduler.h (right): https://codereview.chromium.org/237353007/diff/140001/media/filters/video_fra... media/filters/video_frame_scheduler.h:43: virtual void Reset() = 0; I do like this style of API as it makes less guarantees, however I noticed during testing that when going from a high playback rate to a low playback rate that we end up showing a few duplicate frames. it's a bit of a tricky problem where the real answer might be to re-introduce the RESET parameter and make SetPlaybackRate() async :( I need to think about this more ... https://codereview.chromium.org/237353007/diff/140001/media/filters/video_ren... File media/filters/video_renderer_impl.cc (right): https://codereview.chromium.org/237353007/diff/140001/media/filters/video_ren... media/filters/video_renderer_impl.cc:430: LOG(ERROR) << "Ignoring frame_id=" << frame_id note to self: remove
On 2014/04/25 00:42:42, xhwang wrote: > A related question about VideoFrameSchedulerImpl: > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide... > > How accurate is the timer? Can it be late? What if we have a frame scheduled to > be displayed at time t. But timer fired at t' > t. Then the frame will become > expired and dropped, which doesn't sound right. Am I missing any thing? the current scheduler definitely needs more polish ... it's perhaps a little stricter than it should be :) as a compromise, still decides to display the *last* expired frame: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide... this means that on a tick if only one frame has expired but the rest still aren't ready yet, we'll still end up showing that single expired frame if there are 2+ expired frames, we'll show only the last expired frame
On 2014/04/25 02:07:09, scherkus wrote: > On 2014/04/25 00:42:42, xhwang wrote: > > A related question about VideoFrameSchedulerImpl: > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide... > > > > How accurate is the timer? Can it be late? What if we have a frame scheduled > to > > be displayed at time t. But timer fired at t' > t. Then the frame will become > > expired and dropped, which doesn't sound right. Am I missing any thing? > > the current scheduler definitely needs more polish ... it's perhaps a little > stricter than it should be :) > > as a compromise, still decides to display the *last* expired frame: > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide... > > this means that on a tick if only one frame has expired but the rest still > aren't ready yet, we'll still end up showing that single expired frame > > if there are 2+ expired frames, we'll show only the last expired frame I see. But if the timer is a bit early, say t' = t - delta. Then the frame which is supposed to be displayed won't get displayed because it's not expired yet. Then when the next timer fires, we may just display the next frame and drop this frame. For example: Frame timestamp: 1 2 Timer fired at: 0.99 2.01 Then frame "1" will be dropped.
I didn't fully review this PS. Just a few more comments when I skim it. https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... File media/filters/video_renderer_impl.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:73: // stream and needs to drain it before flushing it. On 2014/04/25 02:04:47, scherkus wrote: > On 2014/04/24 16:43:59, acolwell wrote: > > nit: Is this comment still correct? It confuses me. We need to do this because > > frames could have arrived in the course of pausing right? > > That's my understanding. > > I feel the comment might be a bit stale... xhwang: you added this in r211288 -- > do you recall whether it still might be relevant? If you look at the history, it existed even before r211288. In r211288 I modified it a bit when I introduce VFS... Anyway, this comment is confusing here and I am in favor of removing it :) https://codereview.chromium.org/237353007/diff/140001/media/filters/test_vide... File media/filters/test_video_frame_scheduler.cc (right): https://codereview.chromium.org/237353007/diff/140001/media/filters/test_vide... media/filters/test_video_frame_scheduler.cc:65: done_frames[i].done_cb.Run(done_frames[i].frame, reason); You #include "base/message_loop/message_loop_proxy.h" but are not using it anywhere. Post task here? https://codereview.chromium.org/237353007/diff/140001/media/filters/video_fra... File media/filters/video_frame_scheduler.h (right): https://codereview.chromium.org/237353007/diff/140001/media/filters/video_fra... media/filters/video_frame_scheduler.h:43: virtual void Reset() = 0; On 2014/04/25 02:04:47, scherkus wrote: > I do like this style of API as it makes less guarantees, however I noticed > during testing that when going from a high playback rate to a low playback rate > that we end up showing a few duplicate frames. > > it's a bit of a tricky problem where the real answer might be to re-introduce > the RESET parameter and make SetPlaybackRate() async :( > > I need to think about this more ... Note to myself: How does the caller know which frame has been displayed/dropped or not? https://codereview.chromium.org/237353007/diff/140001/media/filters/video_ren... File media/filters/video_renderer_impl.cc (right): https://codereview.chromium.org/237353007/diff/140001/media/filters/video_ren... media/filters/video_renderer_impl.cc:371: } With all the asyncness, it's not a surprise that a frame pushed back here has actually already been displayed. What's the plan here? How can we make sure that a frame that's been displayed (expired) will not be displayed again?
On 2014/04/25 05:03:45, xhwang wrote: > On 2014/04/25 02:07:09, scherkus wrote: > > On 2014/04/25 00:42:42, xhwang wrote: > > > A related question about VideoFrameSchedulerImpl: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide... > > > > > > How accurate is the timer? Can it be late? What if we have a frame scheduled > > to > > > be displayed at time t. But timer fired at t' > t. Then the frame will > become > > > expired and dropped, which doesn't sound right. Am I missing any thing? > > > > the current scheduler definitely needs more polish ... it's perhaps a little > > stricter than it should be :) > > > > as a compromise, still decides to display the *last* expired frame: > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide... > > > > this means that on a tick if only one frame has expired but the rest still > > aren't ready yet, we'll still end up showing that single expired frame > > > > if there are 2+ expired frames, we'll show only the last expired frame > > I see. But if the timer is a bit early, say t' = t - delta. Then the frame which > is supposed to be displayed won't get displayed because it's not expired yet. > Then when the next timer fires, we may just display the next frame and drop this > frame. For example: > > Frame timestamp: 1 2 > Timer fired at: 0.99 2.01 > > Then frame "1" will be dropped. Thanks for the example and thinking of edge cases! My understanding is the implementation of the timer uses PostDelayedTask(), which will only run tasks at or later than the desired time, but never early.
https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... File media/filters/video_renderer_impl.cc (right): https://codereview.chromium.org/237353007/diff/120001/media/filters/video_ren... media/filters/video_renderer_impl.cc:73: // stream and needs to drain it before flushing it. On 2014/04/25 05:44:47, xhwang wrote: > On 2014/04/25 02:04:47, scherkus wrote: > > On 2014/04/24 16:43:59, acolwell wrote: > > > nit: Is this comment still correct? It confuses me. We need to do this > because > > > frames could have arrived in the course of pausing right? > > > > That's my understanding. > > > > I feel the comment might be a bit stale... xhwang: you added this in r211288 > -- > > do you recall whether it still might be relevant? > > If you look at the history, it existed even before r211288. In r211288 I > modified it a bit when I introduce VFS... > > Anyway, this comment is confusing here and I am in favor of removing it :) Removed. https://codereview.chromium.org/237353007/diff/140001/media/filters/video_ren... File media/filters/video_renderer_impl.cc (right): https://codereview.chromium.org/237353007/diff/140001/media/filters/video_ren... media/filters/video_renderer_impl.cc:371: } On 2014/04/25 05:44:47, xhwang wrote: > With all the asyncness, it's not a surprise that a frame pushed back here has > actually already been displayed. What's the plan here? How can we make sure > that a frame that's been displayed (expired) will not be displayed again? It's very possible we'll need to introduce more waiting/coordination between the VFS and VRI. I have some ideas on how to fix the SetPlaybackRate() issue, but I'm trying to think through other scenarios where we reset the scheduler that could lead to duplicate frame displays.
On 2014/04/25 18:21:40, scherkus wrote: > On 2014/04/25 05:03:45, xhwang wrote: > > On 2014/04/25 02:07:09, scherkus wrote: > > > On 2014/04/25 00:42:42, xhwang wrote: > > > > A related question about VideoFrameSchedulerImpl: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide... > > > > > > > > How accurate is the timer? Can it be late? What if we have a frame > scheduled > > > to > > > > be displayed at time t. But timer fired at t' > t. Then the frame will > > become > > > > expired and dropped, which doesn't sound right. Am I missing any thing? > > > > > > the current scheduler definitely needs more polish ... it's perhaps a little > > > stricter than it should be :) > > > > > > as a compromise, still decides to display the *last* expired frame: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide... > > > > > > this means that on a tick if only one frame has expired but the rest still > > > aren't ready yet, we'll still end up showing that single expired frame > > > > > > if there are 2+ expired frames, we'll show only the last expired frame > > > > I see. But if the timer is a bit early, say t' = t - delta. Then the frame > which > > is supposed to be displayed won't get displayed because it's not expired yet. > > Then when the next timer fires, we may just display the next frame and drop > this > > frame. For example: > > > > Frame timestamp: 1 2 > > Timer fired at: 0.99 2.01 > > > > Then frame "1" will be dropped. > > Thanks for the example and thinking of edge cases! > > My understanding is the implementation of the timer uses PostDelayedTask(), > which will only run tasks at or later than the desired time, but never early. I see. That makes sense. Thanks for the explanation.
re: playback rate funkiness ... it looks like it's actually a bad interaction with how the clock is being updated when there is audio present the current CL actually works pretty well as is for video-only content. hmm...
On 2014/04/25 20:56:28, scherkus wrote: > re: playback rate funkiness ... it looks like it's actually a bad interaction > with how the clock is being updated when there is audio present > > the current CL actually works pretty well as is for video-only content. hmm... mystery solved: http://crbug.com/367343 I might split out this CL into a few smaller stand alone bits while I tackle the audio bug...
alright .. I split out 4 CLs: https://codereview.chromium.org/251733005/ https://codereview.chromium.org/251833004/ https://codereview.chromium.org/252703006/ https://codereview.chromium.org/257793004/ ... and rebased this CL on top of them. Let's hold off reviewing this until those 4 land I and I figure out the audio stuff. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
