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

Issue 2526953003: Add MediaGpuChannelManager::LookupChannel(). (Closed)

Created:
4 years ago by sandersd (OOO until July 31)
Modified:
3 years, 11 months ago
Reviewers:
tguilbert, dcheng
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, Aaron Boodman, posciak+watch_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, feature-media-reviews_chromium.org, 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

Add MediaGpuChannelManager::LookupChannel(). This adds a path for VDAv2 clients to look up a command buffer identified by |command_buffer_id|. Also in this CL is a method to look up the route ID from the accelerated GPU factories so that it's easy to pass the |command_buffer_id|. 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

Patch Set 1 #

Patch Set 2 : Rebase. #

Patch Set 3 : Rebase. #

Total comments: 1

Patch Set 4 : Document GetCommandBufferRouteId() better. #

Total comments: 6

Patch Set 5 : Requested changes. #

Total comments: 4

Patch Set 6 : Implement MockGpuVideoAcceleratorFactories::GetCommandBufferRouteId(). #

Unified diffs Side-by-side diffs Delta from patch set Stats (+94 lines, -17 lines) Patch
M content/renderer/media/renderer_gpu_video_accelerator_factories.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/media/renderer_gpu_video_accelerator_factories.cc View 1 2 3 4 1 chunk +7 lines, -0 lines 0 comments Download
M media/gpu/ipc/service/media_gpu_channel.h View 1 2 3 4 3 chunks +4 lines, -1 line 0 comments Download
M media/gpu/ipc/service/media_gpu_channel.cc View 1 2 3 4 1 chunk +4 lines, -3 lines 0 comments Download
M media/gpu/ipc/service/media_gpu_channel_manager.h View 1 2 3 4 2 chunks +7 lines, -0 lines 0 comments Download
M media/gpu/ipc/service/media_gpu_channel_manager.cc View 1 2 3 4 1 chunk +19 lines, -1 line 0 comments Download
M media/mojo/clients/mojo_video_decoder.h View 1 chunk +3 lines, -1 line 0 comments Download
M media/mojo/clients/mojo_video_decoder.cc View 1 2 3 chunks +16 lines, -3 lines 0 comments Download
M media/mojo/interfaces/video_decoder.mojom View 1 2 3 4 1 chunk +14 lines, -1 line 0 comments Download
M media/mojo/services/gpu_mojo_media_client.h View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M media/mojo/services/gpu_mojo_media_client.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M media/mojo/services/mojo_media_client.h View 1 2 3 4 2 chunks +3 lines, -1 line 0 comments Download
M media/mojo/services/mojo_media_client.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M media/mojo/services/mojo_video_decoder_service.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M media/mojo/services/mojo_video_decoder_service.cc View 1 2 3 4 1 chunk +4 lines, -2 lines 0 comments Download
M media/renderers/gpu_video_accelerator_factories.h View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M media/renderers/mock_gpu_video_accelerator_factories.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 25 (12 generated)
tguilbert
LGTM I however do not have strong feelings as to the API names (and am ...
3 years, 11 months ago (2017-01-10 01:16:36 UTC) #4
sandersd (OOO until July 31)
dcheng@: Please security review changes to video_decoder.mojom. This change finally hooks up all the steps ...
3 years, 11 months ago (2017-01-10 01:36:58 UTC) #6
dcheng
https://codereview.chromium.org/2526953003/diff/60001/content/renderer/media/renderer_gpu_video_accelerator_factories.cc File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://codereview.chromium.org/2526953003/diff/60001/content/renderer/media/renderer_gpu_video_accelerator_factories.cc#newcode132 content/renderer/media/renderer_gpu_video_accelerator_factories.cc:132: if (!context_provider_) How come most other things use CheckContextLost()? ...
3 years, 11 months ago (2017-01-10 06:03:42 UTC) #7
sandersd (OOO until July 31)
https://codereview.chromium.org/2526953003/diff/60001/content/renderer/media/renderer_gpu_video_accelerator_factories.cc File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://codereview.chromium.org/2526953003/diff/60001/content/renderer/media/renderer_gpu_video_accelerator_factories.cc#newcode132 content/renderer/media/renderer_gpu_video_accelerator_factories.cc:132: if (!context_provider_) On 2017/01/10 06:03:42, dcheng wrote: > How ...
3 years, 11 months ago (2017-01-10 23:54:14 UTC) #8
dcheng
For future CLs, it would be better if the full implementation backing the IPC is ...
3 years, 11 months ago (2017-01-11 09:14:19 UTC) #10
sandersd (OOO until July 31)
> For future CLs, it would be better if the full implementation backing the > ...
3 years, 11 months ago (2017-01-11 21:33:09 UTC) #11
dcheng
On 2017/01/11 21:33:09, sandersd wrote: > > For future CLs, it would be better if ...
3 years, 11 months ago (2017-01-11 22:01:59 UTC) #12
sandersd (OOO until July 31)
> From the perspective of a security review, it's not just the interfaces > being ...
3 years, 11 months ago (2017-01-11 22:21:31 UTC) #13
dcheng
On 2017/01/11 22:21:31, sandersd wrote: > > From the perspective of a security review, it's ...
3 years, 11 months ago (2017-01-11 22:36:31 UTC) #14
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/2526953003/100001
3 years, 11 months ago (2017-01-11 22:40:12 UTC) #17
commit-bot: I haz the power
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_compile_dbg_ng/builds/335795)
3 years, 11 months ago (2017-01-11 22:59:35 UTC) #19
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/2526953003/120001
3 years, 11 months ago (2017-01-11 23:05:24 UTC) #22
commit-bot: I haz the power
3 years, 11 months ago (2017-01-12 01:18:59 UTC) #25
Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.

Powered by Google App Engine
This is Rietveld 408576698