media: Add static MojoCdmService::GetCdm().

media: Add static MojoCdmService::GetCdm().

This provides a way to statically lookup all CDMs created by MojoCdmService by
their CDM IDs.

BUG=545099
Committed: https://crrev.com/13eab7473462c47b59ff811e7076784c4a8efbee
Cr-Commit-Position: refs/heads/master@{#360402}

[xhwang, 2015-11-17 23:53:52]

xhwang@chromium.org changed reviewers:
+ dalecurtis@chromium.org, timav@chromium.org

[xhwang, 2015-11-17 23:53:52]

xhwang@chromium.org changed required reviewers:
+ dalecurtis@chromium.org

[xhwang, 2015-11-17 23:53:53]

dalecurtis / timav: PTAL!

timav: With this CL and https://chromiumcodereview.appspot.com/1450173002/, you
should be able to get the CDM in AVDA::SetCdm(). Then you can start the work to
connect CDM and AVDA together.

[DaleCurtis, 2015-11-17 23:57:05]

https://chromiumcodereview.appspot.com/1448303002/diff/20001/media/mojo/servi...
File media/mojo/services/mojo_cdm_service.cc (right):

https://chromiumcodereview.appspot.com/1448303002/diff/20001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.cc:30: base::LazyInstance<CdmMap>::Leaky
g_cdm_map = LAZY_INSTANCE_INITIALIZER;
Instead of two lazy instances you could put this in a class that returns the id
based on an internal lock?

[xhwang, 2015-11-18 00:52:53]

https://chromiumcodereview.appspot.com/1448303002/diff/20001/media/mojo/servi...
File media/mojo/services/mojo_cdm_service.cc (right):

https://chromiumcodereview.appspot.com/1448303002/diff/20001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.cc:30: base::LazyInstance<CdmMap>::Leaky
g_cdm_map = LAZY_INSTANCE_INITIALIZER;
On 2015/11/17 23:57:05, DaleCurtis wrote:
> Instead of two lazy instances you could put this in a class that returns the
id
> based on an internal lock?

It does make the main impl (MojoCdmService) cleaner. Done!

[DaleCurtis, 2015-11-18 01:00:21]

lgtm % optional changes

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
File media/mojo/services/mojo_cdm_service.cc (right):

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.cc:37: return cdm_map_.count(cdm_id) ?
cdm_map_[cdm_id] : nullptr;
General question: Why count instead of find? For a map count is always 1, but
other containers must by necessity traverse the whole container instead of
stopping at the first occurrence.

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.cc:56: std::map<int,
scoped_refptr<MediaKeys>> cdm_map_;
Map is fine, but if you only ever expect 1 or 2 of these a vector will be
better.

[Tima Vaisburd, 2015-11-18 01:24:20]

lgtm % nits

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
File media/mojo/services/mojo_cdm_service.cc (right):

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.cc:55: base::Lock cdm_map_lock_;
nit: I would rename |cdm_map_lock_| into |lock_| because I'd expect some
meaningful names for locks only if they protect portions of the data. But this
is totally up to you.

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
File media/mojo/services/mojo_cdm_service.h (right):

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.h:30: // thread only when it is safe to do
so.
"only when it is safe to do so" - unclear, do you mean that the thread safety of
this MojoCdmService does not imply the thread safety of MediaKeys?

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.h:32: // MojoMeidaApplication is running,
regardless of which render process or
s/Meida/Media/

[xhwang, 2015-11-18 18:31:58]

xhwang@chromium.org changed reviewers:
+ sievers@chromium.org

[xhwang, 2015-11-18 18:31:59]

sievers: Please OWNERS review changes in content/common/BUILD.gn.

[xhwang, 2015-11-18 18:45:17]

comments addressed

[xhwang, 2015-11-18 18:45:39]

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
File media/mojo/services/mojo_cdm_service.cc (right):

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.cc:37: return cdm_map_.count(cdm_id) ?
cdm_map_[cdm_id] : nullptr;
On 2015/11/18 01:00:21, DaleCurtis wrote:
> General question: Why count instead of find? For a map count is always 1, but
> other containers must by necessity traverse the whole container instead of
> stopping at the first occurrence.

Done.

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.cc:55: base::Lock cdm_map_lock_;
On 2015/11/18 01:24:20, Tima Vaisburd wrote:
> nit: I would rename |cdm_map_lock_| into |lock_| because I'd expect some
> meaningful names for locks only if they protect portions of the data. But this
> is totally up to you.

Done.

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.cc:56: std::map<int,
scoped_refptr<MediaKeys>> cdm_map_;
On 2015/11/18 01:00:21, DaleCurtis wrote:
> Map is fine, but if you only ever expect 1 or 2 of these a vector will be
> better.

I don't really see performance difference here and since we have the key (cdm
ID) and value (cdm) maps seems more natural.

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
File media/mojo/services/mojo_cdm_service.h (right):

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.h:30: // thread only when it is safe to do
so.
On 2015/11/18 01:24:20, Tima Vaisburd wrote:
> "only when it is safe to do so" - unclear, do you mean that the thread safety
of
> this MojoCdmService does not imply the thread safety of MediaKeys?

Done.

https://chromiumcodereview.appspot.com/1448303002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.h:32: // MojoMeidaApplication is running,
regardless of which render process or
On 2015/11/18 01:24:20, Tima Vaisburd wrote:
> s/Meida/Media/

Done.

[xhwang, 2015-11-18 18:47:22]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-18 18:49:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1448303002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1448303002/60001

[no sievers, 2015-11-18 19:22:51]

On 2015/11/18 18:31:59, xhwang wrote:
> sievers: Please OWNERS review changes in content/common/BUILD.gn.

lgtm

[xhwang, 2015-11-18 19:23:49]

The CQ bit was unchecked by xhwang@chromium.org

[xhwang, 2015-11-18 19:23:50]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-11-18 19:23:50]

The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1448303002/#ps60001
(title: "comments addressed")

[DaleCurtis, 2015-11-18 19:24:25]

Since I hadn't seen the benchmark recently, I wrote a small comparison of
vector/map for insert/find/erase/insert for low sizes vector is much faster, I
don't think this code path is particularly hot, but FYI for future reference:

https://paste.googleplex.com/6454185432186880

elapsed map find/erase/insert: 15058 us
elapsed vector find/erase/insert: 8863 us

[commit-bot: I haz the power, 2015-11-18 19:25:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1448303002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1448303002/60001

[xhwang, 2015-11-18 20:16:00]

On 2015/11/18 19:24:25, DaleCurtis wrote:
> Since I hadn't seen the benchmark recently, I wrote a small comparison of
> vector/map for insert/find/erase/insert for low sizes vector is much faster, I
> don't think this code path is particularly hot, but FYI for future reference:
> 
> https://paste.googleplex.com/6454185432186880
> 
> elapsed map find/erase/insert: 15058 us
> elapsed vector find/erase/insert: 8863 us

Indeed, this code path is actually very cold (once per playback session) so
simplicity wins :)

Really nice comparison though! But again this would only matter when the map is
small enough and the code using it is hot enough. Wondering whether you have
used it anywhere in the codebase? Also, shouldn't we have a wrapper for the
vector based map before any potential widespread adoption?

[commit-bot: I haz the power, 2015-11-18 20:29:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-18 20:30:42]

Patchset 4 (id:??) landed as
https://crrev.com/13eab7473462c47b59ff811e7076784c4a8efbee
Cr-Commit-Position: refs/heads/master@{#360402}