Issue 551963004: media: scaffolding and plumbing for MojoRenderer{Impl, Service}

Issue 551963004: media: scaffolding and plumbing for MojoRenderer{Impl, Service} (Closed)

Created:
6 years, 3 months ago by tim (not reviewing)

Modified:
6 years, 3 months ago

Reviewers:
jamesr, brettw, xhwang, scherkus (not reviewing)

CC:
chromium-reviews, scherkus (not reviewing), DaveMoore, alokp

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Project:
chromium

Visibility:
Public.

More Reviews

Description

media: scaffolding and plumbing for MojoRenderer{Impl, Service} * Add skeleton client-side and service-side MediaRenderer pieces with basic plumbing and connections but no real logic. * model a media::DemuxerStream in a mojom to clean up MediaRenderer interface and line-up with RendererImpl expectations. * add DecoderConfig plumbing support * add a basic unittest verifying things do actually hook up * consolidate around 'media_mojo_' naming in media.gyp and add media_mojo target to build all this stuff. BUG=410451 Committed: https://crrev.com/09f86c05e4475d8044297a0bcf6c29aa12458b2e Cr-Commit-Position: refs/heads/master@{#295579}

Patch Set 1 #

Total comments: 91

Patch Set 2 : reviews #

Patch Set 3 : git cl format #

Patch Set 4 : rebase + DEPS #

Total comments: 2

Patch Set 5 : retry platforms #

Patch Set 6 : shared_memory_support #

Patch Set 7 : don't include mojo_shell #

Patch Set 8 : #

Patch Set 9 : gn #

Patch Set 10 : media gn #

Patch Set 11 : rebase #

Patch Set 12 : fix gn #

Patch Set 13 : actually fix gn #

Total comments: 2

Patch Set 14 : data() #

Patch Set 15 : add to //BUILD.gn #

Patch Set 16 : deps #

Patch Set 17 : oops #

Total comments: 2

Patch Set 18 : public_deps #

Created: 6 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1418 lines, -42 lines)			Patch
M	BUILD.gn	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+1 line, -0 lines	0 comments	Download
M	media/media.gyp	View	1 2 3 4 5 6 7 8 9 10	3 chunks	+64 lines, -7 lines	0 comments	Download
A +	media/mojo/BUILD.gn	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+9 lines, -6 lines	0 comments	Download
M	media/mojo/DEPS	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	media/mojo/interfaces/BUILD.gn	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+2 lines, -1 line	0 comments	Download
A	media/mojo/interfaces/demuxer_stream.mojom	View		1 chunk	+54 lines, -0 lines	0 comments	Download
M	media/mojo/interfaces/media_renderer.mojom	View		1 chunk	+3 lines, -10 lines	0 comments	Download
M	media/mojo/interfaces/media_types.mojom	View		1 chunk	+87 lines, -7 lines	0 comments	Download
M	media/mojo/services/BUILD.gn	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	2 chunks	+93 lines, -3 lines	0 comments	Download
M	media/mojo/services/media_type_converters.h	View	1 2	2 chunks	+10 lines, -0 lines	0 comments	Download
M	media/mojo/services/media_type_converters.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	3 chunks	+151 lines, -7 lines	0 comments	Download
M	media/mojo/services/media_type_converters_unittest.cc	View	1 2	3 chunks	+21 lines, -1 line	0 comments	Download
A	media/mojo/services/mojo_demuxer_stream_adapter.h	View	1 2 3	1 chunk	+75 lines, -0 lines	0 comments	Download
A	media/mojo/services/mojo_demuxer_stream_adapter.cc	View	1 2 3	1 chunk	+103 lines, -0 lines	0 comments	Download
A	media/mojo/services/mojo_demuxer_stream_impl.h	View	1 2	1 chunk	+53 lines, -0 lines	0 comments	Download
A	media/mojo/services/mojo_demuxer_stream_impl.cc	View	1 2	1 chunk	+60 lines, -0 lines	0 comments	Download
A	media/mojo/services/mojo_renderer_impl.h	View	1 2 3	1 chunk	+90 lines, -0 lines	0 comments	Download
A	media/mojo/services/mojo_renderer_impl.cc	View	1 2 3	1 chunk	+127 lines, -0 lines	0 comments	Download
A	media/mojo/services/mojo_renderer_service.h	View	1 2	1 chunk	+89 lines, -0 lines	0 comments	Download
A	media/mojo/services/mojo_renderer_service.cc	View	1 2 3	1 chunk	+141 lines, -0 lines	0 comments	Download
A	media/mojo/services/renderer_unittest.cc	View	1 2 3	1 chunk	+184 lines, -0 lines	0 comments	Download

Messages

Total messages: 33 (9 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

tim (not reviewing)

Hi Xiaohan, this is based off an earlier patch but is now officially ready for ...

6 years, 3 months ago (2014-09-08 23:40:38 UTC) #2

scherkus (not reviewing)

initial pass https://codereview.chromium.org/551963004/diff/1/media/mojo/interfaces/media_renderer.mojom File media/mojo/interfaces/media_renderer.mojom (right): https://codereview.chromium.org/551963004/diff/1/media/mojo/interfaces/media_renderer.mojom#newcode15 media/mojo/interfaces/media_renderer.mojom:15: Initialize(DemuxerStream stream) => (); FYI this will ...

6 years, 3 months ago (2014-09-09 20:35:33 UTC) #4

xhwang

Good patch! Here's my first round of comments. Quick question: where's the real IPC/MessagePipe code, ...

6 years, 3 months ago (2014-09-10 00:00:53 UTC) #5

tim (not reviewing)

@xhwang: the IPC code is all here! Thus is the magic of mojo. In fact, ...

6 years, 3 months ago (2014-09-10 23:08:31 UTC) #6

@xhwang: the IPC code is all here! Thus is the magic of mojo.  In fact, if you
patch this in and do

ninja -C out/Debug mojo
ninja -C out/Debug media_mojo
./out/Debug/mojo_shell mojo:media_mojo_renderer_apptest

it will cause a MojoRendererImpl to be created by the shell in one application
(on thread A). MRI then tries to connect to a MediaRenderer, meaning the shell
will actually spin up a second application (on thread B) and instantiate a
MojoRendererService for us. At this point the two ends are talking to each other
for realz in the test, and the call to Initialize actually makes round trips.

https://codereview.chromium.org/551963004/diff/1/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/551963004/diff/1/media/media.gyp#newcode1746
media/media.gyp:1746: ['OS=="linux"', {
On 2014/09/10 00:00:52, xhwang wrote:
> Any reason this is linux only?

Well, mojo stuff isn't really building anywhere at the moment aside from linux
and android afaik.
Also though, I think some of the media/audio stuff is different on (e.g) Mac?
AudioParameters?

https://codereview.chromium.org/551963004/diff/1/media/mojo/interfaces/media_...
File media/mojo/interfaces/media_renderer.mojom (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/interfaces/media_...
media/mojo/interfaces/media_renderer.mojom:15: Initialize(DemuxerStream stream)
=> ();
On 2014/09/09 20:35:32, scherkus wrote:
> FYI this will likely change in response to
> https://codereview.chromium.org/523283002/

OK... I'll loop back to this once I address the bulk of the comments.

https://codereview.chromium.org/551963004/diff/1/media/mojo/interfaces/media_...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/interfaces/media_...
media/mojo/interfaces/media_types.mojom:8: // Kept in sync with
media::BufferingState via COMPILE_ASSERTs.
On 2014/09/09 20:35:32, scherkus wrote:
> mojo development question: is it perhaps better to use mojo for defining enums
> once instead of keeping copies? it creates a dependency on mojo IDL ... but
> seems like less maintenance
> 
> this would be similar to using protobufs for creating simple POD classes
instead
> of writing a lot of boilerplate code

I asked an analogous question to mojo-dev about structs here,
https://groups.google.com/a/chromium.org/d/msg/mojo-dev/DeyYqV2wvVA/qp0H0Sc9visJ

I agree it's a burden to keep both in sync. But from that thread I kind of got
the impression that using mojom types in client code, while supported, wasn't
really the goal or intent. It's more cumbersome for structs, though, so perhaps
for enums it makes sense. In general I think it's up to the client (media in
this case) and not mojo... but I would caution the desire to change everything
over right now given that this mojo stuff is very much experimental...

https://codereview.chromium.org/551963004/diff/1/media/mojo/interfaces/media_...
media/mojo/interfaces/media_types.mojom:97: };
On 2014/09/10 00:00:52, xhwang wrote:
> I understand these are required to serialize a media::AudioDecoderConfig
through
> mojo MessagePipe. But this looks like a LOT of boilerplate code to me (see all
> the ASSERT_ENUM_EQ code below).
> 
> Can we support a mode where the class itself (e.g. media::AudioDecoderConfig)
> provides it's own way of serializing and deserializing, so that we don't need
to
> redefine a lot of stuff in mojo?

Hm, well, the only real point to using mojo is that you get serialization /
deserialization and IPC for free.  I think in the future it might make sense (as
Andrew alluded to above) to use mojo enums as the truth and maybe in some cases
structs. But note even here, |extra_data| will generate a mojo::Array. Although
mojo::Array was made to be mostly interchangeable with std::vector, this still
has implications on the surrounding code... so I might advise waiting for dust
to settle a bit more before deciding on a unified format.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/media_ty...
File media/mojo/services/media_type_converters.cc (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/media_ty...
media/mojo/services/media_type_converters.cc:29: ASSERT_ENUM_EQ(AudioCodec,
kCodec, AUDIO_CODEC_, AAC);
On 2014/09/09 20:35:32, scherkus wrote:
> potentially stupid idea: if having copies of enums is the norm in mojo land I
> wonder whether this boilerplate could get autogenerated by stating what it is
> supposed to be copied from
> 
> ... but I still think I'd rather have mojo be the sole source of enums :\

Yeah probably. It's tricky because enums aren't always consistent, e.g in media
you guys have kCamelCaseValue and CAPS_LOCK_ENUM_VALUE styles, and in fact even
within an enum things can be inconsistent (I didn't build a macro that was
general enough for e.g kCodecVorbis and kAudioCodecMax being part of the same
enum, either). Mojo on the other hand always prepends ENUM_NAME_ to values. But
yeah, you could probably do it making necessary changes to media/ code.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:30: read_cb));
On 2014/09/10 00:00:52, xhwang wrote:
> Based on the DemuxerStream API, we should have at most one pending read_cb at
> any time. So we can have a permanent |read_cb_| defined in this class. That
way,
> we don't need to bind the |read_cb| here.
> 
> Also, in case of flushing, we'll need to return the pending read_cb. Not sure
> how we'll implement that yet, but by binding the callback in a callback, we
> can't access the original callback easily...

I see.  I was going to say why bother adding state to |this| if we can tuck it
into the callback and forget about it, but if we need to return it on flush...
then I can add a member I guess.  Hmm, actually this isn't clear to me - if you
get kConfigChanged OnBufferReady, is the |buffer| always null, or can it be
provided?

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:39: DCHECK(false);  //
TODO(tim): Video unsupported.
On 2014/09/09 20:35:32, scherkus wrote:
> NOTREACHED()?

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:39: DCHECK(false);  //
TODO(tim): Video unsupported.
On 2014/09/10 00:00:52, xhwang wrote:
> NOTREACHED() given l.44.

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:49: }
On 2014/09/10 00:00:52, xhwang wrote:
> In media pipeline this is only used by GpuVideoDecoder. I guess the decoder we
> are targeting won't need this. How about just NOTREACHED()?

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:53: }
On 2014/09/10 00:00:52, xhwang wrote:
> This is only used by audio_renderer_impl.cc. So if we are not using ARI we can
> NOTREACHED() this. If we want to have a complete implementation, we need to
> plumb the value from the original DemuxerStream over here.

For now I'm using ARI.  In fact the test I added instantiates and initializes
one and does plumb the value over from the dx stream (See
OnAudioDecoderConfigChanged).

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:89:
DCHECK_GE(config_queue_.size(), 1U);
On 2014/09/09 20:35:32, scherkus wrote:
> DCHECK(!config_queue_.empty()) ?

There should be at least 1 element in the queue at this point; the current
config.
Or am I missing something?

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:95: on_config_change_cb_ =
base::Bind(read_cb, media_status, media_buffer);
On 2014/09/10 00:00:52, xhwang wrote:
> If we keep a class member read_cb_, we don't need on_config_change_cb_... 

See my question at 30, I might be misunderstanding OnBufferReady semantics
(because I'd like to avoid having to add multiple member variables to also
provide temp storage for the returned buffer as well as the callback)

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
File media/mojo/services/mojo_demuxer_stream_adapter.h (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.h:30:
MojoDemuxerStreamAdapter(mojo::DemuxerStreamPtr stream,
On 2014/09/10 00:00:52, xhwang wrote:
> nit: s/stream/demuxer_stream

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.h:31: const base::Closure&
stream_ready);
On 2014/09/10 00:00:52, xhwang wrote:
> nit: s/stream_ready/stream_ready_cb/

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.h:34: // media::DemuxerStream
implementation
On 2014/09/09 20:35:32, scherkus wrote:
> comments end w/ periods

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.h:58: base::Closure
stream_ready_callback_;
On 2014/09/10 00:00:52, xhwang wrote:
> nit: s/stream_ready_callback_/stream_ready_cb_/

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
File media/mojo/services/mojo_demuxer_stream_impl.cc (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_impl.cc:27: callback));
On 2014/09/10 00:00:52, xhwang wrote:
> In media code, we don't allow unsatisfied callbacks. So in theory, when we
> destroy |this| and if there's any pending callback (e.g. read cb), we should
> fire that callback. Binding that callback to a weak pointer will prevent us
from
> being able to do that.

The caller of this Read(...) is the MojoDXStreamAdapter, which is a mojo
client() of this object, meaning signalling for this sort of thing is typically
handled by the OnConnectionError stuff.  

I think what you're saying is that ARI would expect the callback to be satisfied
no matter what? I think that would be handled by the fact that
MojoDXStreamAdapter has the actual callback that ARI passed in cached, so we
should fire that OnConnectionError with ABORTED Status?

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_impl.cc:35: if (status ==
media::DemuxerStream::kConfigChanged) {
On 2014/09/09 20:35:32, scherkus wrote:
> to keep things simple for this CL (and future ones) we could also
> DCHECK/NOTIMPLEMENTED on this particular value until we get the basic
> non-mid-stream-config-change scenario up and running
> 
> WDYT?

Maybe I need to understand this better.  I added some support to plumb the
values across (in fact it was really helpful in figuring out a better model for
DemuxerStream than what I had before).  What else needs to happen at this level
(given that I'm using ARI)?

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
File media/mojo/services/mojo_demuxer_stream_impl.h (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_impl.h:40: mojo::MediaDecoderBufferPtr)>
callback,
On 2014/09/10 00:00:52, xhwang wrote:
> nit: typedef this callback?

OK, although it feels weird to use the typedef in the function signature for the
overridden function above.  Did you want to change that too?

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_impl.h:41: media::DemuxerStream::Status
status,
On 2014/09/09 20:35:32, scherkus wrote:
> remove media:: here and everywhere else
> 
> (80 characters are precious! we shouldn't waste it not needed!)

Well... there's media::DemuxerStream and mojo::DemuxerStream right now.

I suppose we could convert to using 'module media' in the mojom files and prefix
all the mojom interfaces with 'Mojo' :/

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
File media/mojo/services/mojo_renderer_impl.cc (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_impl.cc:25: DCHECK(audio_renderer_provider);
On 2014/09/09 20:35:32, scherkus wrote:
> would the next line crash if this was NULL?
> 
> if so ... typically just leave our the DCHECK

I mean, it's not clear right, which is why I left the DCHECK. If it was a raw
deref on the next line I'd leave it out also, but essentially this whole class
is written (for now) with the assumption that we're here if and only if audio,
which is separate from the call to mojo::ConnectToService.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_impl.cc:48: mojo::DemuxerStreamPtr dx_ptr;
On 2014/09/10 00:00:52, xhwang wrote:
> s/dx/demuxer_stream/

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_impl.cc:61:
remote_audio_renderer_->StartPlayingFrom(time.ToInternalValue());
On 2014/09/10 00:00:52, xhwang wrote:
> ToMicroseconds()?

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_impl.cc:81:
DCHECK(remote_audio_renderer_.get());  // We always bind the renderer.
On 2014/09/09 20:35:32, scherkus wrote:
> hrmm... will Has{Audio,Video}() eventually be methods on the
> mojo::MediaRenderer()?
> 
> IOW, should we eventually just have a single |remote_renderer_|?

I haven't really thought this through tbh; I was modelling it after RendererImpl
(see comment in .h).  If you go with two separate ones, it allows the
flexibility to connect to different applications for audio vs video (which may
or may not be useful). It also lets you return synchronously here (this may also
be useless, I don't know for sure).

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_impl.cc:115: // TODO(tim): Should we have
OnDecodeError and OnRenderError?
On 2014/09/09 20:35:32, scherkus wrote:
> I believe one day we'd want the remote mojo::MediaRenderer to be able to
provide
> an error code, but for now let's determine which error code to fire using the
> following heuristic:
>   - If OnError() is called before Initialize() callback fires, return
> PIPELINE_ERROR_COULD_NOT_RENDER
>   - If OnError() is called after Initialize() callback fires, return
> PIPELINE_ERROR_DECODE
> 
> If you're wondering, the difference is that these error codes get translated
> into HTMLMediaElement error codes (i.e,. are end-user visible) so we should
try
> to maintain compatibility:
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...

Done.  Thanks for the pointer.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
File media/mojo/services/mojo_renderer_impl.h (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_impl.h:31: // TODO(tim): Only audio is
currently supported. Bug 410451.
On 2014/09/09 20:35:33, scherkus wrote:
> URLify bugs
> 
> http://crbug.com/410451

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_impl.h:73: mojo::MediaRendererPtr
remote_audio_renderer_;
On 2014/09/10 00:00:52, xhwang wrote:
> s/remote_audio_renderer_/remote_renderer_/ ?

Hmm.  I was thinking of this as the analog to the fact that we pass both an
AudioRenderer and VideoRenderer in to RendererImpl. But I guess we could have a
single one?  Depends if you prefer parity with the existing stuff (assuming I
have it right)?

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_impl.h:75: // Callbacks passed to Initialize
that we forward messages from
On 2014/09/09 20:35:32, scherkus wrote:
> Initialize()

Dang it!  I thought I caught all of these :)

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
File media/mojo/services/mojo_renderer_service.cc (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:23: // TODO(tim): We should use
|connection|  passed to the MojoRendererService
On 2014/09/09 20:35:33, scherkus wrote:
> remove extra space before passed

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:26: class DummyAudioRendererSink :
public media::AudioRendererSink {
On 2014/09/09 20:35:33, scherkus wrote:
> you might be able to use media::NullAudioSink here instead

Ah thanks!

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:65: :
hardware_config_(media::AudioParameters(), media::AudioParameters()),
On 2014/09/09 20:35:33, scherkus wrote:
> remove media:: here and everywhere else

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:86: init_cb_ = callback;;
On 2014/09/09 20:35:33, scherkus wrote:
> remove extra ;

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:132:
client()->OnTimeUpdate(time.InMilliseconds(), max_time.InMilliseconds());
On 2014/09/10 00:00:52, xhwang wrote:
> InMicroseconds?

Uh oh.  Aaaaand here's why it's useful to have a time type :/

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:146: client()->OnError();
On 2014/09/10 00:00:52, xhwang wrote:
> Pass |error| in OnError()?

In the last CL I thought you proposed that the renderer-side only needed to know
whether or not there was an error, so we didn't mojo-ify PipelineStatus and
plumb it through the MediaRendererClient mojom.

We can do that now if you like?

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.cc:151: MojoResult MojoMain(MojoHandle
shell_handle) {
On 2014/09/09 20:35:33, scherkus wrote:
> does MojoMain() belong here?

It typically goes wherever the ApplicationDelegate is.  We could add a separate
mojo_renderer_application_main.cc file that just declares an ApplicationDelegate
instance and defines the MojoMain, but pretty often for most of the (basic)
example apps that only vend one service you'll see it be a part of the service
implementation file instead of an extra .cc.  

It's up to you guys.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
File media/mojo/services/mojo_renderer_service.h (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.h:53: void
OnAudioRendererInitializeDone(media::PipelineStatus status);
On 2014/09/09 20:35:33, scherkus wrote:
> remove media::

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_service.h:80: media::AudioHardwareConfig
hardware_config_;
On 2014/09/09 20:35:33, scherkus wrote:
> remove media::

Mmmh, same comment as before, there's both a media:: and mojo:: one of these.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/renderer...
File media/mojo/services/renderer_unittest.cc (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/renderer...
media/mojo/services/renderer_unittest.cc:48: class FakeDemuxer : public
media::Demuxer,
On 2014/09/09 20:35:33, scherkus wrote:
> when xhwang@'s DemuxerStreamProvider CL lands, you might be able to replace
this
> with the existing FakeDemuxerStream inside of media/
> 
> there's always MockDemuxer{Stream} as well if gmock is your thing

Well, FDS requires considerable work to support audio as well as it supports
video (if i understand correctly), so I did this because my needs are super
basic and it seemed weird to have a FDS implementation where the audio path is
an entirely lighter weight implementation than the video path in the same class.
 WDYT?

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/renderer...
media/mojo/services/renderer_unittest.cc:129: media::Demuxer* demuxer() { return
demuxer_.get(); }
On 2014/09/09 20:35:33, scherkus wrote:
> remove media:: here and everywhere else

Done.

scherkus (not reviewing)

do you want to rebase now that https://crrev.com/1c5668ece2d095df3f33d0fe8c63a5fce45d3573 has landed to use DemuxerStreamProvider? https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_renderer_impl.cc File ...

6 years, 3 months ago (2014-09-12 19:14:41 UTC) #7

xhwang

The CL lgtm. Later, we may want to use a Renderer interface instead of AudioRenderer ...

6 years, 3 months ago (2014-09-15 04:57:29 UTC) #8

The CL lgtm. Later, we may want to use a Renderer interface instead of
AudioRenderer interface in the service. But as scherkus mentioned, we can do it
in future CLs.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:30: read_cb));
On 2014/09/10 23:08:28, timsteele wrote:
> On 2014/09/10 00:00:52, xhwang wrote:
> > Based on the DemuxerStream API, we should have at most one pending read_cb
at
> > any time. So we can have a permanent |read_cb_| defined in this class. That
> way,
> > we don't need to bind the |read_cb| here.
> > 
> > Also, in case of flushing, we'll need to return the pending read_cb. Not
sure
> > how we'll implement that yet, but by binding the callback in a callback, we
> > can't access the original callback easily...
> 
> I see.  I was going to say why bother adding state to |this| if we can tuck it
> into the callback and forget about it, but if we need to return it on flush...
> then I can add a member I guess.  Hmm, actually this isn't clear to me - if
you
> get kConfigChanged OnBufferReady, is the |buffer| always null, or can it be
> provided?

For kConfigChanged, the buffer is always NULL.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:89:
DCHECK_GE(config_queue_.size(), 1U);
On 2014/09/10 23:08:28, timsteele wrote:
> On 2014/09/09 20:35:32, scherkus wrote:
> > DCHECK(!config_queue_.empty()) ?
> 
> There should be at least 1 element in the queue at this point; the current
> config.
> Or am I missing something?

scherkus was recommending to check !empty(), which is same as >=1 ?

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:95: on_config_change_cb_ =
base::Bind(read_cb, media_status, media_buffer);
On 2014/09/10 23:08:28, timsteele wrote:
> On 2014/09/10 00:00:52, xhwang wrote:
> > If we keep a class member read_cb_, we don't need on_config_change_cb_... 
> 
> See my question at 30, I might be misunderstanding OnBufferReady semantics
> (because I'd like to avoid having to add multiple member variables to also
> provide temp storage for the returned buffer as well as the callback)

See my reply above.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
File media/mojo/services/mojo_demuxer_stream_impl.h (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_impl.h:40: mojo::MediaDecoderBufferPtr)>
callback,
On 2014/09/10 23:08:29, timsteele wrote:
> On 2014/09/10 00:00:52, xhwang wrote:
> > nit: typedef this callback?
> 
> OK, although it feels weird to use the typedef in the function signature for
the
> overridden function above.  Did you want to change that too?

Yeah.. it's a pretty long callback, may just be typedef it in the base class.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
File media/mojo/services/mojo_renderer_impl.h (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_impl.h:73: mojo::MediaRendererPtr
remote_audio_renderer_;
On 2014/09/10 23:08:30, timsteele wrote:
> On 2014/09/10 00:00:52, xhwang wrote:
> > s/remote_audio_renderer_/remote_renderer_/ ?
> 
> Hmm.  I was thinking of this as the analog to the fact that we pass both an
> AudioRenderer and VideoRenderer in to RendererImpl. But I guess we could have
a
> single one?  Depends if you prefer parity with the existing stuff (assuming I
> have it right)?

I see your point here. Let's keep this and iterate later.

tim (not reviewing)

+jamesr for mojo/application approval after https://codereview.chromium.org/565323002/ https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_demuxer_stream_adapter.cc File media/mojo/services/mojo_demuxer_stream_adapter.cc (right): https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_demuxer_stream_adapter.cc#newcode30 media/mojo/services/mojo_demuxer_stream_adapter.cc:30: read_cb)); On 2014/09/15 ...

6 years, 3 months ago (2014-09-15 21:52:51 UTC) #12

+jamesr for mojo/application approval after
https://codereview.chromium.org/565323002/

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:30: read_cb));
On 2014/09/15 04:57:28, xhwang wrote:
> On 2014/09/10 23:08:28, timsteele wrote:
> > On 2014/09/10 00:00:52, xhwang wrote:
> > > Based on the DemuxerStream API, we should have at most one pending read_cb
> at
> > > any time. So we can have a permanent |read_cb_| defined in this class.
That
> > way,
> > > we don't need to bind the |read_cb| here.
> > > 
> > > Also, in case of flushing, we'll need to return the pending read_cb. Not
> sure
> > > how we'll implement that yet, but by binding the callback in a callback,
we
> > > can't access the original callback easily...
> > 
> > I see.  I was going to say why bother adding state to |this| if we can tuck
it
> > into the callback and forget about it, but if we need to return it on
flush...
> > then I can add a member I guess.  Hmm, actually this isn't clear to me - if
> you
> > get kConfigChanged OnBufferReady, is the |buffer| always null, or can it be
> > provided?
> 
> For kConfigChanged, the buffer is always NULL.

Acknowledged.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:89:
DCHECK_GE(config_queue_.size(), 1U);
On 2014/09/15 04:57:28, xhwang wrote:
> On 2014/09/10 23:08:28, timsteele wrote:
> > On 2014/09/09 20:35:32, scherkus wrote:
> > > DCHECK(!config_queue_.empty()) ?
> > 
> > There should be at least 1 element in the queue at this point; the current
> > config.
> > Or am I missing something?
> 
> scherkus was recommending to check !empty(), which is same as >=1 ?

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_adapter.cc:95: on_config_change_cb_ =
base::Bind(read_cb, media_status, media_buffer);
On 2014/09/15 04:57:28, xhwang wrote:
> On 2014/09/10 23:08:28, timsteele wrote:
> > On 2014/09/10 00:00:52, xhwang wrote:
> > > If we keep a class member read_cb_, we don't need on_config_change_cb_... 
> > 
> > See my question at 30, I might be misunderstanding OnBufferReady semantics
> > (because I'd like to avoid having to add multiple member variables to also
> > provide temp storage for the returned buffer as well as the callback)
> 
> See my reply above.

Acknowledged.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
File media/mojo/services/mojo_demuxer_stream_impl.h (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_dem...
media/mojo/services/mojo_demuxer_stream_impl.h:40: mojo::MediaDecoderBufferPtr)>
callback,
On 2014/09/15 04:57:28, xhwang wrote:
> On 2014/09/10 23:08:29, timsteele wrote:
> > On 2014/09/10 00:00:52, xhwang wrote:
> > > nit: typedef this callback?
> > 
> > OK, although it feels weird to use the typedef in the function signature for
> the
> > overridden function above.  Did you want to change that too?
> 
> Yeah.. it's a pretty long callback, may just be typedef it in the base class.

Done.

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
File media/mojo/services/mojo_renderer_impl.h (right):

https://codereview.chromium.org/551963004/diff/1/media/mojo/services/mojo_ren...
media/mojo/services/mojo_renderer_impl.h:73: mojo::MediaRendererPtr
remote_audio_renderer_;
On 2014/09/15 04:57:29, xhwang wrote:
> On 2014/09/10 23:08:30, timsteele wrote:
> > On 2014/09/10 00:00:52, xhwang wrote:
> > > s/remote_audio_renderer_/remote_renderer_/ ?
> > 
> > Hmm.  I was thinking of this as the analog to the fact that we pass both an
> > AudioRenderer and VideoRenderer in to RendererImpl. But I guess we could
have
> a
> > single one?  Depends if you prefer parity with the existing stuff (assuming
I
> > have it right)?
> 
> I see your point here. Let's keep this and iterate later.

Acknowledged.

tim (not reviewing)

I should say, +jamesr for media/mojo/DEPS approval (I added mojo/application).

6 years, 3 months ago (2014-09-15 21:53:32 UTC) #13

jamesr

https://codereview.chromium.org/551963004/diff/100001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/551963004/diff/100001/media/media.gyp#newcode1765 media/media.gyp:1765: ['OS=="linux"', { what is linux only about this section?

6 years, 3 months ago (2014-09-15 22:03:14 UTC) #14

tim (not reviewing)

6 years, 3 months ago (2014-09-15 23:59:42 UTC) #16

jamesr

If you do change it, you need to modify the GN equivalent as well. But ...

6 years, 3 months ago (2014-09-16 00:25:28 UTC) #17

tim (not reviewing)

On 2014/09/16 00:25:28, jamesr wrote: > If you do change it, you need to modify ...

6 years, 3 months ago (2014-09-16 05:09:25 UTC) #19

jamesr

Could you please update the GN build as well for the changes you are making?

6 years, 3 months ago (2014-09-16 17:13:34 UTC) #20

tim (not reviewing)

On 2014/09/16 17:13:34, jamesr wrote: > Could you please update the GN build as well ...

6 years, 3 months ago (2014-09-17 18:45:27 UTC) #22

jamesr

lgtm w/ one suggestion. sorry for delaying this so much. I want to have idiomatic-ish ...

6 years, 3 months ago (2014-09-17 21:55:49 UTC) #23

tim (not reviewing)

On 2014/09/17 21:55:49, jamesr wrote: > lgtm w/ one suggestion. sorry for delaying this so ...

6 years, 3 months ago (2014-09-17 22:16:27 UTC) #24

jamesr

On 2014/09/17 22:16:27, timsteele wrote: > On 2014/09/17 21:55:49, jamesr wrote: > > lgtm w/ ...

6 years, 3 months ago (2014-09-17 22:24:12 UTC) #25

brettw

lgtm https://codereview.chromium.org/551963004/diff/380001/media/mojo/BUILD.gn File media/mojo/BUILD.gn (right): https://codereview.chromium.org/551963004/diff/380001/media/mojo/BUILD.gn#newcode6 media/mojo/BUILD.gn:6: deps = [ Can you change this to ...

6 years, 3 months ago (2014-09-18 20:11:00 UTC) #28

tim (not reviewing)

thanks! https://codereview.chromium.org/551963004/diff/380001/media/mojo/BUILD.gn File media/mojo/BUILD.gn (right): https://codereview.chromium.org/551963004/diff/380001/media/mojo/BUILD.gn#newcode6 media/mojo/BUILD.gn:6: deps = [ On 2014/09/18 20:10:59, brettw wrote: ...

6 years, 3 months ago (2014-09-18 21:15:23 UTC) #29

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/551963004/400001

6 years, 3 months ago (2014-09-18 21:18:16 UTC) #31

commit-bot: I haz the power

Committed patchset #18 (id:400001) as 6a1aeaf19b4233542fdb6d3de2c3f36bdf426632

6 years, 3 months ago (2014-09-18 22:10:24 UTC) #32

commit-bot: I haz the power

6 years, 3 months ago (2014-09-18 22:11:10 UTC) #33

Message was sent while issue was closed.

Patchset 18 (id:??) landed as
https://crrev.com/09f86c05e4475d8044297a0bcf6c29aa12458b2e
Cr-Commit-Position: refs/heads/master@{#295579}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages