media: Plumb MediaLog to MojoVideoDecoderService.

media/mojo/clients/BUILD.gn

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 # Implementations of media C++ interfaces using corresponding mojo services.
 source_set("clients") {
   visibility = [
     "//chromecast/*",
     "//content/renderer:*",
 
     # TODO(xhwang): Only allow //media/mojo:media_mojo_unittests
     "//media/mojo:*",
 
-    # TODO(xhwang): Only allow //media/mojo/services:media_service_unittests
     "//media/mojo/services:*",
 
     "//media/test/*",
 
     "//content/gpu:*",
 
     # TODO(liberato): can we avoid this?
     "//content/test/*",
   ]
 
   sources = [
     "mojo_audio_decoder.cc",
     "mojo_audio_decoder.h",
     "mojo_cdm.cc",
     "mojo_cdm.h",
     "mojo_cdm_factory.cc",
     "mojo_cdm_factory.h",
     "mojo_decoder_factory.cc",
     "mojo_decoder_factory.h",
     "mojo_decryptor.cc",
     "mojo_decryptor.h",
     "mojo_demuxer_stream_impl.cc",
     "mojo_demuxer_stream_impl.h",
+    "mojo_media_log.cc",
+    "mojo_media_log.h",

[xhwang, 2017/06/30 16:24:57]

This is actually used on the "service" side...

[sandersd (OOO until July 31), 2017/06/30 17:44:30]

This is a client in the same sense as MojoVideoDecoder--it uses a Mojo service
to implement media::MediaLog for clients. The service side consumes a
media::MediaLog and provides a Mojo interface to it.

[xhwang, 2017/06/30 19:54:24]

I see your point and I admit it's a bit confusing. Here're some randoms
thoughts:

1. The name "clients" and "services" are too generic. We should probably rename
"services" to "playback_services". (See Dale's doc on "playback" at [1]). It
should not be a placeholder for all mojo interface implementations.

2. Historically, "services" means everything running in the MediaService in the
remote process.

3. And "clients" means the client side running in the render process. That's why
"clients" have the visibility on "//content/renderer:*" (lien 9)*.
"mojo_demuxer_stream_impl.h" is an example that it implements a mojo interface
to be used by objects in the MediaService. Similarly, we should probably put
"mojo_media_log_service.*" here, not "mojo_media_log.*".

4. We could adopt your model and put all "mojom implementations" in "services",
ignoring which group of classes actually work together. This is how our "media"
target works. But I feel the gist of GN is that you can have many smaller
targets which are logically self-contained, but I could be wrong or at least too
aggressive on that.

[1]
https://cs.chromium.org/chromium/src/media/README.md?rcl=12dd20d1972b5a0a2100...

*  Nothing prevents us to run such a client in another process (e.g. utility),
but since "clients" is a source_set, that could cause duplicated static symbols
which could lead to bugs in component builds.

[sandersd (OOO until July 31), 2017/07/06 21:25:24]

I'm having trouble turning this into a plan forward for this CL. How would you
like me to group these particular classes for now?

[sandersd (OOO until July 31), 2017/07/07 21:48:47]

Done.

     "mojo_renderer.cc",
     "mojo_renderer.h",
     "mojo_renderer_factory.cc",
     "mojo_renderer_factory.h",
     "mojo_video_decoder.cc",
     "mojo_video_decoder.h",
   ]
 
   if (is_android) {
     sources += [
@@ skipping 19 matching lines @@
     "//services/service_manager/public/cpp",
   ]
 
   if (is_android) {
     deps += [
       "//gpu/ipc/common:common",
       "//ui/gl:gl",
     ]
   }
 }