Finish plumbing MojoVideoDecoder.

Finish plumbing MojoVideoDecoder.

This completes the work to connect from a renderer to the VideoDecoder service in the GPU process, and implements a basic, functional VideoDecoder service.

There are no working VideoDecoders to actually instantiate yet, so the service will always return false from Configure(). Later CLs will add command buffer stub sharing and other requirements so that real decoders can be implemented.

BUG=522298
Committed: https://crrev.com/e78f9cb7d7bb4709436482572eb6fcb9a795e3b1
Cr-Commit-Position: refs/heads/master@{#395971}

[xhwang, 2016-04-20 21:09:27]

The main idea looks good! I have a few comments. I didn't review
MojoVideoDecoder* in detail.

https://chromiumcodereview.appspot.com/1899363002/diff/20001/content/gpu/gpu_...
File content/gpu/gpu_process_control_impl.h (right):

https://chromiumcodereview.appspot.com/1899363002/diff/20001/content/gpu/gpu_...
content/gpu/gpu_process_control_impl.h:10: #include
"base/single_thread_task_runner.h"
This can be forward declared

https://chromiumcodereview.appspot.com/1899363002/diff/20001/content/gpu/gpu_...
content/gpu/gpu_process_control_impl.h:19:
scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner);
gpu_main_task_runner just to be more explicit?

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/inter...
File media/mojo/interfaces/video_decoder.mojom (right):

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/inter...
media/mojo/interfaces/video_decoder.mojom:14: // TODO(sandersd): Do we need to
return a failure status?
Yes, we should at least have the equivalent of the init_cb?

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/inter...
media/mojo/interfaces/video_decoder.mojom:15: Configure(VideoDecoderConfig
config, bool low_delay) => ();
I am a bit confused here. The parameters in Initialize() and Configure() both
belongs to C++ VideoDecoder::Initialize() (indirectly).

Where are the gpu specific parameters you plan to pass in?

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
File media/mojo/services/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
media/mojo/services/BUILD.gn:227: }
ApplicationFactory does not use GpuMojoMediaClient..

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

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
media/mojo/services/android_mojo_media_client.cc:11: #include
"media/base/video_decoder.h"
why do you need these?

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

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
media/mojo/services/default_mojo_media_client.cc:7: #include
"media/base/cdm_factory.h"
Is any code complaining about this? I thought you don't need it if CdmFactory is
not used. Maybe I was wrong.

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
File media/mojo/services/gpu_mojo_media_client.h (right):

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
media/mojo/services/gpu_mojo_media_client.h:17: class GpuMojoMediaClient :
public MojoMediaClient {
Do you need some more gpu stuff in this client? Should this be located somewhere
in content/gpu?

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
media/mojo/services/gpu_mojo_media_client.h:20:
scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner);
explicit

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
media/mojo/services/gpu_mojo_media_client.h:31:
scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner_;
Add a comment why we need this.

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
File media/mojo/services/mojo_media_client.h (right):

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
media/mojo/services/mojo_media_client.h:10: #include "base/memory/ref_counted.h"
nice: thanks!

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
File media/mojo/services/service_factory_impl.h (right):

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
media/mojo/services/service_factory_impl.h:50:
mojo::InterfaceRequest<interfaces::VideoDecoder> request) final;
nit: this could be interfaces::VideoDecoderRequest now. This is pretty new so we
have a lot of code to be converted.

[sandersd (OOO until July 31), 2016-04-20 21:16:07]

(A few clarifications)

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/inter...
File media/mojo/interfaces/video_decoder.mojom (right):

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/inter...
media/mojo/interfaces/video_decoder.mojom:14: // TODO(sandersd): Do we need to
return a failure status?
On 2016/04/20 21:09:27, xhwang wrote:
> Yes, we should at least have the equivalent of the init_cb?

We do with this setup, there is a callback with no parameters, and on failure we
can close the pipe.

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/inter...
media/mojo/interfaces/video_decoder.mojom:15: Configure(VideoDecoderConfig
config, bool low_delay) => ();
On 2016/04/20 21:09:27, xhwang wrote:
> I am a bit confused here. The parameters in Initialize() and Configure() both
> belongs to C++ VideoDecoder::Initialize() (indirectly).
> 
> Where are the gpu specific parameters you plan to pass in?

Working on that right now, they will be passed to Initialize().

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

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
media/mojo/services/android_mojo_media_client.cc:11: #include
"media/base/video_decoder.h"
On 2016/04/20 21:09:27, xhwang wrote:
> why do you need these?

Oops, video_decoder.h isn't necessary.

The others are to be able to return a unique_ptr; otherwise it's an incomplete
type and apparently can't be returned.

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

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
media/mojo/services/default_mojo_media_client.cc:7: #include
"media/base/cdm_factory.h"
On 2016/04/20 21:09:27, xhwang wrote:
> Is any code complaining about this? I thought you don't need it if CdmFactory
is
> not used. Maybe I was wrong.

It does now that I removed the header includes from mojo_media_client.h.

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
File media/mojo/services/gpu_mojo_media_client.h (right):

https://chromiumcodereview.appspot.com/1899363002/diff/20001/media/mojo/servi...
media/mojo/services/gpu_mojo_media_client.h:17: class GpuMojoMediaClient :
public MojoMediaClient {
On 2016/04/20 21:09:27, xhwang wrote:
> Do you need some more gpu stuff in this client? Should this be located
somewhere
> in content/gpu?

We will need some more, yes. It probably should go in content/gpu, but I have no
experience with that tree. Any ideas?

[sandersd (OOO until July 31), 2016-05-02 22:09:51]

Description was changed from

==========
Plumb MojoVideoDecoder.

BUG=522298
==========

to

==========
Plumb MojoVideoDecoder.

BUG=522298
==========

[sandersd (OOO until July 31), 2016-05-03 00:54:35]

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

[sandersd (OOO until July 31), 2016-05-03 00:54:37]

I've slimmed this down enough that it may even be commit-able now, depending on
what you require of the actual decoder and service implementations.

Two simplifications were made:
  - Removed GpuMojoMediaClient. We may still want it, but I don't currently need
it.
  - Removed plumbing for the GPU task runner. The new plan is to make
MediaService a singleton and get it from there.

https://codereview.chromium.org/1899363002/diff/20001/content/gpu/gpu_process...
File content/gpu/gpu_process_control_impl.h (right):

https://codereview.chromium.org/1899363002/diff/20001/content/gpu/gpu_process...
content/gpu/gpu_process_control_impl.h:10: #include
"base/single_thread_task_runner.h"
On 2016/04/20 21:09:27, xhwang wrote:
> This can be forward declared

I have removed these changes from the CL.

https://codereview.chromium.org/1899363002/diff/20001/media/mojo/services/BUI...
File media/mojo/services/BUILD.gn (right):

https://codereview.chromium.org/1899363002/diff/20001/media/mojo/services/BUI...
media/mojo/services/BUILD.gn:227: }
On 2016/04/20 21:09:27, xhwang wrote:
> ApplicationFactory does not use GpuMojoMediaClient..

(Removed from CL)

https://codereview.chromium.org/1899363002/diff/20001/media/mojo/services/ser...
File media/mojo/services/service_factory_impl.h (right):

https://codereview.chromium.org/1899363002/diff/20001/media/mojo/services/ser...
media/mojo/services/service_factory_impl.h:50:
mojo::InterfaceRequest<interfaces::VideoDecoder> request) final;
On 2016/04/20 21:09:27, xhwang wrote:
> nit: this could be interfaces::VideoDecoderRequest now. This is pretty new so
we
> have a lot of code to be converted.

Done.

[xhwang, 2016-05-05 17:26:37]

looking good. Just a few more comments/nits.

https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/inter...
File media/mojo/interfaces/video_decoder.mojom (right):

https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/inter...
media/mojo/interfaces/video_decoder.mojom:9: interface VideoDecoder {
Please add comments for these new interfaces.

https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/inter...
media/mojo/interfaces/video_decoder.mojom:12: Configure(VideoDecoderConfig
config, bool low_delay) => (bool success);
What's the plan around this? For now (to commit it), shall we just merge these
two calls into one Initialize()?

In AudioDecoder, the Initialize() call takes both AudioDecoderClient and
AudioDecoderConfig: 
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/interfa...

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

https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_decoder_factory.cc:44: video_decoders->push_back(new
media::MojoVideoDecoder(
nit: here and above, no need for media::

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

https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_video_decoder_service.cc:61: base::Unretained(this),
callback),
weak_this_?

https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_video_decoder_service.cc:110: base::Unretained(this),
callback));
weak_this_?

[xhwang, 2016-05-05 17:27:08]

Also, please add more details in the CL description.

[sandersd (OOO until July 31), 2016-05-06 01:23:55]

https://codereview.chromium.org/1899363002/diff/40001/media/mojo/interfaces/v...
File media/mojo/interfaces/video_decoder.mojom (right):

https://codereview.chromium.org/1899363002/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_decoder.mojom:9: interface VideoDecoder {
On 2016/05/05 17:26:37, xhwang wrote:
> Please add comments for these new interfaces.

Done.

https://codereview.chromium.org/1899363002/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_decoder.mojom:12: Configure(VideoDecoderConfig
config, bool low_delay) => (bool success);
On 2016/05/05 17:26:37, xhwang wrote:
> What's the plan around this? For now (to commit it), shall we just merge these
> two calls into one Initialize()?
> 
> In AudioDecoder, the Initialize() call takes both AudioDecoderClient and
> AudioDecoderConfig: 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/interfa...

I don't think it makes sense to merge them, and I don't see anything wrong with
having two methods.

What advantage do you see?

https://codereview.chromium.org/1899363002/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_decoder_factory.cc (right):

https://codereview.chromium.org/1899363002/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_decoder_factory.cc:44: video_decoders->push_back(new
media::MojoVideoDecoder(
On 2016/05/05 17:26:37, xhwang wrote:
> nit: here and above, no need for media::

Done.

https://codereview.chromium.org/1899363002/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_video_decoder_service.cc:110: base::Unretained(this),
callback));
On 2016/05/05 17:26:37, xhwang wrote:
> weak_this_?

I totally forgot that I already set up a weak pointer factory in this class, so
I was putting that off until I had actual decoders to test.

Done!

[sandersd (OOO until July 31), 2016-05-06 01:28:28]

Description was changed from

==========
Plumb MojoVideoDecoder.

BUG=522298
==========

to

==========
Finish plumbing MojoVideoDecoder.

This completes the work to connect from a renderer to the VideoDecoder service
in the GPU process, and implements the first version of an actual VideoDecoder.

There are no VideoDecoders to actually instantiate yet, so the service will
always return false from Configure(). Later CLs will add command buffer stub
sharing and other requirements so that read decoders can be implemented.

BUG=522298
==========

[sandersd (OOO until July 31), 2016-05-06 01:29:06]

Description was changed from

==========
Finish plumbing MojoVideoDecoder.

This completes the work to connect from a renderer to the VideoDecoder service
in the GPU process, and implements the first version of an actual VideoDecoder.

There are no VideoDecoders to actually instantiate yet, so the service will
always return false from Configure(). Later CLs will add command buffer stub
sharing and other requirements so that read decoders can be implemented.

BUG=522298
==========

to

==========
Finish plumbing MojoVideoDecoder.

This completes the work to connect from a renderer to the VideoDecoder service
in the GPU process, and implements a basic, functional VideoDecoder service.

There are no working VideoDecoders to actually instantiate yet, so the service
will always return false from Configure(). Later CLs will add command buffer
stub sharing and other requirements so that read decoders can be implemented.

BUG=522298
==========

[xhwang, 2016-05-06 17:44:44]

LGTM with a naming discussion.

https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/inter...
File media/mojo/interfaces/video_decoder.mojom (right):

https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/inter...
media/mojo/interfaces/video_decoder.mojom:12: Configure(VideoDecoderConfig
config, bool low_delay) => (bool success);
On 2016/05/06 01:23:55, sandersd wrote:
> On 2016/05/05 17:26:37, xhwang wrote:
> > What's the plan around this? For now (to commit it), shall we just merge
these
> > two calls into one Initialize()?
> > 
> > In AudioDecoder, the Initialize() call takes both AudioDecoderClient and
> > AudioDecoderConfig: 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/interfa...
> 
> I don't think it makes sense to merge them, and I don't see anything wrong
with
> having two methods.
> 
> What advantage do you see?

I don't remember why we had them separate (there was a reason) but from the
current code it just doesn't seem necessary. Feel free to keep as is and we can
revisit later.

That being said, can we switch the names Initialize() and Configure() given
Configure() always calls media::VideoDecoder::Initialize()?

[sandersd (OOO until July 31), 2016-05-06 18:01:54]

https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/inter...
File media/mojo/interfaces/video_decoder.mojom (right):

https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/inter...
media/mojo/interfaces/video_decoder.mojom:12: Configure(VideoDecoderConfig
config, bool low_delay) => (bool success);
On 2016/05/06 17:44:44, xhwang wrote:
> On 2016/05/06 01:23:55, sandersd wrote:
> > On 2016/05/05 17:26:37, xhwang wrote:
> > > What's the plan around this? For now (to commit it), shall we just merge
> these
> > > two calls into one Initialize()?
> > > 
> > > In AudioDecoder, the Initialize() call takes both AudioDecoderClient and
> > > AudioDecoderConfig: 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/interfa...
> > 
> > I don't think it makes sense to merge them, and I don't see anything wrong
> with
> > having two methods.
> > 
> > What advantage do you see?
> 
> I don't remember why we had them separate (there was a reason) but from the
> current code it just doesn't seem necessary. Feel free to keep as is and we
can
> revisit later.
> 
> That being said, can we switch the names Initialize() and Configure() given
> Configure() always calls media::VideoDecoder::Initialize()?

I would rather start a discussion about renaming
media::VideoDecoder::Initialize() to media::VideoDecoder::Configure(). The
current name is rather confusing.

[xhwang, 2016-05-06 18:05:38]

On 2016/05/06 18:01:54, sandersd wrote:
>
https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/inter...
> File media/mojo/interfaces/video_decoder.mojom (right):
> 
>
https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/inter...
> media/mojo/interfaces/video_decoder.mojom:12: Configure(VideoDecoderConfig
> config, bool low_delay) => (bool success);
> On 2016/05/06 17:44:44, xhwang wrote:
> > On 2016/05/06 01:23:55, sandersd wrote:
> > > On 2016/05/05 17:26:37, xhwang wrote:
> > > > What's the plan around this? For now (to commit it), shall we just merge
> > these
> > > > two calls into one Initialize()?
> > > > 
> > > > In AudioDecoder, the Initialize() call takes both AudioDecoderClient and
> > > > AudioDecoderConfig: 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/interfa...
> > > 
> > > I don't think it makes sense to merge them, and I don't see anything wrong
> > with
> > > having two methods.
> > > 
> > > What advantage do you see?
> > 
> > I don't remember why we had them separate (there was a reason) but from the
> > current code it just doesn't seem necessary. Feel free to keep as is and we
> can
> > revisit later.
> > 
> > That being said, can we switch the names Initialize() and Configure() given
> > Configure() always calls media::VideoDecoder::Initialize()?
> 
> I would rather start a discussion about renaming
> media::VideoDecoder::Initialize() to media::VideoDecoder::Configure(). The
> current name is rather confusing.

That makes some sense. But it'll be a pretty large change since we use
Initialize() in so many places. Do you want to start that discussion first and
hold on this CL, or switch the names here, land it, and do a mass renaming later
pending the discussion result?

[sandersd (OOO until July 31), 2016-05-06 23:03:06]

On 2016/05/06 18:05:38, xhwang wrote:
> On 2016/05/06 18:01:54, sandersd wrote:
> >
>
https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/inter...
> > File media/mojo/interfaces/video_decoder.mojom (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1899363002/diff/40001/media/mojo/inter...
> > media/mojo/interfaces/video_decoder.mojom:12: Configure(VideoDecoderConfig
> > config, bool low_delay) => (bool success);
> > On 2016/05/06 17:44:44, xhwang wrote:
> > > On 2016/05/06 01:23:55, sandersd wrote:
> > > > On 2016/05/05 17:26:37, xhwang wrote:
> > > > > What's the plan around this? For now (to commit it), shall we just
merge
> > > these
> > > > > two calls into one Initialize()?
> > > > > 
> > > > > In AudioDecoder, the Initialize() call takes both AudioDecoderClient
and
> > > > > AudioDecoderConfig: 
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/interfa...
> > > > 
> > > > I don't think it makes sense to merge them, and I don't see anything
wrong
> > > with
> > > > having two methods.
> > > > 
> > > > What advantage do you see?
> > > 
> > > I don't remember why we had them separate (there was a reason) but from
the
> > > current code it just doesn't seem necessary. Feel free to keep as is and
we
> > can
> > > revisit later.
> > > 
> > > That being said, can we switch the names Initialize() and Configure()
given
> > > Configure() always calls media::VideoDecoder::Initialize()?
> > 
> > I would rather start a discussion about renaming
> > media::VideoDecoder::Initialize() to media::VideoDecoder::Configure(). The
> > current name is rather confusing.
> 
> That makes some sense. But it'll be a pretty large change since we use
> Initialize() in so many places. Do you want to start that discussion first and
> hold on this CL, or switch the names here, land it, and do a mass renaming
later
> pending the discussion result?

Done. PTAL.

[sandersd (OOO until July 31), 2016-05-06 23:14:24]

sandersd@chromium.org changed reviewers:
+ dcheng@chromium.org

[sandersd (OOO until July 31), 2016-05-06 23:14:25]

dcheng@: Please security review mojom and type converter changes.

[xhwang, 2016-05-06 23:19:35]

LGTM++

[sandersd (OOO until July 31), 2016-05-10 17:48:26]

On 2016/05/06 23:14:25, sandersd wrote:
> dcheng@: Please security review mojom and type converter changes.

dcheng@: Ping.

[sandersd (OOO until July 31), 2016-05-10 23:58:26]

Description was changed from

==========
Finish plumbing MojoVideoDecoder.

This completes the work to connect from a renderer to the VideoDecoder service
in the GPU process, and implements a basic, functional VideoDecoder service.

There are no working VideoDecoders to actually instantiate yet, so the service
will always return false from Configure(). Later CLs will add command buffer
stub sharing and other requirements so that read decoders can be implemented.

BUG=522298
==========

to

==========
Finish plumbing MojoVideoDecoder.

This completes the work to connect from a renderer to the VideoDecoder service
in the GPU process, and implements a basic, functional VideoDecoder service.

There are no working VideoDecoders to actually instantiate yet, so the service
will always return false from Configure(). Later CLs will add command buffer
stub sharing and other requirements so that real decoders can be implemented.

BUG=522298
==========

[dcheng, 2016-05-11 07:37:45]

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/interfaces/...
File media/mojo/interfaces/video_decoder.mojom (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/interfaces/...
media/mojo/interfaces/video_decoder.mojom:27: Initialize(VideoDecoderConfig
config, bool low_delay) => (bool success);
It's a bit weird to have a Construct() and Initialize() on the same interface.
Can we combine them?

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:59:
base::Bind(&MojoVideoDecoder::OnInitializeDone, base::Unretained(this)));
Why can't we just pass init_cb directly here? Similar question in other
callbacks, it seems weird to trampoline through a function that doesn't do very
much.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:84: uint32_t data_size =
base::checked_cast<uint32_t>(buffer->data_size());
This is... in the renderer process? It's a bit hard to tell, maybe there's some
sort of naming convention we can use? For example, we currently have a FooHost /
Foo convention for things like RenderFrameHost / RenderFrame. It would be nice
to keep that with Mojo.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:165: options.struct_size =
sizeof(MojoCreateDataPipeOptions);
Style guide recommends using sizeof(options).

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:190:
base::ResetAndReturn(&reset_cb_).Run();
I think it'd be better if there was a separate callback for connection error
rather than signalling every callback. I'm not sure adding another callback
abstraction to //media is good.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:88:
buffer.To<scoped_refptr<DecoderBuffer>>());
Which process is this running in? And where does the data come from? I looked at
the TypeConverter, and I think there are some problems with it: it doesn't
validate that side data actually has side data bytes. I don't know if there are
other issues.

We should also just migrate the type converters to StructTraits.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:108:
base::Bind(&MojoVideoDecoderService::OnDecoderDecoded,
Why do we need a trampoline callback here?

[xhwang, 2016-05-11 16:13:57]

comments only

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:59:
base::Bind(&MojoVideoDecoder::OnInitializeDone, base::Unretained(this)));
On 2016/05/11 07:37:45, dcheng wrote:
> Why can't we just pass init_cb directly here? Similar question in other
> callbacks, it seems weird to trampoline through a function that doesn't do
very
> much.

Last time I checked, you can't implicitly convert a base::Callback to
mojo::Callback and vice versa.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:84: uint32_t data_size =
base::checked_cast<uint32_t>(buffer->data_size());
On 2016/05/11 07:37:45, dcheng wrote:
> This is... in the renderer process? It's a bit hard to tell, maybe there's
some
> sort of naming convention we can use? For example, we currently have a FooHost
/
> Foo convention for things like RenderFrameHost / RenderFrame. It would be nice
> to keep that with Mojo.

I don't see examples of keeping such convention when using Mojo :) Also in the
IPC world, the Host is always in the browser process. But in the mojo world, the
service can be hosted in any process (e.g. GPU, Utility), so Host would actually
be confusing.

[dcheng, 2016-05-11 17:02:50]

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:59:
base::Bind(&MojoVideoDecoder::OnInitializeDone, base::Unretained(this)));
On 2016/05/11 at 16:13:57, xhwang wrote:
> On 2016/05/11 07:37:45, dcheng wrote:
> > Why can't we just pass init_cb directly here? Similar question in other
> > callbacks, it seems weird to trampoline through a function that doesn't do
very
> > much.
> 
> Last time I checked, you can't implicitly convert a base::Callback to
mojo::Callback and vice versa.

How is using base::Bind() to create a base::Callback different than just passing
the base::Callback directly here? Maybe I'm missing something.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:84: uint32_t data_size =
base::checked_cast<uint32_t>(buffer->data_size());
On 2016/05/11 at 16:13:57, xhwang wrote:
> On 2016/05/11 07:37:45, dcheng wrote:
> > This is... in the renderer process? It's a bit hard to tell, maybe there's
some
> > sort of naming convention we can use? For example, we currently have a
FooHost /
> > Foo convention for things like RenderFrameHost / RenderFrame. It would be
nice
> > to keep that with Mojo.
> 
> I don't see examples of keeping such convention when using Mojo :)

Well, we should figure one out. I'll start a thread on chromium-mojo. As it is,
it's pretty confusing.

> Also in the IPC world, the Host is always in the browser process. But in the
mojo world, the service can be hosted in any process (e.g. GPU, Utility), so
Host would actually be confusing.

That's already not true in IPC today (GPU, plugin) so I don't think this would
be more confusing, especially with more comments in the mojom to indicate where
things live (if possible)

[xhwang, 2016-05-11 17:14:58]

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:59:
base::Bind(&MojoVideoDecoder::OnInitializeDone, base::Unretained(this)));
On 2016/05/11 17:02:50, dcheng wrote:
> On 2016/05/11 at 16:13:57, xhwang wrote:
> > On 2016/05/11 07:37:45, dcheng wrote:
> > > Why can't we just pass init_cb directly here? Similar question in other
> > > callbacks, it seems weird to trampoline through a function that doesn't do
> very
> > > much.
> > 
> > Last time I checked, you can't implicitly convert a base::Callback to
> mojo::Callback and vice versa.
> 
> How is using base::Bind() to create a base::Callback different than just
passing
> the base::Callback directly here? Maybe I'm missing something.

oops, this is actually different than what I was thinking. I was talking about
the other side, where I vaguely remember that we can't simply pass a
mojo::Callback where base::Callback is expected. So I had to use base::Bind to
do the conversion:

https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...

This code is actually different. It seems we are already doing base::Callback to
mojo::Callback conversion. To be honest I don't know how that works :)

[sandersd (OOO until July 31), 2016-05-11 18:20:18]

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/interfaces/...
File media/mojo/interfaces/video_decoder.mojom (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/interfaces/...
media/mojo/interfaces/video_decoder.mojom:27: Initialize(VideoDecoderConfig
config, bool low_delay) => (bool success);
On 2016/05/11 07:37:45, dcheng wrote:
> It's a bit weird to have a Construct() and Initialize() on the same interface.
> Can we combine them?

They have significantly different semantics. As the comments describe, the
current names are poor, but match media::VideoDecoder.

Specifically, Constuct() should be called Initialize(), and does what you
expect.

Initialize() should be called Configure(), and may be called multiple times
throughout decoding.

If they were combined, the result would be all nullable parameters, with two
distinct calling styles.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:59:
base::Bind(&MojoVideoDecoder::OnInitializeDone, base::Unretained(this)));
> Why can't we just pass init_cb directly here? Similar question in other
> callbacks, it seems weird to trampoline through a function that doesn't do
> very much.

The truth is that it's an artifact of changes to the interface during
development, it's not doing anything critical anymore. However, I would rather
keep the debugging features it adds while I'm actively developing.

> This code is actually different. It seems we are already doing base::Callback
> to mojo::Callback conversion. To be honest I don't know how that works :)

There is a templated conversion constructor for anything with a Run() method
(https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...).

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:165: options.struct_size =
sizeof(MojoCreateDataPipeOptions);
On 2016/05/11 07:37:45, dcheng wrote:
> Style guide recommends using sizeof(options).

Done.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:190:
base::ResetAndReturn(&reset_cb_).Run();
On 2016/05/11 07:37:45, dcheng wrote:
> I think it'd be better if there was a separate callback for connection error
> rather than signalling every callback. I'm not sure adding another callback
> abstraction to //media is good.

We're constrained to implement the media::VideoDecoder interface here, and I see
no reason to add such a callback to it. This code isn't that terrible, and it
would somewhat defeat the point if the rest of media code needed to understand
what a remote decoder was.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:88:
buffer.To<scoped_refptr<DecoderBuffer>>());
On 2016/05/11 07:37:45, dcheng wrote:
> Which process is this running in? And where does the data come from? I looked
at
> the TypeConverter, and I think there are some problems with it: it doesn't
> validate that side data actually has side data bytes. I don't know if there
are
> other issues.
> 
> We should also just migrate the type converters to StructTraits.

That does indeed look bad. Where can I find documentation on StructTraits?

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:108:
base::Bind(&MojoVideoDecoderService::OnDecoderDecoded,
On 2016/05/11 07:37:45, dcheng wrote:
> Why do we need a trampoline callback here?

Because we can't cast |callback| from mojo::Callback to base::Callback.

[dcheng, 2016-05-11 18:26:24]

(I'll reply to the rest of the comments later)

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:88:
buffer.To<scoped_refptr<DecoderBuffer>>());
On 2016/05/11 at 18:20:18, sandersd wrote:
> On 2016/05/11 07:37:45, dcheng wrote:
> > Which process is this running in? And where does the data come from? I
looked at
> > the TypeConverter, and I think there are some problems with it: it doesn't
> > validate that side data actually has side data bytes. I don't know if there
are
> > other issues.
> > 
> > We should also just migrate the type converters to StructTraits.
> 
> That does indeed look bad. Where can I find documentation on StructTraits?

https://www.chromium.org/developers/design-documents/mojo/type-mapping

[dcheng, 2016-05-11 22:15:06]

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/interfaces/...
File media/mojo/interfaces/video_decoder.mojom (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/interfaces/...
media/mojo/interfaces/video_decoder.mojom:27: Initialize(VideoDecoderConfig
config, bool low_delay) => (bool success);
On 2016/05/11 at 18:20:18, sandersd wrote:
> On 2016/05/11 07:37:45, dcheng wrote:
> > It's a bit weird to have a Construct() and Initialize() on the same
interface.
> > Can we combine them?
> 
> They have significantly different semantics. As the comments describe, the
current names are poor, but match media::VideoDecoder.
> 
> Specifically, Constuct() should be called Initialize(), and does what you
expect.
> 
> Initialize() should be called Configure(), and may be called multiple times
throughout decoding.
> 
> If they were combined, the result would be all nullable parameters, with two
distinct calling styles.

Can we at least fix the names in the mojom interface? it might be confusing in
the interim period, but I think we should eventually strive to fix these names.
It's pretty confusing as-is.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:59:
base::Bind(&MojoVideoDecoder::OnInitializeDone, base::Unretained(this)));
On 2016/05/11 at 18:20:18, sandersd wrote:
> > Why can't we just pass init_cb directly here? Similar question in other
> > callbacks, it seems weird to trampoline through a function that doesn't do
> > very much.
> 
> The truth is that it's an artifact of changes to the interface during
development, it's not doing anything critical anymore. However, I would rather
keep the debugging features it adds while I'm actively developing.
> 

When will we actually remove them? Code like this has a tendency to stick
around, and indirecting so many times through base::Bind just obfuscates the
control flow.

> > This code is actually different. It seems we are already doing
base::Callback
> > to mojo::Callback conversion. To be honest I don't know how that works :)
> 
> There is a templated conversion constructor for anything with a Run() method
(https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...).

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:108:
base::Bind(&MojoVideoDecoderService::OnDecoderDecoded,
On 2016/05/11 at 18:20:18, sandersd wrote:
> On 2016/05/11 07:37:45, dcheng wrote:
> > Why do we need a trampoline callback here?
> 
> Because we can't cast |callback| from mojo::Callback to base::Callback.

Blah. Going to ask the Mojo people about this today.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.h (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.h:32:
mojo::ScopedDataPipeConsumerHandle decoder_buffer_pipe) final;
If no one is supposed to derive from this class, maybe just do:

class MojoVideoDecoderService final : public interfaces::VideoDecoder {

[sandersd (OOO until July 31), 2016-05-11 22:49:42]

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:88:
buffer.To<scoped_refptr<DecoderBuffer>>());
On 2016/05/11 18:26:24, dcheng wrote:
> On 2016/05/11 at 18:20:18, sandersd wrote:
> > On 2016/05/11 07:37:45, dcheng wrote:
> > > Which process is this running in? And where does the data come from? I
> looked at
> > > the TypeConverter, and I think there are some problems with it: it doesn't
> > > validate that side data actually has side data bytes. I don't know if
there
> are
> > > other issues.
> > > 
> > > We should also just migrate the type converters to StructTraits.
> > 
> > That does indeed look bad. Where can I find documentation on StructTraits?
> 
> https://www.chromium.org/developers/design-documents/mojo/type-mapping

This looks large enough that it should be a separate CL, so I filed
http://crbug.com/611224. For now I've just made the obvious fix to
DecoderBuffer.

[sandersd (OOO until July 31), 2016-05-11 23:00:50]

Sorry I missed your comments in the earlier PS!

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/interfaces/...
File media/mojo/interfaces/video_decoder.mojom (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/interfaces/...
media/mojo/interfaces/video_decoder.mojom:27: Initialize(VideoDecoderConfig
config, bool low_delay) => (bool success);
On 2016/05/11 22:15:06, dcheng wrote:
> On 2016/05/11 at 18:20:18, sandersd wrote:
> > On 2016/05/11 07:37:45, dcheng wrote:
> > > It's a bit weird to have a Construct() and Initialize() on the same
> interface.
> > > Can we combine them?
> > 
> > They have significantly different semantics. As the comments describe, the
> current names are poor, but match media::VideoDecoder.
> > 
> > Specifically, Constuct() should be called Initialize(), and does what you
> expect.
> > 
> > Initialize() should be called Configure(), and may be called multiple times
> throughout decoding.
> > 
> > If they were combined, the result would be all nullable parameters, with two
> distinct calling styles.
> 
> Can we at least fix the names in the mojom interface? it might be confusing in
> the interim period, but I think we should eventually strive to fix these
names.
> It's pretty confusing as-is.

I'll have to defer this discussion to xhwang@, as I had the same request. How
about it Xiaohan?

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:59:
base::Bind(&MojoVideoDecoder::OnInitializeDone, base::Unretained(this)));
On 2016/05/11 22:15:06, dcheng wrote:
> On 2016/05/11 at 18:20:18, sandersd wrote:
> > > Why can't we just pass init_cb directly here? Similar question in other
> > > callbacks, it seems weird to trampoline through a function that doesn't do
> > > very much.
> > 
> > The truth is that it's an artifact of changes to the interface during
> development, it's not doing anything critical anymore. However, I would rather
> keep the debugging features it adds while I'm actively developing.
> > 
> 
> When will we actually remove them? Code like this has a tendency to stick
> around, and indirecting so many times through base::Bind just obfuscates the
> control flow.
> 
> > > This code is actually different. It seems we are already doing
> base::Callback
> > > to mojo::Callback conversion. To be honest I don't know how that works :)
> > 
> > There is a templated conversion constructor for anything with a Run() method
>
(https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...).

TODO added.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.h (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.h:32:
mojo::ScopedDataPipeConsumerHandle decoder_buffer_pipe) final;
On 2016/05/11 22:15:06, dcheng wrote:
> If no one is supposed to derive from this class, maybe just do:
> 
> class MojoVideoDecoderService final : public interfaces::VideoDecoder {

Done.

(I had to leave in all the annotations though, because they still need an
override marking to make the compiler happy.)

[sandersd (OOO until July 31), 2016-05-12 18:10:36]

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:84: uint32_t data_size =
base::checked_cast<uint32_t>(buffer->data_size());
On 2016/05/11 17:02:50, dcheng wrote:
> On 2016/05/11 at 16:13:57, xhwang wrote:
> > On 2016/05/11 07:37:45, dcheng wrote:
> > > This is... in the renderer process? It's a bit hard to tell, maybe there's
> some
> > > sort of naming convention we can use? For example, we currently have a
> FooHost /
> > > Foo convention for things like RenderFrameHost / RenderFrame. It would be
> nice
> > > to keep that with Mojo.
> > 
> > I don't see examples of keeping such convention when using Mojo :)
> 
> Well, we should figure one out. I'll start a thread on chromium-mojo. As it
is,
> it's pretty confusing.
> 
> > Also in the IPC world, the Host is always in the browser process. But in the
> mojo world, the service can be hosted in any process (e.g. GPU, Utility), so
> Host would actually be confusing.
> 
> That's already not true in IPC today (GPU, plugin) so I don't think this would
> be more confusing, especially with more comments in the mojom to indicate
where
> things live (if possible)

Since there isn't guidance from that thread yet, perhaps there is a lightweight
method that could work? Comment at the top of the file/header perhaps?

For this CL, mojo_video_decoder runs in the renderer (unprivileged), and
mojo_video_decoder_service runs in the GPU process (privileged). Currently all
media Mojo code is using this same division, Foo -> renderer, FooService -> not
renderer.

[dcheng, 2016-05-17 06:32:29]

On 2016/05/12 at 18:10:36, sandersd wrote:
>
https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
> File media/mojo/services/mojo_video_decoder.cc (right):
> 
>
https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
> media/mojo/services/mojo_video_decoder.cc:84: uint32_t data_size =
base::checked_cast<uint32_t>(buffer->data_size());
> On 2016/05/11 17:02:50, dcheng wrote:
> > On 2016/05/11 at 16:13:57, xhwang wrote:
> > > On 2016/05/11 07:37:45, dcheng wrote:
> > > > This is... in the renderer process? It's a bit hard to tell, maybe
there's
> > some
> > > > sort of naming convention we can use? For example, we currently have a
> > FooHost /
> > > > Foo convention for things like RenderFrameHost / RenderFrame. It would
be
> > nice
> > > > to keep that with Mojo.
> > > 
> > > I don't see examples of keeping such convention when using Mojo :)
> > 
> > Well, we should figure one out. I'll start a thread on chromium-mojo. As it
is,
> > it's pretty confusing.
> > 
> > > Also in the IPC world, the Host is always in the browser process. But in
the
> > mojo world, the service can be hosted in any process (e.g. GPU, Utility), so
> > Host would actually be confusing.
> > 
> > That's already not true in IPC today (GPU, plugin) so I don't think this
would
> > be more confusing, especially with more comments in the mojom to indicate
where
> > things live (if possible)
> 
> Since there isn't guidance from that thread yet, perhaps there is a
lightweight method that could work? Comment at the top of the file/header
perhaps?
> 
> For this CL, mojo_video_decoder runs in the renderer (unprivileged), and
mojo_video_decoder_service runs in the GPU process (privileged). Currently all
media Mojo code is using this same division, Foo -> renderer, FooService -> not
renderer.

Sorry for the delayed reply. One thing I've always had trouble with in media is
the directory organization. A high-level simplification of content is:
- content/browser: code that runs in the privileged browser process
- content/renderer: code that runs in the unprivileged renderer process
- content/common: code that is shared between browser and renderer

I think some comments would help: I had a hard time reasoning about this CL,
because we have the mojom VideoDecoder which corresponds to
MojoVideoDecoderService... but then we have the mojom VideoDecoderClient which
corresponds to MojoVideoDecoder.

https://codereview.chromium.org/1899363002/diff/160001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/160001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:97:
base::checked_cast<uint32_t>(media_buffer->data_size());
This shouldn't be a checked_cast: it's in the GPU process processing data from
the renderer. The renderer shouldn't be able to crash the GPU just by sending a
bad message.

https://codereview.chromium.org/1899363002/diff/160001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:100: CHECK_EQ(
Ditto for this and the other CHECKs in this file when processing mojo messages.

[sandersd (OOO until July 31), 2016-05-17 23:05:00]

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/interfaces/...
File media/mojo/interfaces/video_decoder.mojom (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/interfaces/...
media/mojo/interfaces/video_decoder.mojom:27: Initialize(VideoDecoderConfig
config, bool low_delay) => (bool success);
On 2016/05/11 23:00:50, sandersd wrote:
> On 2016/05/11 22:15:06, dcheng wrote:
> > On 2016/05/11 at 18:20:18, sandersd wrote:
> > > On 2016/05/11 07:37:45, dcheng wrote:
> > > > It's a bit weird to have a Construct() and Initialize() on the same
> > interface.
> > > > Can we combine them?
> > > 
> > > They have significantly different semantics. As the comments describe, the
> > current names are poor, but match media::VideoDecoder.
> > > 
> > > Specifically, Constuct() should be called Initialize(), and does what you
> > expect.
> > > 
> > > Initialize() should be called Configure(), and may be called multiple
times
> > throughout decoding.
> > > 
> > > If they were combined, the result would be all nullable parameters, with
two
> > distinct calling styles.
> > 
> > Can we at least fix the names in the mojom interface? it might be confusing
in
> > the interim period, but I think we should eventually strive to fix these
> names.
> > It's pretty confusing as-is.
> 
> I'll have to defer this discussion to xhwang@, as I had the same request. How
> about it Xiaohan?

xhwang@: ping.

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder.cc (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder.cc:84: uint32_t data_size =
base::checked_cast<uint32_t>(buffer->data_size());
On 2016/05/12 18:10:36, sandersd wrote:
> On 2016/05/11 17:02:50, dcheng wrote:
> > On 2016/05/11 at 16:13:57, xhwang wrote:
> > > On 2016/05/11 07:37:45, dcheng wrote:
> > > > This is... in the renderer process? It's a bit hard to tell, maybe
there's
> > some
> > > > sort of naming convention we can use? For example, we currently have a
> > FooHost /
> > > > Foo convention for things like RenderFrameHost / RenderFrame. It would
be
> > nice
> > > > to keep that with Mojo.
> > > 
> > > I don't see examples of keeping such convention when using Mojo :)
> > 
> > Well, we should figure one out. I'll start a thread on chromium-mojo. As it
> is,
> > it's pretty confusing.
> > 
> > > Also in the IPC world, the Host is always in the browser process. But in
the
> > mojo world, the service can be hosted in any process (e.g. GPU, Utility), so
> > Host would actually be confusing.
> > 
> > That's already not true in IPC today (GPU, plugin) so I don't think this
would
> > be more confusing, especially with more comments in the mojom to indicate
> where
> > things live (if possible)
> 
> Since there isn't guidance from that thread yet, perhaps there is a
lightweight
> method that could work? Comment at the top of the file/header perhaps?
> 
> For this CL, mojo_video_decoder runs in the renderer (unprivileged), and
> mojo_video_decoder_service runs in the GPU process (privileged). Currently all
> media Mojo code is using this same division, Foo -> renderer, FooService ->
not
> renderer.

I've added basic comments to the class headers. Let me know if there is more
information you want added.

https://codereview.chromium.org/1899363002/diff/160001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/160001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:97:
base::checked_cast<uint32_t>(media_buffer->data_size());
On 2016/05/17 06:32:29, dcheng wrote:
> This shouldn't be a checked_cast: it's in the GPU process processing data from
> the renderer. The renderer shouldn't be able to crash the GPU just by sending
a
> bad message.

Done.

https://codereview.chromium.org/1899363002/diff/160001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:100: CHECK_EQ(
On 2016/05/17 06:32:29, dcheng wrote:
> Ditto for this and the other CHECKs in this file when processing mojo
messages.

Done.

I also updated MojoAudioDecoderService, although in the future they should share
an implementation of this.

[xhwang, 2016-05-17 23:15:38]

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/interfaces/...
File media/mojo/interfaces/video_decoder.mojom (right):

https://codereview.chromium.org/1899363002/diff/100001/media/mojo/interfaces/...
media/mojo/interfaces/video_decoder.mojom:27: Initialize(VideoDecoderConfig
config, bool low_delay) => (bool success);
On 2016/05/17 23:05:00, sandersd wrote:
> On 2016/05/11 23:00:50, sandersd wrote:
> > On 2016/05/11 22:15:06, dcheng wrote:
> > > On 2016/05/11 at 18:20:18, sandersd wrote:
> > > > On 2016/05/11 07:37:45, dcheng wrote:
> > > > > It's a bit weird to have a Construct() and Initialize() on the same
> > > interface.
> > > > > Can we combine them?
> > > > 
> > > > They have significantly different semantics. As the comments describe,
the
> > > current names are poor, but match media::VideoDecoder.
> > > > 
> > > > Specifically, Constuct() should be called Initialize(), and does what
you
> > > expect.
> > > > 
> > > > Initialize() should be called Configure(), and may be called multiple
> times
> > > throughout decoding.
> > > > 
> > > > If they were combined, the result would be all nullable parameters, with
> two
> > > distinct calling styles.
> > > 
> > > Can we at least fix the names in the mojom interface? it might be
confusing
> in
> > > the interim period, but I think we should eventually strive to fix these
> > names.
> > > It's pretty confusing as-is.
> > 
> > I'll have to defer this discussion to xhwang@, as I had the same request.
How
> > about it Xiaohan?
> 
> xhwang@: ping.

dcheng: 

We already have C++ VideoDecoder::Initialize() today:
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_d...

I agree the naming is not ideal. I was trying to avoid the confusion where mojo
VideoDecoder::Initialize() does something, then mojo VideoDecoder::Config()
calls C++ VideoDecoder::Initialize().

Does that make some sense?

[Tima Vaisburd, 2016-05-18 00:32:15]

timav@chromium.org changed reviewers:
+ timav@chromium.org

[Tima Vaisburd, 2016-05-18 00:32:16]

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
media/mojo/services/mojo_audio_decoder_service.cc:156: if (data_size !=
media_buffer->data_size())
Drive-by: how can they be different?

[sandersd (OOO until July 31), 2016-05-18 00:49:29]

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
media/mojo/services/mojo_audio_decoder_service.cc:156: if (data_size !=
media_buffer->data_size())
On 2016/05/18 00:32:16, Tima Vaisburd wrote:
> Drive-by: how can they be different?

They can't (assuming size_t is of higher integer rank than uint32_t), but I
didn't want to have to trust that path when reasoning about this code.

[sandersd (OOO until July 31), 2016-05-18 00:50:42]

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
media/mojo/services/mojo_audio_decoder_service.cc:156: if (data_size !=
media_buffer->data_size())
On 2016/05/18 00:49:29, sandersd wrote:
> On 2016/05/18 00:32:16, Tima Vaisburd wrote:
> > Drive-by: how can they be different?
> 
> They can't (assuming size_t is of higher integer rank than uint32_t), but I
> didn't want to have to trust that path when reasoning about this code.

*same or higher

[Tima Vaisburd, 2016-05-18 01:08:13]

On 2016/05/18 00:50:42, sandersd wrote:
>
https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
> File media/mojo/services/mojo_audio_decoder_service.cc (right):
> 
>
https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
> media/mojo/services/mojo_audio_decoder_service.cc:156: if (data_size !=
> media_buffer->data_size())
> On 2016/05/18 00:49:29, sandersd wrote:
> > On 2016/05/18 00:32:16, Tima Vaisburd wrote:
> > > Drive-by: how can they be different?
> > 
> > They can't (assuming size_t is of higher integer rank than uint32_t), but I
> > didn't want to have to trust that path when reasoning about this code.
> 
> *same or higher

I would think so. I did DCHECK_EQ() in this situation.

[sandersd (OOO until July 31), 2016-05-18 18:16:46]

On 2016/05/18 at 01:08:13, timav wrote:
> On 2016/05/18 00:50:42, sandersd wrote:
> >
https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
> > File media/mojo/services/mojo_audio_decoder_service.cc (right):
> > 
> >
https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
> > media/mojo/services/mojo_audio_decoder_service.cc:156: if (data_size !=
> > media_buffer->data_size())
> > On 2016/05/18 00:49:29, sandersd wrote:
> > > On 2016/05/18 00:32:16, Tima Vaisburd wrote:
> > > > Drive-by: how can they be different?
> > > 
> > > They can't (assuming size_t is of higher integer rank than uint32_t), but
I
> > > didn't want to have to trust that path when reasoning about this code.
> > 
> > *same or higher
> 
> I would think so. I did DCHECK_EQ() in this situation.

Fundamentally though, using a DCHECK() is the same as trusting the larger code
path to have no bugs. If the code is correct and inlined here, there is some
chance that the compiler could remove the check itself. If that doesn't happen,
then I would consider it too complicated to trust.

[sandersd (OOO until July 31), 2016-05-19 23:21:36]

On 2016/05/18 18:16:46, sandersd wrote:
> On 2016/05/18 at 01:08:13, timav wrote:
> > On 2016/05/18 00:50:42, sandersd wrote:
> > >
>
https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
> > > File media/mojo/services/mojo_audio_decoder_service.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
> > > media/mojo/services/mojo_audio_decoder_service.cc:156: if (data_size !=
> > > media_buffer->data_size())
> > > On 2016/05/18 00:49:29, sandersd wrote:
> > > > On 2016/05/18 00:32:16, Tima Vaisburd wrote:
> > > > > Drive-by: how can they be different?
> > > > 
> > > > They can't (assuming size_t is of higher integer rank than uint32_t),
but
> I
> > > > didn't want to have to trust that path when reasoning about this code.
> > > 
> > > *same or higher
> > 
> > I would think so. I did DCHECK_EQ() in this situation.
> 
> Fundamentally though, using a DCHECK() is the same as trusting the larger code
> path to have no bugs. If the code is correct and inlined here, there is some
> chance that the compiler could remove the check itself. If that doesn't
happen,
> then I would consider it too complicated to trust.

Everyone: Ping?

Can we make forward progress on getting this committed?

[dcheng, 2016-05-20 21:18:22]

Looking good, just some minor comments. As discussed, we'll reorganize the
directory structure in a followup to clarify the separation between clients and
services.

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/common/medi...
File media/mojo/common/media_type_converters.cc (left):

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/common/medi...
media/mojo/common/media_type_converters.cc:431:
base::checked_cast<uint32_t>(input->side_data_size());
Can we wholly remove this field from the mojom in this patch?

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/common/medi...
File media/mojo/common/media_type_converters.cc (right):

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/common/medi...
media/mojo/common/media_type_converters.cc:477:
buffer->CopySideDataFrom(&input->side_data.front(), input->side_data.size());
Existing semantics don't copy this data if it's zero-length, having a
zero-length side data won't be a problem, right?

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
media/mojo/services/mojo_audio_decoder_service.cc:156: if (data_size !=
media_buffer->data_size())
On 2016/05/18 at 00:50:42, sandersd wrote:
> On 2016/05/18 00:49:29, sandersd wrote:
> > On 2016/05/18 00:32:16, Tima Vaisburd wrote:
> > > Drive-by: how can they be different?
> > 
> > They can't (assuming size_t is of higher integer rank than uint32_t), but I
> > didn't want to have to trust that path when reasoning about this code.
> 
> *same or higher

I'd suggest DCHECK() or static_assert in the TypeConverter, this makes it look
like a legitimate runtime condition that needs to be handled.

[sandersd (OOO until July 31), 2016-05-20 22:27:14]

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/common/medi...
File media/mojo/common/media_type_converters.cc (left):

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/common/medi...
media/mojo/common/media_type_converters.cc:431:
base::checked_cast<uint32_t>(input->side_data_size());
On 2016/05/20 21:18:22, dcheng wrote:
> Can we wholly remove this field from the mojom in this patch?

I already did that; was this comment intended for a different field?

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/common/medi...
File media/mojo/common/media_type_converters.cc (right):

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/common/medi...
media/mojo/common/media_type_converters.cc:477:
buffer->CopySideDataFrom(&input->side_data.front(), input->side_data.size());
On 2016/05/20 21:18:22, dcheng wrote:
> Existing semantics don't copy this data if it's zero-length, having a
> zero-length side data won't be a problem, right?

That's correct, the media::DecoderBuffer structure doesn't distinguish between
no side data and zero-length side data.

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
media/mojo/services/mojo_audio_decoder_service.cc:156: if (data_size !=
media_buffer->data_size())
On 2016/05/20 21:18:22, dcheng wrote:
> On 2016/05/18 at 00:50:42, sandersd wrote:
> > On 2016/05/18 00:49:29, sandersd wrote:
> > > On 2016/05/18 00:32:16, Tima Vaisburd wrote:
> > > > Drive-by: how can they be different?
> > > 
> > > They can't (assuming size_t is of higher integer rank than uint32_t), but
I
> > > didn't want to have to trust that path when reasoning about this code.
> > 
> > *same or higher
> 
> I'd suggest DCHECK() or static_assert in the TypeConverter, this makes it look
> like a legitimate runtime condition that needs to be handled.

Converted to a CHECK() (in MojoVideoDecoderService; MojoAudioDecoderService was
updated in https://codereview.chromium.org/1982883003/)

[dcheng, 2016-05-20 22:52:59]

lgtm with a nit

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/common/medi...
File media/mojo/common/media_type_converters.cc (left):

https://codereview.chromium.org/1899363002/diff/200001/media/mojo/common/medi...
media/mojo/common/media_type_converters.cc:431:
base::checked_cast<uint32_t>(input->side_data_size());
On 2016/05/20 at 22:27:14, sandersd wrote:
> On 2016/05/20 21:18:22, dcheng wrote:
> > Can we wholly remove this field from the mojom in this patch?
> 
> I already did that; was this comment intended for a different field?

Oops, I missed that.

https://codereview.chromium.org/1899363002/diff/220001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:110: CHECK_EQ(data_size,
media_buffer->data_size());
Nit: DCHECK_EQ? This should "never happen" since it's sent across the wire as a
uint32.

[sandersd (OOO until July 31), 2016-05-20 23:42:13]

https://codereview.chromium.org/1899363002/diff/220001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:110: CHECK_EQ(data_size,
media_buffer->data_size());
On 2016/05/20 22:52:59, dcheng wrote:
> Nit: DCHECK_EQ? This should "never happen" since it's sent across the wire as
a
> uint32.

That depends on what we're checking for; the code being outright wrong or
specific cases that may be broken and untested. I'll let you make that call, but
I prefer a real check before overwriting a buffer.

[dcheng, 2016-05-20 23:48:14]

https://codereview.chromium.org/1899363002/diff/220001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:110: CHECK_EQ(data_size,
media_buffer->data_size());
On 2016/05/20 at 23:42:12, sandersd wrote:
> On 2016/05/20 22:52:59, dcheng wrote:
> > Nit: DCHECK_EQ? This should "never happen" since it's sent across the wire
as a
> > uint32.
> 
> That depends on what we're checking for; the code being outright wrong or
specific cases that may be broken and untested. I'll let you make that call, but
I prefer a real check before overwriting a buffer.

That's why I suggested a static_assert in the TypeConverter.

static_assert(sizeof(buffer->data_size) <= sizeof(media_buffer->data_size)) is
essentially what we want to assert, right?

[sandersd (OOO until July 31), 2016-05-20 23:56:14]

https://codereview.chromium.org/1899363002/diff/220001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/1899363002/diff/220001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:110: CHECK_EQ(data_size,
media_buffer->data_size());
On 2016/05/20 23:48:14, dcheng wrote:
> On 2016/05/20 at 23:42:12, sandersd wrote:
> > On 2016/05/20 22:52:59, dcheng wrote:
> > > Nit: DCHECK_EQ? This should "never happen" since it's sent across the wire
> as a
> > > uint32.
> > 
> > That depends on what we're checking for; the code being outright wrong or
> specific cases that may be broken and untested. I'll let you make that call,
but
> I prefer a real check before overwriting a buffer.
> 
> That's why I suggested a static_assert in the TypeConverter.
> 
> static_assert(sizeof(buffer->data_size) <= sizeof(media_buffer->data_size)) is
> essentially what we want to assert, right?

In that case, you are more trusting of cross-file code paths than I.

Done.

[sandersd (OOO until July 31), 2016-05-20 23:57:32]

sandersd@chromium.org changed reviewers:
+ jochen@chromium.org

[sandersd (OOO until July 31), 2016-05-20 23:57:33]

jochen@chromium.org: Please review changes in content/renderer/

[sandersd (OOO until July 31), 2016-05-23 21:48:14]

sandersd@chromium.org changed reviewers:
+ sievers@chromium.org
- jochen@chromium.org

[sandersd (OOO until July 31), 2016-05-23 21:48:39]

=> sievers@ for content/ review, as jochen is OOO.

[sandersd (OOO until July 31), 2016-05-24 17:46:22]

sievers@: ping.

[sandersd (OOO until July 31), 2016-05-25 00:11:39]

sandersd@chromium.org changed reviewers:
+ piman@chromium.org
- sievers@chromium.org

[sandersd (OOO until July 31), 2016-05-25 00:11:41]

Looks like sievers is gone now too.

piman@: Please review changes in content/

[piman, 2016-05-25 00:18:58]

LGTM for content/

[sandersd (OOO until July 31), 2016-05-25 00:22:08]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2016-05-25 00:22:08]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1899363002/#ps240001
(title: "CHECK -> DCHECK")

[commit-bot: I haz the power, 2016-05-25 00:23:01]

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

[commit-bot: I haz the power, 2016-05-25 01:52:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-25 01:52:37]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sandersd (OOO until July 31), 2016-05-25 18:13:54]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2016-05-25 18:13:54]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
dcheng@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1899363002/#ps260001
(title: "Windows needs some more headers, apaprently.")

[commit-bot: I haz the power, 2016-05-25 18:14:29]

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

[commit-bot: I haz the power, 2016-05-25 20:06:47]

Description was changed from

==========
Finish plumbing MojoVideoDecoder.

This completes the work to connect from a renderer to the VideoDecoder service
in the GPU process, and implements a basic, functional VideoDecoder service.

There are no working VideoDecoders to actually instantiate yet, so the service
will always return false from Configure(). Later CLs will add command buffer
stub sharing and other requirements so that real decoders can be implemented.

BUG=522298
==========

to

==========
Finish plumbing MojoVideoDecoder.

This completes the work to connect from a renderer to the VideoDecoder service
in the GPU process, and implements a basic, functional VideoDecoder service.

There are no working VideoDecoders to actually instantiate yet, so the service
will always return false from Configure(). Later CLs will add command buffer
stub sharing and other requirements so that real decoders can be implemented.

BUG=522298
==========

[commit-bot: I haz the power, 2016-05-25 20:06:49]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-05-25 20:08:40]

Description was changed from

==========
Finish plumbing MojoVideoDecoder.

This completes the work to connect from a renderer to the VideoDecoder service
in the GPU process, and implements a basic, functional VideoDecoder service.

There are no working VideoDecoders to actually instantiate yet, so the service
will always return false from Configure(). Later CLs will add command buffer
stub sharing and other requirements so that real decoders can be implemented.

BUG=522298
==========

to

==========
Finish plumbing MojoVideoDecoder.

This completes the work to connect from a renderer to the VideoDecoder service
in the GPU process, and implements a basic, functional VideoDecoder service.

There are no working VideoDecoders to actually instantiate yet, so the service
will always return false from Configure(). Later CLs will add command buffer
stub sharing and other requirements so that real decoders can be implemented.

BUG=522298

Committed: https://crrev.com/e78f9cb7d7bb4709436482572eb6fcb9a795e3b1
Cr-Commit-Position: refs/heads/master@{#395971}
==========

[commit-bot: I haz the power, 2016-05-25 20:08:42]

Patchset 14 (id:??) landed as
https://crrev.com/e78f9cb7d7bb4709436482572eb6fcb9a795e3b1
Cr-Commit-Position: refs/heads/master@{#395971}