|
|
Created:
4 years, 2 months ago by sandersd (OOO until July 31) Modified:
4 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, alokp+watch_chromium.org, piman+watch_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Add GpuMojoMediaClient.
This CL introduces GpuMojoMediaClient, a factory for media objects used by MojoMediaService in the GPU process. The MediaGpuChannelManager is plumbed through to GpuMojoMediaClient so that (in a followup CL) services can look up command buffers.
BUG=522298
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/49bcb723fd4305c26c4013f209315ddd35e8c9dc
Cr-Commit-Position: refs/heads/master@{#436713}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add GpuMojoMediaClient. #
Total comments: 14
Patch Set 3 : Use WeakPtr. #
Total comments: 3
Patch Set 4 : Address nits. #
Total comments: 1
Patch Set 5 : nogncheck #
Messages
Total messages: 37 (21 generated)
sandersd@chromium.org changed reviewers: + xhwang@chromium.org
Just the first bit of plumbing for validating the design.
Mostly looking good; I just have one comment about dependency. https://chromiumcodereview.appspot.com/2382103002/diff/1/media/mojo/services/... File media/mojo/services/mojo_media_application_factory.cc (right): https://chromiumcodereview.appspot.com/2382103002/diff/1/media/mojo/services/... media/mojo/services/mojo_media_application_factory.cc:23: MediaService* media_service, This will make media/mojo depend on media/gpu which in turn depends on /gpu and /ui. This makes me wonder whether you can provide your own function to create MojoMediaApplication similar to what Cast is doing [2]? [1] https://cs.chromium.org/chromium/src/media/gpu/BUILD.gn?rcl=0&l=181 [2] https://cs.chromium.org/chromium/src/chromecast/browser/cast_content_browser_...
https://chromiumcodereview.appspot.com/2382103002/diff/1/media/mojo/services/... File media/mojo/services/mojo_media_application_factory.cc (right): https://chromiumcodereview.appspot.com/2382103002/diff/1/media/mojo/services/... media/mojo/services/mojo_media_application_factory.cc:23: MediaService* media_service, On 2016/09/30 22:39:11, xhwang wrote: > This will make media/mojo depend on media/gpu which in turn depends on /gpu and > /ui. This makes me wonder whether you can provide your own function to create > MojoMediaApplication similar to what Cast is doing [2]? > > [1] https://cs.chromium.org/chromium/src/media/gpu/BUILD.gn?rcl=0&l=181 > > [2] > https://cs.chromium.org/chromium/src/chromecast/browser/cast_content_browser_... Good point. In this case we can pass it as a raw pointer the whole way, and so can get away with just forward declaration everywhere. Would that be considered acceptable?
I guess then your client will be gpu specific and can use anything in media/gpu, /gpu etc. Once you are on that path, I don't really care what you pass in :) On Fri, Sep 30, 2016 at 3:40 PM, <sandersd@chromium.org> wrote: > > https://chromiumcodereview.appspot.com/2382103002/diff/1/ > media/mojo/services/mojo_media_application_factory.cc > File media/mojo/services/mojo_media_application_factory.cc (right): > > https://chromiumcodereview.appspot.com/2382103002/diff/1/ > media/mojo/services/mojo_media_application_factory.cc#newcode23 > media/mojo/services/mojo_media_application_factory.cc:23: MediaService* > media_service, > On 2016/09/30 22:39:11, xhwang wrote: > > This will make media/mojo depend on media/gpu which in turn depends on > /gpu and > > /ui. This makes me wonder whether you can provide your own function to > create > > MojoMediaApplication similar to what Cast is doing [2]? > > > > [1] > https://cs.chromium.org/chromium/src/media/gpu/BUILD.gn?rcl=0&l=181 > > > > [2] > > > https://cs.chromium.org/chromium/src/chromecast/ > browser/cast_content_browser_client.cc?rcl=0&l=76 > > Good point. In this case we can pass it as a raw pointer the whole way, > and so can get away with just forward declaration everywhere. Would that > be considered acceptable? > > https://chromiumcodereview.appspot.com/2382103002/ > -- 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.
Description was changed from ========== Plumb MediaService. BUG=522298 ========== to ========== Add GpuMojoMediaClient. BUG=522298 ==========
PTAL. I have added a GpuMojoMediaClient and plumbed all of the relevant things. I shouldn't need to add any deps on gpu/ for the final implementation, as long as I don't need to peek inside the MediaGpuChannelManager*. Perhaps you see a way to improve the decoupling of gpu/media/mojo here? https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/gpu... File media/mojo/services/gpu_mojo_media_client.cc (right): https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/gpu... media/mojo/services/gpu_mojo_media_client.cc:55: (void)media_gpu_channel_manager_; This just avoids the compiler warning for unused private member; once it's passed to a VideoDecoder this shouldn't be needed anymore.
https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/BUI... File media/mojo/services/BUILD.gn (right): https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/BUI... media/mojo/services/BUILD.gn:56: "gpu_mojo_media_client.h", Main deps concern is here, since the GpuMojoMediaClient may some day take on a variety of deps to implement its factory. The main alternative is to move this to //content/gpu, but then //content/gpu would start growing media deps.
Mostly looking good; I just have a few nits/comments. https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_child_t... File content/gpu/gpu_child_thread.cc (right): https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_child_t... content/gpu/gpu_child_thread.cc:363: media_gpu_channel_manager_.get())); service_factory_ should be destroyed before media_gpu_channel_manager_ since it's holding the raw ptr. Please add comments in the .h file to make this clear (just in case someone switch the declaration order). https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_service... File content/gpu/gpu_service_factory.h (right): https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_service... content/gpu/gpu_service_factory.h:22: GpuServiceFactory(media::MediaGpuChannelManager* media_gpu_channel_manager); nit: explicit https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_service... content/gpu/gpu_service_factory.h:29: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; please add comment. is this class single threaded and everything runs on |task_runner|? or is this runner used for something else? https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/BUI... File media/mojo/services/BUILD.gn (right): https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/BUI... media/mojo/services/BUILD.gn:56: "gpu_mojo_media_client.h", On 2016/11/22 23:12:16, sandersd wrote: > Main deps concern is here, since the GpuMojoMediaClient may some day take on a > variety of deps to implement its factory. The main alternative is to move this > to //content/gpu, but then //content/gpu would start growing media deps. Thanks for bringing this up. Either way it looks good to me. I guess this is totally up to you. Also, content depends on media and media/mojo. I don't see anything wrong to have content/gpu depending on media stuff :) That being said, it won't hurt to add a content/gpu owner to confirm :) https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/gpu... File media/mojo/services/gpu_mojo_media_client.cc (right): https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/gpu... media/mojo/services/gpu_mojo_media_client.cc:32: #endif // defined(OS_ANDROID) It would be nice to avoid the duplication between this file and the AndroidMojoMediaClient. But I don't have any concrete idea ...
Description was changed from ========== Add GpuMojoMediaClient. BUG=522298 ========== to ========== Add GpuMojoMediaClient. BUG=522298 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Add GpuMojoMediaClient. BUG=522298 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media: Add GpuMojoMediaClient. This CL introduces GpuMojoMediaClient, a factory for media objects used by MojoMediaService in the GPU process. BUG=522298 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== media: Add GpuMojoMediaClient. This CL introduces GpuMojoMediaClient, a factory for media objects used by MojoMediaService in the GPU process. BUG=522298 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media: Add GpuMojoMediaClient. This CL introduces GpuMojoMediaClient, a factory for media objects used by MojoMediaService in the GPU process. The MediaGpuChannelManager is plumbed through to GpuMojoMediaClient so that (in a followup CL) services can look up command buffers. BUG=522298 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_child_t... File content/gpu/gpu_child_thread.cc (right): https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_child_t... content/gpu/gpu_child_thread.cc:363: media_gpu_channel_manager_.get())); On 2016/11/23 06:58:03, xhwang wrote: > service_factory_ should be destroyed before media_gpu_channel_manager_ since > it's holding the raw ptr. Please add comments in the .h file to make this clear > (just in case someone switch the declaration order). This is actually safe due to the details of how the GPU child thread shuts down, but I've changed everything over to use weak pointers. This makes the expected usage pattern much clearer. https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_service... File content/gpu/gpu_service_factory.h (right): https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_service... content/gpu/gpu_service_factory.h:22: GpuServiceFactory(media::MediaGpuChannelManager* media_gpu_channel_manager); On 2016/11/23 06:58:03, xhwang wrote: > nit: explicit Done. https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_service... content/gpu/gpu_service_factory.h:29: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2016/11/23 06:58:03, xhwang wrote: > please add comment. is this class single threaded and everything runs on > |task_runner|? or is this runner used for something else? Done. https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/BUI... File media/mojo/services/BUILD.gn (right): https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/BUI... media/mojo/services/BUILD.gn:56: "gpu_mojo_media_client.h", On 2016/11/23 06:58:03, xhwang wrote: > On 2016/11/22 23:12:16, sandersd wrote: > > Main deps concern is here, since the GpuMojoMediaClient may some day take on a > > variety of deps to implement its factory. The main alternative is to move this > > to //content/gpu, but then //content/gpu would start growing media deps. > > Thanks for bringing this up. Either way it looks good to me. I guess this is > totally up to you. > > Also, content depends on media and media/mojo. I don't see anything wrong to > have content/gpu depending on media stuff :) > > That being said, it won't hurt to add a content/gpu owner to confirm :) In that case I'll just keep going, and we can worry about this when there are specific deps worth discussing ;-) https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/gpu... File media/mojo/services/gpu_mojo_media_client.cc (right): https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/gpu... media/mojo/services/gpu_mojo_media_client.cc:32: #endif // defined(OS_ANDROID) On 2016/11/23 06:58:03, xhwang wrote: > It would be nice to avoid the duplication between this file and the > AndroidMojoMediaClient. But I don't have any concrete idea ... It should be possible to just delete it soon. The remaining construction paths are just: - UtilityServiceFactory (Presumably not on Android?) - ChromeContentBrowserClient (browser process)
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
Thanks! Just a few more comments/nits. Also, you need to fix the deps. https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/gpu... File media/mojo/services/gpu_mojo_media_client.cc (right): https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/gpu... media/mojo/services/gpu_mojo_media_client.cc:32: #endif // defined(OS_ANDROID) On 2016/11/23 21:21:02, sandersd wrote: > On 2016/11/23 06:58:03, xhwang wrote: > > It would be nice to avoid the duplication between this file and the > > AndroidMojoMediaClient. But I don't have any concrete idea ... > > It should be possible to just delete it soon. The remaining construction paths > are just: > - UtilityServiceFactory (Presumably not on Android?) > - ChromeContentBrowserClient (browser process) I am a bit lost. Do you mean the "construction paths" of AndroidMojoMediaClient? Or something else? https://codereview.chromium.org/2382103002/diff/60001/content/gpu/gpu_child_t... File content/gpu/gpu_child_thread.cc (right): https://codereview.chromium.org/2382103002/diff/60001/content/gpu/gpu_child_t... content/gpu/gpu_child_thread.cc:363: new GpuServiceFactory(media_gpu_channel_manager_.get()->AsWeakPtr())); no need for .get()? https://codereview.chromium.org/2382103002/diff/60001/content/gpu/gpu_service... File content/gpu/gpu_service_factory.h (right): https://codereview.chromium.org/2382103002/diff/60001/content/gpu/gpu_service... content/gpu/gpu_service_factory.h:36: base::WeakPtr<media::MediaGpuChannelManager> media_gpu_channel_manager_; You can probably guard these two with #if defined(ENABLE_MOJO_MEDIA_IN_GPU_PROCESS) as well, then compiler won't complain. The downside of this is that the constructor would be a bit messy.
Patchset #4 (id:80001) has been deleted
> Also, you need to fix the deps. Previously this was done using //nogncheck. Do you happen to know why that was? https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/gpu... File media/mojo/services/gpu_mojo_media_client.cc (right): https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/gpu... media/mojo/services/gpu_mojo_media_client.cc:32: #endif // defined(OS_ANDROID) On 2016/11/24 07:02:10, xhwang wrote: > On 2016/11/23 21:21:02, sandersd wrote: > > On 2016/11/23 06:58:03, xhwang wrote: > > > It would be nice to avoid the duplication between this file and the > > > AndroidMojoMediaClient. But I don't have any concrete idea ... > > > > It should be possible to just delete it soon. The remaining construction paths > > are just: > > - UtilityServiceFactory (Presumably not on Android?) > > - ChromeContentBrowserClient (browser process) > > I am a bit lost. Do you mean the "construction paths" of AndroidMojoMediaClient? > Or something else? Yes, that's what I mean. The paths that construct an AndroidMojoMediaClient are limited, and I expect we can eliminate them. ChromeContentBrowserClient I don't know much about, so I could be wrong that it will be easy to eliminate that path. https://codereview.chromium.org/2382103002/diff/60001/content/gpu/gpu_child_t... File content/gpu/gpu_child_thread.cc (right): https://codereview.chromium.org/2382103002/diff/60001/content/gpu/gpu_child_t... content/gpu/gpu_child_thread.cc:363: new GpuServiceFactory(media_gpu_channel_manager_.get()->AsWeakPtr())); On 2016/11/24 07:02:11, xhwang wrote: > no need for .get()? Done.
sorry for the delay. lgtm with comments on the deps https://codereview.chromium.org/2382103002/diff/100001/content/gpu/gpu_servic... File content/gpu/gpu_service_factory.cc (right): https://codereview.chromium.org/2382103002/diff/100001/content/gpu/gpu_servic... content/gpu/gpu_service_factory.cc:13: #include "media/mojo/services/media_service_factory.h" We had // nogncheck because gn isn't smart enough about conditional deps, see "gn help nogncheck". The conditional deps is at: https://cs.chromium.org/chromium/src/content/gpu/BUILD.gn?rcl=0&l=77
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sandersd@chromium.org changed reviewers: + nasko@chromium.org
nasko@chromium.org: Please review changes in //content/gpu/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I don't have much knowledge of GPU, but the content/ changes look mostly mechanical, so LGTM. Feel free to loop in more knowledgable reviewers if my assessment is incorrect.
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2382103002/#ps120001 (title: "nogncheck")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481051193654730, "parent_rev": "af546b9201b3ea9ce9140809716678911332c216", "commit_rev": "e969a6e4a9a26e81496089249940f2e632415c14"}
Message was sent while issue was closed.
Description was changed from ========== media: Add GpuMojoMediaClient. This CL introduces GpuMojoMediaClient, a factory for media objects used by MojoMediaService in the GPU process. The MediaGpuChannelManager is plumbed through to GpuMojoMediaClient so that (in a followup CL) services can look up command buffers. BUG=522298 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media: Add GpuMojoMediaClient. This CL introduces GpuMojoMediaClient, a factory for media objects used by MojoMediaService in the GPU process. The MediaGpuChannelManager is plumbed through to GpuMojoMediaClient so that (in a followup CL) services can look up command buffers. BUG=522298 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== media: Add GpuMojoMediaClient. This CL introduces GpuMojoMediaClient, a factory for media objects used by MojoMediaService in the GPU process. The MediaGpuChannelManager is plumbed through to GpuMojoMediaClient so that (in a followup CL) services can look up command buffers. BUG=522298 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media: Add GpuMojoMediaClient. This CL introduces GpuMojoMediaClient, a factory for media objects used by MojoMediaService in the GPU process. The MediaGpuChannelManager is plumbed through to GpuMojoMediaClient so that (in a followup CL) services can look up command buffers. BUG=522298 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/49bcb723fd4305c26c4013f209315ddd35e8c9dc Cr-Commit-Position: refs/heads/master@{#436713} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/49bcb723fd4305c26c4013f209315ddd35e8c9dc Cr-Commit-Position: refs/heads/master@{#436713} |