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

Issue 2101043003: Plumb callbacks from mojo media service to client. (Closed)

Created:
4 years, 5 months ago by slan
Modified:
4 years, 5 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, alokp+watch_chromium.org, darin (slow to review), ben+mojo_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Plumb callbacks from mojo media service to client. Add OnStatisticsUpdate and OnWaitingForDecryptionKey to mojom::RendererClient, and call these callbacks from MojoRendererService. These are needed for Cast Apps like Netflix that depend on decoding statistics from the media pipeline. BUG=585287 BUG= internal b/29390639 Committed: https://crrev.com/267e4634d0d1af89608e0c102a1683bcfb6bbb1f Cr-Commit-Position: refs/heads/master@{#404486}

Patch Set 1 #

Total comments: 11

Patch Set 2 : mojom typo fixed #

Patch Set 3 : VLOG levels adjusted #

Total comments: 2

Patch Set 4 : fwd-declare bugfix #

Patch Set 5 : Mock method update #

Total comments: 1

Patch Set 6 : Use type mapping #

Patch Set 7 : Remove unneeded deps from typemap file. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+104 lines, -3 lines) Patch
M chromecast/browser/media/cast_renderer.cc View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M media/mojo/clients/mojo_renderer.h View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M media/mojo/clients/mojo_renderer.cc View 1 2 3 4 5 2 chunks +13 lines, -0 lines 0 comments Download
M media/mojo/common/OWNERS View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
A media/mojo/common/pipeline_statistics_struct_traits.h View 1 2 3 4 5 1 chunk +49 lines, -0 lines 0 comments Download
M media/mojo/interfaces/media_types.mojom View 1 chunk +9 lines, -0 lines 2 comments Download
A media/mojo/interfaces/pipeline_statistics.typemap View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
M media/mojo/interfaces/renderer.mojom View 1 1 chunk +8 lines, -0 lines 0 comments Download
M media/mojo/interfaces/typemaps.gni View 1 2 3 4 5 1 chunk +4 lines, -1 line 0 comments Download
M media/mojo/services/media_mojo_unittest.cc View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M media/mojo/services/mojo_renderer_service.cc View 1 2 3 4 5 2 chunks +4 lines, -2 lines 0 comments Download

Messages

Total messages: 49 (18 generated)
slan
Fixes Netflix error on playback. PTAL!
4 years, 5 months ago (2016-06-27 21:43:20 UTC) #2
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101043003/1
4 years, 5 months ago (2016-06-27 21:45:59 UTC) #4
halliwell
https://codereview.chromium.org/2101043003/diff/1/media/mojo/interfaces/renderer.mojom File media/mojo/interfaces/renderer.mojom (right): https://codereview.chromium.org/2101043003/diff/1/media/mojo/interfaces/renderer.mojom#newcode67 media/mojo/interfaces/renderer.mojom:67: //OnWaitingForDecryptionKey(); uncomment?
4 years, 5 months ago (2016-06-27 21:47:20 UTC) #5
alokp
https://codereview.chromium.org/2101043003/diff/1/media/mojo/clients/mojo_renderer.cc File media/mojo/clients/mojo_renderer.cc (right): https://codereview.chromium.org/2101043003/diff/1/media/mojo/clients/mojo_renderer.cc#newcode245 media/mojo/clients/mojo_renderer.cc:245: DVLOG(2) << __FUNCTION__; This fires too often. I would ...
4 years, 5 months ago (2016-06-27 21:50:49 UTC) #6
alokp
https://codereview.chromium.org/2101043003/diff/1/media/mojo/common/media_type_converters.cc File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/2101043003/diff/1/media/mojo/common/media_type_converters.cc#newcode732 media/mojo/common/media_type_converters.cc:732: media::mojom::PipelineStatisticsPtr On 2016/06/27 21:50:48, alokp wrote: > Is it ...
4 years, 5 months ago (2016-06-27 22:09:57 UTC) #7
slan
https://codereview.chromium.org/2101043003/diff/1/media/mojo/clients/mojo_renderer.cc File media/mojo/clients/mojo_renderer.cc (right): https://codereview.chromium.org/2101043003/diff/1/media/mojo/clients/mojo_renderer.cc#newcode245 media/mojo/clients/mojo_renderer.cc:245: DVLOG(2) << __FUNCTION__; On 2016/06/27 21:50:48, alokp wrote: > ...
4 years, 5 months ago (2016-06-27 22:19:39 UTC) #8
slan
+sandersd in Xiaohan's stead. https://codereview.chromium.org/2101043003/diff/1/media/mojo/common/media_type_converters.cc File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/2101043003/diff/1/media/mojo/common/media_type_converters.cc#newcode732 media/mojo/common/media_type_converters.cc:732: media::mojom::PipelineStatisticsPtr On 2016/06/27 22:09:57, alokp ...
4 years, 5 months ago (2016-06-27 22:22:58 UTC) #10
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101043003/40001
4 years, 5 months ago (2016-06-27 22:24:21 UTC) #12
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/87861) linux_chromium_rel_ng on ...
4 years, 5 months ago (2016-06-27 22:44:29 UTC) #14
alokp
lgtm https://codereview.chromium.org/2101043003/diff/40001/media/mojo/common/media_type_converters.h File media/mojo/common/media_type_converters.h (right): https://codereview.chromium.org/2101043003/diff/40001/media/mojo/common/media_type_converters.h#newcode21 media/mojo/common/media_type_converters.h:21: class PipelineStatistics; struct
4 years, 5 months ago (2016-06-28 13:09:12 UTC) #15
slan
https://codereview.chromium.org/2101043003/diff/40001/media/mojo/common/media_type_converters.h File media/mojo/common/media_type_converters.h (right): https://codereview.chromium.org/2101043003/diff/40001/media/mojo/common/media_type_converters.h#newcode21 media/mojo/common/media_type_converters.h:21: class PipelineStatistics; On 2016/06/28 13:09:12, alokp wrote: > struct ...
4 years, 5 months ago (2016-06-28 19:13:55 UTC) #16
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101043003/80001
4 years, 5 months ago (2016-06-28 19:15:16 UTC) #18
sandersd (OOO until July 31)
media/ lgtm It's a bit of a shame for this to be a type converter ...
4 years, 5 months ago (2016-06-28 22:22:16 UTC) #19
slan
On 2016/06/28 22:22:16, sandersd wrote: > media/ lgtm > > It's a bit of a ...
4 years, 5 months ago (2016-06-28 22:29:31 UTC) #20
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/2101043003/80001
4 years, 5 months ago (2016-06-28 22:30:37 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/209015)
4 years, 5 months ago (2016-06-28 23:52:32 UTC) #26
slan
+dcheng@ for //ipc/SECURITY_OWNERS
4 years, 5 months ago (2016-06-29 14:58:53 UTC) #28
dcheng
https://codereview.chromium.org/2101043003/diff/80001/media/mojo/common/media_type_converters.cc File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/2101043003/diff/80001/media/mojo/common/media_type_converters.cc#newcode757 media/mojo/common/media_type_converters.cc:757: return result; Please use type maps for this instead: ...
4 years, 5 months ago (2016-07-01 06:07:46 UTC) #29
slan
On 2016/07/01 06:07:46, dcheng wrote: > https://codereview.chromium.org/2101043003/diff/80001/media/mojo/common/media_type_converters.cc > File media/mojo/common/media_type_converters.cc (right): > > https://codereview.chromium.org/2101043003/diff/80001/media/mojo/common/media_type_converters.cc#newcode757 > ...
4 years, 5 months ago (2016-07-06 18:29:32 UTC) #30
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101043003/90001
4 years, 5 months ago (2016-07-06 19:01:59 UTC) #32
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/48546)
4 years, 5 months ago (2016-07-06 20:04:25 UTC) #34
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2101043003/110001
4 years, 5 months ago (2016-07-06 20:45:58 UTC) #36
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 5 months ago (2016-07-07 00:12:24 UTC) #38
dcheng
https://codereview.chromium.org/2101043003/diff/110001/media/mojo/interfaces/media_types.mojom File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/2101043003/diff/110001/media/mojo/interfaces/media_types.mojom#newcode345 media/mojo/interfaces/media_types.mojom:345: int64 audio_memory_usage; Is there a reason for this to ...
4 years, 5 months ago (2016-07-07 04:19:38 UTC) #39
dcheng
4 years, 5 months ago (2016-07-07 04:19:44 UTC) #40
slan
https://codereview.chromium.org/2101043003/diff/110001/media/mojo/interfaces/media_types.mojom File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/2101043003/diff/110001/media/mojo/interfaces/media_types.mojom#newcode345 media/mojo/interfaces/media_types.mojom:345: int64 audio_memory_usage; On 2016/07/07 04:19:37, dcheng wrote: > Is ...
4 years, 5 months ago (2016-07-07 16:41:45 UTC) #41
dcheng
rs lgtm for mojo changes
4 years, 5 months ago (2016-07-08 05:53:16 UTC) #42
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/2101043003/110001
4 years, 5 months ago (2016-07-08 19:37:25 UTC) #45
commit-bot: I haz the power
Committed patchset #7 (id:110001)
4 years, 5 months ago (2016-07-08 21:11:01 UTC) #46
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-07-08 21:11:15 UTC) #47
commit-bot: I haz the power
4 years, 5 months ago (2016-07-08 21:13:02 UTC) #49
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/267e4634d0d1af89608e0c102a1683bcfb6bbb1f
Cr-Commit-Position: refs/heads/master@{#404486}

Powered by Google App Engine
This is Rietveld 408576698