Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1854893002:
[chromecast] Bind and run mojo media services on cma thread. (Closed)
|
Created:
4 years, 8 months ago by alokp Modified:
4 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, lcwu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, halliwell+watch_chromium.org, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[chromecast] Bind and run mojo media services on cma thread.
BUG=594234
Committed: https://crrev.com/587432023fe05823c2ecfbdc64d37657e76817f1
Cr-Commit-Position: refs/heads/master@{#385392}
Patch Set 1 #Patch Set 2 : move task runner #
Total comments: 2
Patch Set 3 : cast-specific media application #Patch Set 4 : updated patch desc #Patch Set 5 : cleaned up deps #
Total comments: 4
Patch Set 6 : addressed comments #Patch Set 7 : addressed comments 2 #
Messages
Total messages: 37 (11 generated)
alokp@chromium.org changed reviewers: + xhwang@chromium.org
xhwang@chromium.org changed reviewers: + rockot@chromium.org
+rockot https://codereview.chromium.org/1854893002/diff/20001/media/mojo/services/moj... File media/mojo/services/mojo_media_application.cc (right): https://codereview.chromium.org/1854893002/diff/20001/media/mojo/services/moj... media/mojo/services/mojo_media_application.cc:55: connection, base::Passed(&request))); As far as I understand, a new thread will be created for this app. Then we are running everything on a different thread. How about running the app directly on an existing thread where you like it to be run on? I explored this idea a while back. Here's a prototype CL: https://chromiumcodereview.appspot.com/1414463005/. It worked for me but then I didn't need it. It may need some more polish I guess.
https://codereview.chromium.org/1854893002/diff/20001/media/mojo/services/moj... File media/mojo/services/mojo_media_application.cc (right): https://codereview.chromium.org/1854893002/diff/20001/media/mojo/services/moj... media/mojo/services/mojo_media_application.cc:55: connection, base::Passed(&request))); On 2016/04/01 20:10:05, xhwang wrote: > As far as I understand, a new thread will be created for this app. Then we are > running everything on a different thread. > > How about running the app directly on an existing thread where you like it to be > run on? I explored this idea a while back. Here's a prototype CL: > https://chromiumcodereview.appspot.com/1414463005/. It worked for me but then I > didn't need it. It may need some more polish I guess. That would work for our use case. In fact it will be better because we avoid an additional thread. I guess we could add this by plumbing application task runner to content::StaticLoader? But again Ken has indicated that content/ usage of mojo shell is being heavily refactored, so it may be StaticLoader is going away as well and may not be worth doing at this point?
alokp@chromium.org changed reviewers: + jam@chromium.org
+jam since Ken is out. John: Is it possible to run a mojo application on an existing thread? Currently content::StaticLoader always starts a thread. It has been discussed in the past: crbug.com/521754 In this case, mojo:media application runs in the browser process. The media backend library that serves both mojo:media application and AudioManager can only run on a single thread. To avoid excessive thread hops, we want to share the media thread between the two. This patch binds the interface request on the media thread and appears to work fine but: 1. There are concerns about thread safety of mojo::MessageLoopRefFactory 2. The thread started for the mojo:media application is mostly redundant, which seems like a waste Ideally we should be able to specify an existing thread on which the application should be run. Perhaps we could specify that in StaticApplicationMap?
There is also a CurrentThreadLoader which can load the app on the "current" thread, which in this case is the browser UI thread. Given StaticThreadLoader and CurrentThreadLoader are both fairly simple, I suppose it won't be hard to add a Loader that can load an app on a particular thread. This would be the cleanest solution at this point. I know Ken is planning to refactor all these stuff so he didn't recommend to use CurrentThreadLoader, let alone adding one. But this is currently blocking Alop, and the functionality we are adding is a typical use case that has emerged before (see crbug.com/521754).
On 2016/04/02 00:17:07, xhwang (OOO Apr4 - Apr8) wrote: > There is also a CurrentThreadLoader which can load the app on the "current" > thread, which in this case is the browser UI thread. Given StaticThreadLoader > and CurrentThreadLoader are both fairly simple, I suppose it won't be hard to > add a Loader that can load an app on a particular thread. This would be the > cleanest solution at this point. > > I know Ken is planning to refactor all these stuff so he didn't recommend to use > CurrentThreadLoader, let alone adding one. But this is currently blocking Alop, > and the functionality we are adding is a typical use case that has emerged > before (see crbug.com/521754). We shouldn't be creating new threads for different mojo applications. This won't scale, and is counter to the Lucky Luke project which aims to cut down threads in the browser process. Even though CurrentThreadLoader is going away, it seems that the work to convert existing users won't increase much more if this media stuff also used it.
On 2016/04/04 16:58:37, jam wrote: > On 2016/04/02 00:17:07, xhwang (OOO Apr4 - Apr8) wrote: > > There is also a CurrentThreadLoader which can load the app on the "current" > > thread, which in this case is the browser UI thread. Given StaticThreadLoader > > and CurrentThreadLoader are both fairly simple, I suppose it won't be hard to > > add a Loader that can load an app on a particular thread. This would be the > > cleanest solution at this point. > > > > I know Ken is planning to refactor all these stuff so he didn't recommend to > use > > CurrentThreadLoader, let alone adding one. But this is currently blocking > Alop, > > and the functionality we are adding is a typical use case that has emerged > > before (see crbug.com/521754). > > We shouldn't be creating new threads for different mojo applications. This won't > scale, and is counter to the Lucky Luke project which aims to cut down threads > in the browser process. > > Even though CurrentThreadLoader is going away, it seems that the work to convert > existing users won't increase much more if this media stuff also used it. To be clear, CurrentThreadLoader is not appropriate for mojo media application because we do not want to run it on the browser UI thread. We want to run it on an existing media thread. One way to do that would be extend StaticLoader to accept a task runner. If the provided task runner is NULL, then it would launch the application on a dedicated thread. Another option would be what xhwang@ suggested - define a new Loader. Or perhaps morph CurrentThreadLoader into ExistingThreadLoader, that can also serve the purpose of CurrentThreadLoader. Ken/John: What is the timeline for the refactor? If it is couple of weeks, I can unblock the cast team by doing this just for cast media application, which would avoid unnecessary work in extending the loaders that will be deprecated soon.
On Mon, Apr 4, 2016 at 2:05 PM, <alokp@chromium.org> wrote: > On 2016/04/04 16:58:37, jam wrote: > > On 2016/04/02 00:17:07, xhwang (OOO Apr4 - Apr8) wrote: > > > There is also a CurrentThreadLoader which can load the app on the > "current" > > > thread, which in this case is the browser UI thread. Given > StaticThreadLoader > > > and CurrentThreadLoader are both fairly simple, I suppose it won't be > hard > to > > > add a Loader that can load an app on a particular thread. This would > be the > > > cleanest solution at this point. > > > > > > I know Ken is planning to refactor all these stuff so he didn't > recommend to > > use > > > CurrentThreadLoader, let alone adding one. But this is currently > blocking > > Alop, > > > and the functionality we are adding is a typical use case that has > emerged > > > before (see crbug.com/521754). > > > > We shouldn't be creating new threads for different mojo applications. > This > won't > > scale, and is counter to the Lucky Luke project which aims to cut down > threads > > in the browser process. > > > > Even though CurrentThreadLoader is going away, it seems that the work to > convert > > existing users won't increase much more if this media stuff also used it. > > To be clear, CurrentThreadLoader is not appropriate for mojo media > application > because we do not want to run it on the browser UI thread. We want to run > it on > an existing media thread. One way to do that would be extend StaticLoader to > accept a task runner. If the provided task runner is NULL, then it would > launch > the application on a dedicated thread. Another option would be what xhwang@ > suggested - define a new Loader. Or perhaps morph CurrentThreadLoader into > ExistingThreadLoader, that can also serve the purpose of > CurrentThreadLoader. > > Ken/John: What is the timeline for the refactor? If it is couple of weeks, > I can > unblock the cast team by doing this just for cast media application, which > would > avoid unnecessary work in extending the loaders that will be deprecated > soon. > The refactor itself is pretty trivial, it's just blocking on some design issues that should be resolved quickly once I'm back in office (2 days). Loader is a redundant mechanism for fulfilling ShellClientRequests, and I want to eliminate it in favor of a more robust mechanism. If you want to move forward quickly, extending StaticLoader sounds fine to me. The threading problem/solution is really independent of the refactor: with or without Loader, you're going to have a ShellClientRequest you can bind on whatever thread you like. > https://codereview.chromium.org/1854893002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> The refactor itself is pretty trivial, it's just blocking on some design > issues that should be resolved quickly once I'm back in office (2 days). > Loader is a redundant mechanism for fulfilling ShellClientRequests, and I > want to eliminate it in favor of a more robust mechanism. > > If you want to move forward quickly, extending StaticLoader sounds fine to > me. The threading problem/solution is really independent of the refactor: > with or without Loader, you're going to have a ShellClientRequest you can > bind on whatever thread you like. > Ken: Thanks for your reply. I will go ahead and do this for cast media application to unblock ourselves. I will redo this once the new loader mechanism in place. Enjoy your vacation!
Description was changed from ========== Allow MojoMediaClient specify media task runner. BUG=594234 ========== to ========== [chromecast] Bind and run mojo media services on cma thread. BUG=594234 ==========
Description was changed from ========== [chromecast] Bind and run mojo media services on cma thread. BUG=594234 ========== to ========== [chromecast] Bind and run mojo media services on cma thread. BUG=594234 ==========
alokp@chromium.org changed reviewers: + ben@chromium.org, halliwell@chromium.org - jam@chromium.org, rockot@chromium.org, xhwang@chromium.org
halliwell: chromecast/ ben: mojo/shell/public DEPS
https://codereview.chromium.org/1854893002/diff/80001/chromecast/browser/medi... File chromecast/browser/media/cast_mojo_media_application.cc (right): https://codereview.chromium.org/1854893002/diff/80001/chromecast/browser/medi... chromecast/browser/media/cast_mojo_media_application.cc:49: base::Bind(&CastMojoMediaApplication::Create, base::Unretained(this), I'm not familiar with the lifetime of 'this' - is Unretained definitely safe here? https://codereview.chromium.org/1854893002/diff/80001/chromecast/browser/medi... File chromecast/browser/media/cast_mojo_media_application.h (right): https://codereview.chromium.org/1854893002/diff/80001/chromecast/browser/medi... chromecast/browser/media/cast_mojo_media_application.h:28: scoped_refptr<base::SingleThreadTaskRunner> media_task_runner); nit, #includes for scoped_ptr, scoped_refptr, SingleThreadTaskRunner?
https://codereview.chromium.org/1854893002/diff/80001/chromecast/browser/medi... File chromecast/browser/media/cast_mojo_media_application.cc (right): https://codereview.chromium.org/1854893002/diff/80001/chromecast/browser/medi... chromecast/browser/media/cast_mojo_media_application.cc:49: base::Bind(&CastMojoMediaApplication::Create, base::Unretained(this), On 2016/04/05 00:01:56, halliwell wrote: > I'm not familiar with the lifetime of 'this' - is Unretained definitely safe > here? It wasn't. In fact there was another bug that would cause ref_factory_ to make media message loop to quit. I have fixed it in the latest patch and added comments. https://codereview.chromium.org/1854893002/diff/80001/chromecast/browser/medi... File chromecast/browser/media/cast_mojo_media_application.h (right): https://codereview.chromium.org/1854893002/diff/80001/chromecast/browser/medi... chromecast/browser/media/cast_mojo_media_application.h:28: scoped_refptr<base::SingleThreadTaskRunner> media_task_runner); On 2016/04/05 00:01:56, halliwell wrote: > nit, #includes for scoped_ptr, scoped_refptr, SingleThreadTaskRunner? Done.
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
I am not a big fan of this approach: - We'll still have an extra thread only for the app while everything in the app is actually running on the media thread. - The threading model in the app is a bit tricky (even though it might be perfectly safe after careful inspection). - We are duplicating MojoMediaApp into CastMojoMediaApp only to be able to run it on an existing thread. It seems like Ken's suggestion was to extend StaticLoader into an ExistingThreadLoader. Can we explore in that direction a bit more? I can try it in the next 2 hours and see where we can get. If that's working, then we only need to a different way to register the app in BrowserContentClient. WDYT? If this is working, I'd like to use it for our media app running in the GPU process on Android as well to avoid creating an extra thread: https://code.google.com/p/chromium/codesearch#chromium/src/content/gpu/gpu_pr...
On 2016/04/05 05:48:47, xhwang (OOO Apr4 - Apr8) wrote: > I am not a big fan of this approach: > - We'll still have an extra thread only for the app while everything in the app > is actually running on the media thread. > - The threading model in the app is a bit tricky (even though it might be > perfectly safe after careful inspection). I am aware that we will have an extra thread. But currently there is no way to avoid that. We could go off and extend StaticLoader, but I am worried that it will all be wasted work once Loader is deprecated. This is a temporary workaround to unblock us. I will revisit this once Ken is done with his refactor. > - We are duplicating MojoMediaApp into CastMojoMediaApp only to be able to run > it on an existing thread. I had to create a CastMojoMediaApp anyway to add cast-specific media services in addition to the ones hosted by MojoMediaApp. Once Ken is done refactoring Loader and we can agree on how to add this support in MojoMediaApp, I will derive CastMojoMediaApp from MojoMediaApp to avoid duplicated code. That said, there is not much code in MojoMediaApp to start with. Most of the tricky logic is under ServiceFactoryImpl layer that is not being duplicated. > It seems like Ken's suggestion was to extend StaticLoader into an > ExistingThreadLoader. Can we explore in that direction a bit more? I can try it > in the next 2 hours and see where we can get. If that's working, then we only > need to a different way to register the app in BrowserContentClient. WDYT? > > If this is working, I'd like to use it for our media app running in the GPU > process on Android as well to avoid creating an extra thread: > https://code.google.com/p/chromium/codesearch#chromium/src/content/gpu/gpu_pr... I think it would be better to wait until the Loader refactor is done.
On 2016/04/05 06:05:21, alokp wrote: > On 2016/04/05 05:48:47, xhwang (OOO Apr4 - Apr8) wrote: > > I am not a big fan of this approach: > > - We'll still have an extra thread only for the app while everything in the > app > > is actually running on the media thread. > > - The threading model in the app is a bit tricky (even though it might be > > perfectly safe after careful inspection). > > I am aware that we will have an extra thread. But currently there is no way to > avoid that. We could go off and extend StaticLoader, but I am worried that it > will all be wasted work once Loader is deprecated. This is a temporary > workaround to unblock us. I will revisit this once Ken is done with his > refactor. I tried the idea of ExistingThreadLoader and it seems pretty straightforward. I tried it and can get mojo media app running on the main thread (an existing thread) or a test thread in the utility process. Impl: https://chromiumcodereview.appspot.com/1856313002/ Test: https://chromiumcodereview.appspot.com/1857103003/ > > - We are duplicating MojoMediaApp into CastMojoMediaApp only to be able to run > > it on an existing thread. > > I had to create a CastMojoMediaApp anyway to add cast-specific media services in > addition to the ones hosted by MojoMediaApp. Once Ken is done refactoring Loader > and we can agree on how to add this support in MojoMediaApp, I will derive > CastMojoMediaApp from MojoMediaApp to avoid duplicated code. That said, there is > not much code in MojoMediaApp to start with. Most of the tricky logic is under > ServiceFactoryImpl layer that is not being duplicated. Thanks for the info! OOC, what "cast-specific media services" will we have in the application? I wonder whether we could have better ways to support additional services for embedders (e.g. via the MojoMediaClient). > > It seems like Ken's suggestion was to extend StaticLoader into an > > ExistingThreadLoader. Can we explore in that direction a bit more? I can try > it > > in the next 2 hours and see where we can get. If that's working, then we only > > need to a different way to register the app in BrowserContentClient. WDYT? > > > > If this is working, I'd like to use it for our media app running in the GPU > > process on Android as well to avoid creating an extra thread: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/gpu/gpu_pr... > > I think it would be better to wait until the Loader refactor is done. We are shipping mojo media app on Android. If this can help reduce an extra thread it is at least worth trying for me for M51.
> I tried the idea of ExistingThreadLoader and it seems pretty straightforward. I > tried it and can get mojo media app running on the main thread (an existing > thread) or a test thread in the utility process. > > Impl: https://chromiumcodereview.appspot.com/1856313002/ > Test: https://chromiumcodereview.appspot.com/1857103003/ > Thanks for the patch. I left comments there. > Thanks for the info! OOC, what "cast-specific media services" will we have in > the application? I wonder whether we could have better ways to support > additional services for embedders (e.g. via the MojoMediaClient). > We have two at present - volume controller and audio loopback. I am sure we will add more over time.
Ben: friendly ping about DEPS.
On 2016/04/05 15:54:38, alokp wrote: > > I tried the idea of ExistingThreadLoader and it seems pretty straightforward. > I > > tried it and can get mojo media app running on the main thread (an existing > > thread) or a test thread in the utility process. > > > > Impl: https://chromiumcodereview.appspot.com/1856313002/ > > Test: https://chromiumcodereview.appspot.com/1857103003/ > > > > Thanks for the patch. I left comments there. > > > Thanks for the info! OOC, what "cast-specific media services" will we have in > > the application? I wonder whether we could have better ways to support > > additional services for embedders (e.g. via the MojoMediaClient). > > > > We have two at present - volume controller and audio loopback. I am sure we will > add more over time. lgtm
alokp@chromium.org changed reviewers: + amistry@chromium.org, rockot@chromium.org - ben@chromium.org
amistry/rockot: DEPS on mojo/shell/public
lgtm
The CQ bit was checked by alokp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1854893002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1854893002/120001
Message was sent while issue was closed.
Description was changed from ========== [chromecast] Bind and run mojo media services on cma thread. BUG=594234 ========== to ========== [chromecast] Bind and run mojo media services on cma thread. BUG=594234 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [chromecast] Bind and run mojo media services on cma thread. BUG=594234 ========== to ========== [chromecast] Bind and run mojo media services on cma thread. BUG=594234 Committed: https://crrev.com/587432023fe05823c2ecfbdc64d37657e76817f1 Cr-Commit-Position: refs/heads/master@{#385392} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/587432023fe05823c2ecfbdc64d37657e76817f1 Cr-Commit-Position: refs/heads/master@{#385392}
Message was sent while issue was closed.
On 2016/04/05 15:54:38, alokp wrote: > > I tried the idea of ExistingThreadLoader and it seems pretty straightforward. > I > > tried it and can get mojo media app running on the main thread (an existing > > thread) or a test thread in the utility process. > > > > Impl: https://chromiumcodereview.appspot.com/1856313002/ > > Test: https://chromiumcodereview.appspot.com/1857103003/ > > > > Thanks for the patch. I left comments there. > > > Thanks for the info! OOC, what "cast-specific media services" will we have in > > the application? I wonder whether we could have better ways to support > > additional services for embedders (e.g. via the MojoMediaClient). > > > > We have two at present - volume controller and audio loopback. I am sure we will > add more over time. Thanks! Do you have more details about how these services work, e.g. in a doc?
Message was sent while issue was closed.
+kmackay Ken: Do we have any design doc for audio loopback service?
Message was sent while issue was closed.
On 2016/04/11 21:09:46, alokp wrote: > +kmackay > > Ken: Do we have any design doc for audio loopback service? Not really. We just need a way to read post-mixer audio output data. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
