Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(598)

Issue 2382103002: media: Add GpuMojoMediaClient. (Closed)

Created:
4 years, 2 months ago by sandersd (OOO until July 31)
Modified:
4 years ago
Reviewers:
xhwang, nasko
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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+177 lines, -7 lines) Patch
M content/gpu/gpu_child_thread.cc View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M content/gpu/gpu_service_factory.h View 1 2 3 1 chunk +18 lines, -1 line 0 comments Download
M content/gpu/gpu_service_factory.cc View 1 2 3 4 1 chunk +13 lines, -3 lines 0 comments Download
M media/gpu/ipc/service/media_gpu_channel_manager.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M media/mojo/services/BUILD.gn View 1 1 chunk +2 lines, -0 lines 0 comments Download
A media/mojo/services/gpu_mojo_media_client.h View 1 2 1 chunk +46 lines, -0 lines 0 comments Download
A media/mojo/services/gpu_mojo_media_client.cc View 1 2 3 1 chunk +71 lines, -0 lines 0 comments Download
M media/mojo/services/media_service_factory.h View 1 2 1 chunk +13 lines, -1 line 0 comments Download
M media/mojo/services/media_service_factory.cc View 1 2 2 chunks +9 lines, -0 lines 0 comments Download

Messages

Total messages: 37 (21 generated)
sandersd (OOO until July 31)
Just the first bit of plumbing for validating the design.
4 years, 2 months ago (2016-09-30 00:55:42 UTC) #2
xhwang
Mostly looking good; I just have one comment about dependency. 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 ...
4 years, 2 months ago (2016-09-30 22:39:11 UTC) #3
sandersd (OOO until July 31)
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 ...
4 years, 2 months ago (2016-09-30 22:40:50 UTC) #4
xhwang
I guess then your client will be gpu specific and can use anything in media/gpu, ...
4 years, 2 months ago (2016-09-30 22:46:32 UTC) #5
sandersd (OOO until July 31)
PTAL. I have added a GpuMojoMediaClient and plumbed all of the relevant things. I shouldn't ...
4 years, 1 month ago (2016-11-22 23:08:00 UTC) #7
sandersd (OOO until July 31)
https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/BUILD.gn File media/mojo/services/BUILD.gn (right): https://codereview.chromium.org/2382103002/diff/20001/media/mojo/services/BUILD.gn#newcode56 media/mojo/services/BUILD.gn:56: "gpu_mojo_media_client.h", Main deps concern is here, since the GpuMojoMediaClient ...
4 years, 1 month ago (2016-11-22 23:12:16 UTC) #8
xhwang
Mostly looking good; I just have a few nits/comments. https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_child_thread.cc File content/gpu/gpu_child_thread.cc (right): https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_child_thread.cc#newcode363 content/gpu/gpu_child_thread.cc:363: ...
4 years ago (2016-11-23 06:58:03 UTC) #9
sandersd (OOO until July 31)
https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_child_thread.cc File content/gpu/gpu_child_thread.cc (right): https://codereview.chromium.org/2382103002/diff/20001/content/gpu/gpu_child_thread.cc#newcode363 content/gpu/gpu_child_thread.cc:363: media_gpu_channel_manager_.get())); On 2016/11/23 06:58:03, xhwang wrote: > service_factory_ should ...
4 years ago (2016-11-23 21:21:02 UTC) #14
xhwang
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_mojo_media_client.cc File ...
4 years ago (2016-11-24 07:02:10 UTC) #19
sandersd (OOO until July 31)
> Also, you need to fix the deps. Previously this was done using //nogncheck. Do ...
4 years ago (2016-11-28 00:56:15 UTC) #21
xhwang
sorry for the delay. lgtm with comments on the deps https://codereview.chromium.org/2382103002/diff/100001/content/gpu/gpu_service_factory.cc File content/gpu/gpu_service_factory.cc (right): https://codereview.chromium.org/2382103002/diff/100001/content/gpu/gpu_service_factory.cc#newcode13 ...
4 years ago (2016-11-29 00:12:18 UTC) #22
sandersd (OOO until July 31)
nasko@chromium.org: Please review changes in //content/gpu/
4 years ago (2016-12-03 01:43:18 UTC) #26
nasko
I don't have much knowledge of GPU, but the content/ changes look mostly mechanical, so ...
4 years ago (2016-12-06 15:10:46 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2382103002/120001
4 years ago (2016-12-06 19:07:12 UTC) #32
commit-bot: I haz the power
Committed patchset #5 (id:120001)
4 years ago (2016-12-06 20:52:42 UTC) #35
commit-bot: I haz the power
4 years ago (2016-12-06 20:54:26 UTC) #37
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/49bcb723fd4305c26c4013f209315ddd35e8c9dc
Cr-Commit-Position: refs/heads/master@{#436713}

Powered by Google App Engine
This is Rietveld 408576698