Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
680533004:
Modify MojoRendererService to host a RendererImpl. (Closed)
|
Created:
6 years, 2 months ago by DaleCurtis Modified:
6 years, 1 month ago Reviewers:
xhwang CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionModify MojoRendererService to host a RendererImpl.
Removes the raw AudioRenderer in favor of a RendererImpl which
deduplicates a lot of code.
Notes:
- Time updates are scheduled from StartPlayingFrom() until ended or flush.
- Liveness detection will be moved elsewhere in the chain as part of resolving
http://crbug.com/420025
BUG=410451
TEST=mojo_shell
Committed: https://crrev.com/f461a3f3ffe02908892ea8b72f6d71b2dbfa933c
Cr-Commit-Position: refs/heads/master@{#301955}
Patch Set 1 #
Total comments: 21
Patch Set 2 : Rebase. Fixes. #
Total comments: 6
Patch Set 3 : Comments. #
Total comments: 4
Patch Set 4 : Comments. #
Messages
Total messages: 15 (2 generated)
dalecurtis@chromium.org changed reviewers: + xhwang@chromium.org
WDYT? How have you been testing this stuff by the way. It'd be nice if we could get some unittests going.
Looking pretty good. I only have a few comments. https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:67: // Stub VideoRenderer, scoped_ptr<VideoRenderer> doesn't take a nullptr nicely. This is unfortunate... Does this work for you? scoped_ptr<VideoRenderer> video_renderer(nullptr); new RendererImpl(task_runner, audio_renderer.Pass(), video_renderer.Pass()); It compiles for me at least. https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:109: static void MojoTrampoline(const mojo::Closure& closure) { I wonder whether we have some generic way to convert base::Callback to mojo::Callback... I searched around and didn't find any though. https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:178: callback))).Pass())); Imagine when we have two streams, how will this call look like in terms of callback passing? https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:195: // a bunch of additional state to this class plus RendererImpl, how to tell? hmm, I thought after StartPlayingFrom() (i.e. as long as we are playing) the time will advance. So what do you mean by "time updates shouldn't always tick"? I think I am missing something... https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:232: } How about if (state_ == STATE_ERROR) { renderer_.reset(); } else { DCHECK_EQ(state_, STATE_INITIALIZING); state_ = STATE_PLAYING; } https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:244: return; See comments above... How often is media_time == last_media_time_ happening? If it does happen, can we make |only_if_changed| an implementation detail of UpdateMediaTime()? You can think it this way, only_if_changed is always true implicitly. Then during StartPlayingFrom(time_delta_usec), we can either: 1, set last_media_time_ to time_delta_usec, or 2, set last_media_time_ to 0. In (1), we won't update time if it is the same as time_delta_usec. This is okay because MojoRendererImpl sets time_ = time in StartPlayingFrom() as well. In (2), we always update time once after StartPlayingFrom(). I feel (2) is better. We probably shouldn't rely on how MojoRendererImpl is implemented. On a second thought, we probably should clear last_media_time_ in PausePlayback(). WDYT? https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... File media/mojo/services/mojo_renderer_service.h (right): https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.h:85: scoped_ptr<RendererImpl> renderer_; Can this be a scoped_ptr<Renderer>? https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.h:89: uint64_t last_media_time_; Is this in us?
On 2014/10/24 23:15:58, DaleCurtis wrote: > WDYT? How have you been testing this stuff by the way. It'd be nice if we could > get some unittests going. So far I only test it with my test page. No unit test yet :(
https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:67: // Stub VideoRenderer, scoped_ptr<VideoRenderer> doesn't take a nullptr nicely. On 2014/10/28 17:13:33, xhwang wrote: > This is unfortunate... > > Does this work for you? > > scoped_ptr<VideoRenderer> video_renderer(nullptr); > > new RendererImpl(task_runner, audio_renderer.Pass(), video_renderer.Pass()); > > It compiles for me at least. \o/, yes :) https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:109: static void MojoTrampoline(const mojo::Closure& closure) { On 2014/10/28 17:13:33, xhwang wrote: > I wonder whether we have some generic way to convert base::Callback to > mojo::Callback... I searched around and didn't find any though. Yeah I think that'd be useful, but am too weak in variadic templates to craft such a thing quickly myself. :| https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:178: callback))).Pass())); On 2014/10/28 17:13:33, xhwang wrote: > Imagine when we have two streams, how will this call look like in terms of > callback passing? I was thinking the DemuxerStreamProviderShim would handle creation of the MojoDemuxerStreamAdapter using a SerialRunner before finally calling a provided OnStreamReadyCB. Otherwise it would work like any other provider, it would just route calls based on which DemuxerStream::Type is given. I can do that in this CL if you prefer. I thought it might be nicer to do so once we add support for video streams; though that ended up sooner than expected. https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:195: // a bunch of additional state to this class plus RendererImpl, how to tell? On 2014/10/28 17:13:33, xhwang wrote: > hmm, I thought after StartPlayingFrom() (i.e. as long as we are playing) the > time will advance. So what do you mean by "time updates shouldn't always tick"? > I think I am missing something... Well this class can't determine if time is actually moving when nominally "playing" since underflows, sink startup time and such aren't really moving the clock forward. There's also the matter of when time updates should stop. I.e. what about playback_rate=0? Do we want time to always tick until OnRendererEnded? That's what I'm doing now... but I'm not convinced it's the right thing. Ideally these updates should only tick when time is actually moving. We could also let clients register with the RendererImpl for MediaTime updates or something similar using an observer pattern. https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:232: } On 2014/10/28 17:13:33, xhwang wrote: > How about > > if (state_ == STATE_ERROR) { > renderer_.reset(); > } else { > DCHECK_EQ(state_, STATE_INITIALIZING); > state_ = STATE_PLAYING; > } Done. https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:244: return; On 2014/10/28 17:13:33, xhwang wrote: > See comments above... How often is media_time == last_media_time_ happening? > > If it does happen, can we make |only_if_changed| an implementation detail of > UpdateMediaTime()? You can think it this way, only_if_changed is always true > implicitly. Then during StartPlayingFrom(time_delta_usec), we can either: > 1, set last_media_time_ to time_delta_usec, or > 2, set last_media_time_ to 0. > > In (1), we won't update time if it is the same as time_delta_usec. This is okay > because MojoRendererImpl sets time_ = time in StartPlayingFrom() as well. > > In (2), we always update time once after StartPlayingFrom(). > > I feel (2) is better. We probably shouldn't rely on how MojoRendererImpl is > implemented. On a second thought, we probably should clear last_media_time_ in > PausePlayback(). > > WDYT? This is just to prevent unnecessary time update calls when the time hasn't changed. I expect it will happen for ~hundred milliseconds or so around startup and for an unknown amount of time if the renderer underflows. 2) is in fact what I'm doing right now unless I misunderstand you. Above, SchedulePeriodicMediaTimeUpdates() is called by StartPlayingFrom() which forces a media time update. https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... File media/mojo/services/mojo_renderer_service.h (right): https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.h:85: scoped_ptr<RendererImpl> renderer_; On 2014/10/28 17:13:33, xhwang wrote: > Can this be a scoped_ptr<Renderer>? Done.
looking pretty good now. Just a few comments. https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:109: static void MojoTrampoline(const mojo::Closure& closure) { On 2014/10/28 21:51:29, DaleCurtis wrote: > On 2014/10/28 17:13:33, xhwang wrote: > > I wonder whether we have some generic way to convert base::Callback to > > mojo::Callback... I searched around and didn't find any though. > > Yeah I think that'd be useful, but am too weak in variadic templates to craft > such a thing quickly myself. :| Agreed :) https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:178: callback))).Pass())); On 2014/10/28 21:51:29, DaleCurtis wrote: > On 2014/10/28 17:13:33, xhwang wrote: > > Imagine when we have two streams, how will this call look like in terms of > > callback passing? > > I was thinking the DemuxerStreamProviderShim would handle creation of the > MojoDemuxerStreamAdapter using a SerialRunner before finally calling a provided > OnStreamReadyCB. > > Otherwise it would work like any other provider, it would just route calls based > on which DemuxerStream::Type is given. I can do that in this CL if you prefer. > I thought it might be nicer to do so once we add support for video streams; > though that ended up sooner than expected. I think we are dropping SerialRunner... Route calls based on Type sgtm. Let's do it in the next CL. https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:195: // a bunch of additional state to this class plus RendererImpl, how to tell? On 2014/10/28 21:51:29, DaleCurtis wrote: > On 2014/10/28 17:13:33, xhwang wrote: > > hmm, I thought after StartPlayingFrom() (i.e. as long as we are playing) the > > time will advance. So what do you mean by "time updates shouldn't always > tick"? > > I think I am missing something... > > Well this class can't determine if time is actually moving when nominally > "playing" since underflows, sink startup time and such aren't really moving the > clock forward. > > There's also the matter of when time updates should stop. I.e. what about > playback_rate=0? Do we want time to always tick until OnRendererEnded? That's > what I'm doing now... but I'm not convinced it's the right thing. Ideally these > updates should only tick when time is actually moving. > > We could also let clients register with the RendererImpl for MediaTime updates > or something similar using an observer pattern. I see. Underflow is indeed a legit case. I was suggesting dropping |only_if_changed| and decide whether we should update the media time solely based on the current media time and last_media_time_. That requires us to update last_media_time_ during StartPlayingFrom() and PausePlayback(). But I feel that's cleaner. WDYT? https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:244: return; On 2014/10/28 21:51:29, DaleCurtis wrote: > On 2014/10/28 17:13:33, xhwang wrote: > > See comments above... How often is media_time == last_media_time_ happening? > > > > If it does happen, can we make |only_if_changed| an implementation detail of > > UpdateMediaTime()? You can think it this way, only_if_changed is always true > > implicitly. Then during StartPlayingFrom(time_delta_usec), we can either: > > 1, set last_media_time_ to time_delta_usec, or > > 2, set last_media_time_ to 0. > > > > In (1), we won't update time if it is the same as time_delta_usec. This is > okay > > because MojoRendererImpl sets time_ = time in StartPlayingFrom() as well. > > > > In (2), we always update time once after StartPlayingFrom(). > > > > I feel (2) is better. We probably shouldn't rely on how MojoRendererImpl is > > implemented. On a second thought, we probably should clear last_media_time_ in > > PausePlayback(). > > > > WDYT? > > This is just to prevent unnecessary time update calls when the time hasn't > changed. I expect it will happen for ~hundred milliseconds or so around startup > and for an unknown amount of time if the renderer underflows. > > 2) is in fact what I'm doing right now unless I misunderstand you. Above, > SchedulePeriodicMediaTimeUpdates() is called by StartPlayingFrom() which forces > a media time update. See my comment above about dropping |only_if_changed|. https://codereview.chromium.org/680533004/diff/20001/media/mojo/services/mojo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/680533004/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_service.cc:84: : state_(STATE_UNINITIALIZED), You still need to initialize last_media_time_ https://codereview.chromium.org/680533004/diff/20001/media/mojo/services/mojo... File media/mojo/services/mojo_renderer_service.h (right): https://codereview.chromium.org/680533004/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_service.h:26: class AudioRendererSink; order https://codereview.chromium.org/680533004/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_service.h:88: uint64_t last_media_time_; s/last_media_time_/last_media_time_usec_
https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:195: // a bunch of additional state to this class plus RendererImpl, how to tell? On 2014/10/28 22:43:28, xhwang wrote: > On 2014/10/28 21:51:29, DaleCurtis wrote: > > On 2014/10/28 17:13:33, xhwang wrote: > > > hmm, I thought after StartPlayingFrom() (i.e. as long as we are playing) the > > > time will advance. So what do you mean by "time updates shouldn't always > > tick"? > > > I think I am missing something... > > > > Well this class can't determine if time is actually moving when nominally > > "playing" since underflows, sink startup time and such aren't really moving > the > > clock forward. > > > > There's also the matter of when time updates should stop. I.e. what about > > playback_rate=0? Do we want time to always tick until OnRendererEnded? That's > > what I'm doing now... but I'm not convinced it's the right thing. Ideally > these > > updates should only tick when time is actually moving. > > > > We could also let clients register with the RendererImpl for MediaTime updates > > or something similar using an observer pattern. > > I see. Underflow is indeed a legit case. > > I was suggesting dropping |only_if_changed| and decide whether we should update > the media time solely based on the current media time and last_media_time_. That > requires us to update last_media_time_ during StartPlayingFrom() and > PausePlayback(). But I feel that's cleaner. WDYT? There is no PausePlayback() method, do you mean when SetPlaybackRate() changes? https://codereview.chromium.org/680533004/diff/20001/media/mojo/services/mojo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/680533004/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_service.cc:84: : state_(STATE_UNINITIALIZED), On 2014/10/28 22:43:28, xhwang wrote: > You still need to initialize last_media_time_ Done. https://codereview.chromium.org/680533004/diff/20001/media/mojo/services/mojo... File media/mojo/services/mojo_renderer_service.h (right): https://codereview.chromium.org/680533004/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_service.h:26: class AudioRendererSink; On 2014/10/28 22:43:28, xhwang wrote: > order Done. https://codereview.chromium.org/680533004/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_service.h:88: uint64_t last_media_time_; On 2014/10/28 22:43:28, xhwang wrote: > s/last_media_time_/last_media_time_usec_ Done.
https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/680533004/diff/1/media/mojo/services/mojo_ren... media/mojo/services/mojo_renderer_service.cc:195: // a bunch of additional state to this class plus RendererImpl, how to tell? On 2014/10/29 00:11:19, DaleCurtis wrote: > On 2014/10/28 22:43:28, xhwang wrote: > > On 2014/10/28 21:51:29, DaleCurtis wrote: > > > On 2014/10/28 17:13:33, xhwang wrote: > > > > hmm, I thought after StartPlayingFrom() (i.e. as long as we are playing) > the > > > > time will advance. So what do you mean by "time updates shouldn't always > > > tick"? > > > > I think I am missing something... > > > > > > Well this class can't determine if time is actually moving when nominally > > > "playing" since underflows, sink startup time and such aren't really moving > > the > > > clock forward. > > > > > > There's also the matter of when time updates should stop. I.e. what about > > > playback_rate=0? Do we want time to always tick until OnRendererEnded? > That's > > > what I'm doing now... but I'm not convinced it's the right thing. Ideally > > these > > > updates should only tick when time is actually moving. > > > > > > We could also let clients register with the RendererImpl for MediaTime > updates > > > or something similar using an observer pattern. > > > > I see. Underflow is indeed a legit case. > > > > I was suggesting dropping |only_if_changed| and decide whether we should > update > > the media time solely based on the current media time and last_media_time_. > That > > requires us to update last_media_time_ during StartPlayingFrom() and > > PausePlayback(). But I feel that's cleaner. WDYT? > > There is no PausePlayback() method, do you mean when SetPlaybackRate() changes? Also note, that if the first media time is 0 then we won't send an update if we rely only on last_media_time != media_time (which is why I have the only if changed flag)
https://codereview.chromium.org/680533004/diff/40001/media/mojo/services/mojo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/680533004/diff/40001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_service.cc:135: renderer_->Flush(base::Bind(&MojoTrampoline, callback)); I see. PausePlayback() is now hidden in RendererImpl. The equivalent of it now is Flush(). If you look at RendererImpl, Flush() calls PausePlayback(). What you are doing now (restart the timer everytime we call StartPlayingFrom()) is fine. But during pause, we'll still call UpdateMediaTime(), which will do nothing since time hasn't changed. It sounds to me not UpdateMediaTime() is better... https://codereview.chromium.org/680533004/diff/40001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_service.cc:188: void MojoRendererService::UpdateMediaTime(bool only_if_changed) { I see. A boolean sgtm. One last nit though: s/only_if_changed/force?
https://codereview.chromium.org/680533004/diff/40001/media/mojo/services/mojo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/680533004/diff/40001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_service.cc:135: renderer_->Flush(base::Bind(&MojoTrampoline, callback)); On 2014/10/29 00:39:44, xhwang wrote: > I see. PausePlayback() is now hidden in RendererImpl. The equivalent of it now > is Flush(). If you look at RendererImpl, Flush() calls PausePlayback(). > > What you are doing now (restart the timer everytime we call StartPlayingFrom()) > is fine. But during pause, we'll still call UpdateMediaTime(), which will do > nothing since time hasn't changed. It sounds to me not UpdateMediaTime() is > better... Ahh, I see. I hadn't made that connection. I've added code to reset the timer during flush. https://codereview.chromium.org/680533004/diff/40001/media/mojo/services/mojo... media/mojo/services/mojo_renderer_service.cc:188: void MojoRendererService::UpdateMediaTime(bool only_if_changed) { On 2014/10/29 00:39:44, xhwang wrote: > I see. A boolean sgtm. One last nit though: s/only_if_changed/force? Done.
lgtm
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/680533004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f461a3f3ffe02908892ea8b72f6d71b2dbfa933c Cr-Commit-Position: refs/heads/master@{#301955} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
