Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1896883002:
Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo (Closed)
|
Created:
4 years, 8 months ago by rchtara Modified:
4 years, 7 months ago Reviewers:
Henrik Grunell, tommi (sloooow) - chröme, Niklas Enbom, jam, dcheng, mcasas, Mike West, DaleCurtis, xhwang, miu CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, creis+watch_chromium.org, Charlie Reis, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, miu+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMojo interfaces needed for switching audio rendering stream
creation and closing from IPC to Mojo
This adds:
* AudioOutput which allows managing audio output streams.
* AudioOutputStream which handles audio output stream operations.
* Type mapping for AudioParameters.
This is the media part: The content part is in the CL http://codereview.chromium.org/1930393002
BUG=606707
Committed: https://crrev.com/78870627c0044f56533da4746e4b3231d04d87f1
Cr-Commit-Position: refs/heads/master@{#393537}
Patch Set 1 #Patch Set 2 : add windows support #Patch Set 3 : rebase #
Total comments: 14
Patch Set 4 : rebase #
Total comments: 60
Patch Set 5 : tests compiling #Patch Set 6 : fix #Patch Set 7 : All grunell comments resolved #
Total comments: 77
Patch Set 8 : #
Total comments: 10
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : remove content files #
Total comments: 35
Patch Set 12 : unit_tests #Patch Set 13 : unit_tests #Patch Set 14 : rebase #
Total comments: 7
Patch Set 15 : add audio_parameters #Patch Set 16 : rm includes #Patch Set 17 : render_frame_id #
Total comments: 11
Patch Set 18 : move audio_output.mojom #
Total comments: 10
Patch Set 19 : add close #
Total comments: 6
Patch Set 20 : #Patch Set 21 : #Patch Set 22 : #
Total comments: 6
Patch Set 23 : #Patch Set 24 : #Patch Set 25 : #Patch Set 26 : remove render_frame_id #Patch Set 27 : add TODO for CreateStream #
Total comments: 4
Patch Set 28 : move interface to media/use convertor rebase #
Total comments: 11
Patch Set 29 : fixgn #Patch Set 30 : fix #Patch Set 31 : add deps #Patch Set 32 : nochromecast #Patch Set 33 : nochrome #Patch Set 34 : nocontentbrowser #Patch Set 35 : nocontentrenderer #Patch Set 36 : nochrome #Patch Set 37 : nochromecontent #Patch Set 38 #Patch Set 39 : nocommon #Patch Set 40 : rename media.interfaces - media.mojom #Messages
Total messages: 128 (30 generated)
Description was changed from ========== Switch Chrome Audio Rendering IPC to Mojo: Use Mojo for stream creation and stream closing. BUG= ========== to ========== Switch Chrome Audio Rendering IPC to Mojo: Use Mojo for stream creation and stream closing. BUG= ==========
rchtara@chromium.org changed reviewers: + grunell@chromium.org
Hello! Could you please have a look to this cl! Thanks a lot Riadh
First round. - Create a bug and link to it. - Update and add tests. - Make sure you get early feedback from the owners you plan to add as reviewers. https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:108: // is stripped away in the WebRTC audio input pipeline and never seen outside Nit: 80 chars. https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:122: int32 channels_; // Number of channels. Value set based on Align the comments. https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:131: // Used on the renderer side. Remove this comment and same below. The interface is general and should not care about processes etc. (I had that comment (I think) in the proposal just to point out how it will be used.) https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:133: Close(); Did you look into if it's possible to use delete/unbind of the object as a way to notify the service that it should be closed? https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:139: int32 stream_id_, No trailing _ in these variables. https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:139: int32 stream_id_, Add a comment (before CreateStream) and explain why the stream_id is needed. https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:143: int32 stream_id, Put stream_id first. https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... File content/browser/media/audio_output_impl.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.cc:46: entry_(entry), Seems like the entry isn't used? Also, the renderer host already keeps track of the entries by stream id so it should only be necessary to know about the renderer host in this class. https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.cc:61: base::Bind(&AudioOutputImpl::InitMojo, base::Unretained(this), How is base::Unretained safe here? https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.cc:75: audio_renderer_host_->OnCreateStream(stream_id, render_frame_id, audio_params, I think you should remove "On" from OnCreateStream - it's an IPC style. (Unless that's also a common mojo style.) https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... File content/browser/media/audio_output_impl.h (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.h:40: AudioRendererHost::AudioEntry* entry_; What's the lifetime of AudioOutputStreamImpl? I.e. is the AudioEntry and AudioRendererHost guaranteed to outlive it? https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.h:63: mojom::AudioOutputStreamPtr* StreamFactory( Who calls this function? Comments on the functions would be good. https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.h:81: std::map<int, scoped_ptr<AudioOutputStreamImpl>> stream_impls; Add trailing _ to member variable. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:217: const mojom::AudioOutput::CreateStreamCallback& callback) { Can the callback be stored in the AudioEntry instead? https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:267: scoped_ptr<mojom::AudioOutputStreamPtr> stream_ptr( std::unique_ptr. Here and elsewhere. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:303: // Mojo is going to close the socket handle when Mojo shutdowns. I'm not sure what Mojo means here. And what "when Mojo shutdowns" exactly means. Could you rephrase this section? https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:305: // the socket is going to need to be closed too. This line is hard to parse. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:309: #if defined(OS_WIN) This duplication looks like a good candidate for a separate function. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:574: scoped_ptr<base::SharedMemory> shared_memory(new base::SharedMemory()); scoped_ptr is deprecated. Keep std::unique_ptr. (Not sure why you changed. :)) https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:707: void AudioRendererHost::ReportErrorAndCloseMojo( "CloseMojo" sounds like the whole mojo infrastructure would be closed. :) It seems to be the stream that is closed, right? https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.h:88: class CONTENT_EXPORT AudioEntry Why does it have to be moved to the header? https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1084: base::Bind(&AudioOutputImpl::CreateService, audio_renderer_host_)); How long does this callback live? I.e. how long is the ref held to ARH? https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... File content/renderer/media/audio_message_filter.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_message_filter.cc:110: filter_->audio_output_client_->CreateStream(stream_id_, render_frame_id_, Is it possible to not go through the IPCImpl in this intermediate state this CL creates? It's not a big deal since upcoming CLs will remove the filter, but it would be nice. Add todo comments that this is temporary and will soon be changed. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... File content/renderer/media/audio_output_client.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 Same elsewhere. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:24: // TODO(rchtara): Check that the enum in mojo and in the rendrer are the some This must be done in this CL. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:46: int findKeys( Change name to something that describes a bit more in details what it does. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:46: int findKeys( Capital first letter. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:47: const std::map<int, scoped_ptr<content::mojom::AudioOutputStreamPtr>>& map, Paramater name "map" doesn't say anything. Improve name. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:47: const std::map<int, scoped_ptr<content::mojom::AudioOutputStreamPtr>>& map, typedef the map. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:49: for (auto it = map.begin(); it != map.end(); ++it) { You can do for (auto it: map) { ... } https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:59: namespace content { Put on top. (Anonymous namespace nested in this.) https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:60: Remove two lines breaks. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... File content/renderer/media/audio_output_client.h (right): https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.h:44: void CreateStream(int stream_id, Add comments to all functions. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.h:76: AudioMessageFilter* audio_message_filter_; How is it guaranteed that the filter outlives this object?
Also, the description must contain much more info. It should answer what? and why? in a clear, structured way. Bullets are often useful.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
+me so I remember to look over this.
Thanks a lot for the review, could you have a look again https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:108: // is stripped away in the WebRTC audio input pipeline and never seen outside On 2016/04/19 15:36:08, Henrik Grunell wrote: > Nit: 80 chars. Done. https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:122: int32 channels_; // Number of channels. Value set based on On 2016/04/19 15:36:08, Henrik Grunell wrote: > Align the comments. Done. https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:131: // Used on the renderer side. On 2016/04/19 15:36:08, Henrik Grunell wrote: > Remove this comment and same below. The interface is general and should not care > about processes etc. (I had that comment (I think) in the proposal just to point > out how it will be used.) Acknowledged. https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:133: Close(); On 2016/04/19 15:36:08, Henrik Grunell wrote: > Did you look into if it's possible to use delete/unbind of the object as a way > to notify the service that it should be closed? yes, I did. but as you probably remember, we decided to keep that to report errors from the browser to renderer as it's harder to do. https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:139: int32 stream_id_, On 2016/04/19 15:36:08, Henrik Grunell wrote: > No trailing _ in these variables. Done. https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:139: int32 stream_id_, On 2016/04/19 15:36:08, Henrik Grunell wrote: > Add a comment (before CreateStream) and explain why the stream_id is needed. Done. https://codereview.chromium.org/1896883002/diff/40001/media/mojo/interfaces/a... media/mojo/interfaces/audio_output.mojom:143: int32 stream_id, On 2016/04/19 15:36:08, Henrik Grunell wrote: > Put stream_id first. Done. https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... File content/browser/media/audio_output_impl.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.cc:46: entry_(entry), On 2016/04/19 15:36:08, Henrik Grunell wrote: > Seems like the entry isn't used? Also, the renderer host already keeps track of > the entries by stream id so it should only be necessary to know about the > renderer host in this class. Done. https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.cc:75: audio_renderer_host_->OnCreateStream(stream_id, render_frame_id, audio_params, On 2016/04/19 15:36:08, Henrik Grunell wrote: > I think you should remove "On" from OnCreateStream - it's an IPC style. (Unless > that's also a common mojo style.) Done. https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... File content/browser/media/audio_output_impl.h (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.h:40: AudioRendererHost::AudioEntry* entry_; On 2016/04/19 15:36:08, Henrik Grunell wrote: > What's the lifetime of AudioOutputStreamImpl? I.e. is the AudioEntry and > AudioRendererHost guaranteed to outlive it? |audio_renderer_host_| is scoped_refptr in AudioOutputImpl. And because all AudioOutputStreamImpl instances are going to be owned by AudioOutputImpl, it's safe to access |audio_renderer_host| from AudioOutputStreamImpl. The only issue that could happen is during AudioOutputStreamImpl destructor. |audio_renderer_host| cannot access from there. enry is removed' https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.h:63: mojom::AudioOutputStreamPtr* StreamFactory( On 2016/04/19 15:36:08, Henrik Grunell wrote: > Who calls this function? Comments on the functions would be good. Done. https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.h:81: std::map<int, scoped_ptr<AudioOutputStreamImpl>> stream_impls; On 2016/04/19 15:36:08, Henrik Grunell wrote: > Add trailing _ to member variable. Done. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:217: const mojom::AudioOutput::CreateStreamCallback& callback) { On 2016/04/19 15:36:09, Henrik Grunell wrote: > Can the callback be stored in the AudioEntry instead? it's possible to do that, but we are going to have soon other callbacks after we mojofy play, pause, ... So we need to store them somewhere, and they could come at the some time which makes that even more complicated. I think it's better to bind them to avoid issues. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:267: scoped_ptr<mojom::AudioOutputStreamPtr> stream_ptr( On 2016/04/19 15:36:09, Henrik Grunell wrote: > std::unique_ptr. Here and elsewhere. Done. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:303: // Mojo is going to close the socket handle when Mojo shutdowns. On 2016/04/19 15:36:08, Henrik Grunell wrote: > I'm not sure what Mojo means here. And what "when Mojo shutdowns" exactly means. > Could you rephrase this section? Done. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:305: // the socket is going to need to be closed too. On 2016/04/19 15:36:09, Henrik Grunell wrote: > This line is hard to parse. Done. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:309: #if defined(OS_WIN) On 2016/04/19 15:36:09, Henrik Grunell wrote: > This duplication looks like a good candidate for a separate function. Done. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:574: scoped_ptr<base::SharedMemory> shared_memory(new base::SharedMemory()); On 2016/04/19 15:36:09, Henrik Grunell wrote: > scoped_ptr is deprecated. Keep std::unique_ptr. (Not sure why you changed. :)) Done. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:707: void AudioRendererHost::ReportErrorAndCloseMojo( On 2016/04/19 15:36:09, Henrik Grunell wrote: > "CloseMojo" sounds like the whole mojo infrastructure would be closed. :) It > seems to be the stream that is closed, right? :) yes exactly. Done. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.h:88: class CONTENT_EXPORT AudioEntry On 2016/04/19 15:36:09, Henrik Grunell wrote: > Why does it have to be moved to the header? Because, I'm using the AudioEntry in the audio_output_impl.h But, as we are not going to do that in this cl, I will cancel this. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1084: base::Bind(&AudioOutputImpl::CreateService, audio_renderer_host_)); On 2016/04/19 15:36:09, Henrik Grunell wrote: > How long does this callback live? I.e. how long is the ref held to ARH? The callback is going to live until |mojo_application_host_| is reset. After that, the ref to ARH is going to be released. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... File content/renderer/media/audio_message_filter.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_message_filter.cc:110: filter_->audio_output_client_->CreateStream(stream_id_, render_frame_id_, On 2016/04/19 15:36:09, Henrik Grunell wrote: > Is it possible to not go through the IPCImpl in this intermediate state this CL > creates? It's not a big deal since upcoming CLs will remove the filter, but it > would be nice. Add todo comments that this is temporary and will soon be > changed. I think that this is going to be very hard, as many functions still rely on delegates_ map to detect errors. When all the IPC is removed, we could have a similar map in AudioOutputClient and use it to detect problems. Right now, the ipc part is still using the delegates_ map, the mojo part needs to use it too. So, i think it'd going to be better to be patient and replace these functions when pause,play.. are mojofyied . https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... File content/renderer/media/audio_output_client.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/19 15:36:09, Henrik Grunell wrote: > 2016 > > Same elsewhere. Done. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:24: // TODO(rchtara): Check that the enum in mojo and in the rendrer are the some On 2016/04/19 15:36:09, Henrik Grunell wrote: > This must be done in this CL. Done. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:46: int findKeys( On 2016/04/19 15:36:09, Henrik Grunell wrote: > Capital first letter. Done. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:46: int findKeys( On 2016/04/19 15:36:09, Henrik Grunell wrote: > Change name to something that describes a bit more in details what it does. Done. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:47: const std::map<int, scoped_ptr<content::mojom::AudioOutputStreamPtr>>& map, On 2016/04/19 15:36:09, Henrik Grunell wrote: > Paramater name "map" doesn't say anything. Improve name. Done. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:47: const std::map<int, scoped_ptr<content::mojom::AudioOutputStreamPtr>>& map, On 2016/04/19 15:36:09, Henrik Grunell wrote: > typedef the map. Done. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:49: for (auto it = map.begin(); it != map.end(); ++it) { On 2016/04/19 15:36:09, Henrik Grunell wrote: > You can do > for (auto it: map) { > ... > } Done. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:59: namespace content { On 2016/04/19 15:36:09, Henrik Grunell wrote: > Put on top. (Anonymous namespace nested in this.) Done. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.cc:60: On 2016/04/19 15:36:09, Henrik Grunell wrote: > Remove two lines breaks. Done. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... File content/renderer/media/audio_output_client.h (right): https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.h:44: void CreateStream(int stream_id, On 2016/04/19 15:36:09, Henrik Grunell wrote: > Add comments to all functions. Done. https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_output_client.h:76: AudioMessageFilter* audio_message_filter_; On 2016/04/19 15:36:09, Henrik Grunell wrote: > How is it guaranteed that the filter outlives this object? I will replace it by scoped_refptr<AudioMessageFilter>
Thanks a lot for the review, could you please have a look again?
https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... File content/browser/media/audio_output_impl.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.cc:61: base::Bind(&AudioOutputImpl::InitMojo, base::Unretained(this), On 2016/04/19 15:36:08, Henrik Grunell wrote: > How is base::Unretained safe here? Done.
rchtara@chromium.org changed reviewers: + tommi@chromium.org
On 2016/04/20 19:23:20, DaleCurtis wrote: > +me so I remember to look over this. Thanks a lot dale.
Hi tommi, As I told you,I'm still working on the tests. Could you please provide me with feedback? Thanks a lot Riadh
Expand the description and add bug (see previous comments). I think owners should take a high level look at this now. https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... File content/browser/media/audio_output_impl.h (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.h:40: AudioRendererHost::AudioEntry* entry_; On 2016/04/21 09:10:17, rchtara wrote: > On 2016/04/19 15:36:08, Henrik Grunell wrote: > > What's the lifetime of AudioOutputStreamImpl? I.e. is the AudioEntry and > > AudioRendererHost guaranteed to outlive it? > > |audio_renderer_host_| is scoped_refptr in AudioOutputImpl. And because all > AudioOutputStreamImpl instances are going to be owned by AudioOutputImpl, > it's safe to access |audio_renderer_host| from AudioOutputStreamImpl. The > only issue that could happen is during AudioOutputStreamImpl destructor. > |audio_renderer_host| cannot access from there. > enry is removed' OK, but what if the reference is released before AudioOutputStreamImpl when AudioOutputImpl is deleted, and it's the last reference. Then AudioRendererHost is deleted before AudioOutputStreamImpl and we could have a use after free. https://codereview.chromium.org/1896883002/diff/60001/content/browser/media/a... content/browser/media/audio_output_impl.h:63: mojom::AudioOutputStreamPtr* StreamFactory( On 2016/04/21 09:10:17, rchtara wrote: > On 2016/04/19 15:36:08, Henrik Grunell wrote: > > Who calls this function? Comments on the functions would be good. > > Done. Don't specify the function that calls it, that can change any time. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:217: const mojom::AudioOutput::CreateStreamCallback& callback) { On 2016/04/21 09:10:18, rchtara wrote: > On 2016/04/19 15:36:09, Henrik Grunell wrote: > > Can the callback be stored in the AudioEntry instead? > it's possible to do that, but we are going to have soon other callbacks after we > mojofy play, pause, ... > So we need to store them somewhere, and they could come at the some time which > makes that even more complicated. > I think it's better to bind them to avoid issues. Acknowledged. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.h:88: class CONTENT_EXPORT AudioEntry On 2016/04/21 09:10:18, rchtara wrote: > On 2016/04/19 15:36:09, Henrik Grunell wrote: > > Why does it have to be moved to the header? > > Because, I'm using the AudioEntry in the audio_output_impl.h > But, as we are not going to do that in this cl, I will cancel this. Acknowledged. https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1084: base::Bind(&AudioOutputImpl::CreateService, audio_renderer_host_)); On 2016/04/21 09:10:18, rchtara wrote: > On 2016/04/19 15:36:09, Henrik Grunell wrote: > > How long does this callback live? I.e. how long is the ref held to ARH? > > The callback is going to live until |mojo_application_host_| is reset. After > that, the ref to ARH is going to be released. > > When is it reset? https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... File content/renderer/media/audio_message_filter.cc (right): https://codereview.chromium.org/1896883002/diff/60001/content/renderer/media/... content/renderer/media/audio_message_filter.cc:110: filter_->audio_output_client_->CreateStream(stream_id_, render_frame_id_, On 2016/04/21 09:10:18, rchtara wrote: > On 2016/04/19 15:36:09, Henrik Grunell wrote: > > Is it possible to not go through the IPCImpl in this intermediate state this > CL > > creates? It's not a big deal since upcoming CLs will remove the filter, but it > > would be nice. Add todo comments that this is temporary and will soon be > > changed. > > I think that this is going to be very hard, as many functions still rely on > delegates_ map to detect errors. > When all the IPC is removed, we could have a similar map in AudioOutputClient > and use it to detect problems. > Right now, the ipc part is still using the delegates_ map, the mojo part needs > to use it too. > So, i think it'd going to be better to be patient and replace these functions > when pause,play.. are mojofyied . Acknowledged. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... File content/browser/media/audio_output_impl.h (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:24: // All the instance of this class are going to be owned by AudioOutputImpl. Put comment before the "class" line. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:24: // All the instance of this class are going to be owned by AudioOutputImpl. Add a brief general description of the class. Then you can just have "Owned by AudioOutputImpl." after that. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:39: // |audio_renderer_host_| is scoped_refptr in AudioOutputImpl. And because all You could put it as: "AudioRendererHost is ref counted and AudioOutputImpl holds a reference to it." https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:40: // AudioOutputStreamImpl instances are going to be owned by AudioOutputImpl "going to be" means that it will happen in the future. Maybe something like "AudioOutputStreamImpl is owned by AudioOutputImpl". https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:60: // Initialize Mojo on the BrowserThread::IO. Mojo is going to be bound to Does it create or only initialize? Both the comment and function name should reflect what it does. "Mojo" -> "Mojo service" "BrowserThread::IO" -> "IO thread" "Mojo is going to be bound" -> "The service is bound" Rephrase/clarify "future Mojo calls". (What is a "mojo call"?) https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:381: // The socket handle is going to be closed when |mojo_application_host_| is Why do we need two handles actually? Only one is used in the end, right? Can it be passed instead? https://codereview.chromium.org/1896883002/diff/120001/content/renderer/media... File content/renderer/media/audio_message_filter.cc (right): https://codereview.chromium.org/1896883002/diff/120001/content/renderer/media... content/renderer/media/audio_message_filter.cc:110: // The plan is to try to completely remove AudioMessageFilter and move all You could put it as: "This is temporary. In soon upcoming CLs AudioMessageFilter and AudioMessageFilter::AudioOutputIPCImpl will be removed and replaced by AudioOutputClient.
Description was changed from ========== Switch Chrome Audio Rendering IPC to Mojo: Use Mojo for stream creation and stream closing. BUG= ========== to ========== Switch stream creation and closing in Chrome audio rendering from IPC to Mojo This replaces audio stream creation and closing by using Mojo instead of IPC. It adds a few Mojo interfaces and AudioOuputClient and AudioOutputImpl that implement the Mojo client and service. Mojo is better than IPC because: * Mojo has a better abstraction: it allows easily to call functions from the client to server in contrast with IPC which requires handling sending messages manually. This enables to create a better architecture. * Mojo auto-generates code for handling communication between the renderer and the browser which allows in the future to get rid of AudioMessageFilter and AudioRendererHost and move the logic inside them into a more compact AudioOuputClient and AudioOutputImpl classes. BUG=605954 ==========
I haven't looked over much yet, but I'm surprised to see more code added than deleted here. I was thinking this would reduce the amount of moving parts. Is this because we're in an in between phase by having both IPC and Mojo?
On 2016/04/22 22:09:40, DaleCurtis wrote: > I haven't looked over much yet, but I'm surprised to see more code added than > deleted here. I was thinking this would reduce the amount of moving parts. Is > this because we're in an in between phase by having both IPC and Mojo? Yes, this is intermediate. The old IPC will be removed in upcoming CL(s). Riadh: remove the explanation of what Mojo is and its benefits from the description. Instead outline the planned CLs and what will be done in them.
Description was changed from ========== Switch stream creation and closing in Chrome audio rendering from IPC to Mojo This replaces audio stream creation and closing by using Mojo instead of IPC. It adds a few Mojo interfaces and AudioOuputClient and AudioOutputImpl that implement the Mojo client and service. Mojo is better than IPC because: * Mojo has a better abstraction: it allows easily to call functions from the client to server in contrast with IPC which requires handling sending messages manually. This enables to create a better architecture. * Mojo auto-generates code for handling communication between the renderer and the browser which allows in the future to get rid of AudioMessageFilter and AudioRendererHost and move the logic inside them into a more compact AudioOuputClient and AudioOutputImpl classes. BUG=605954 ========== to ========== Switch stream creation and closing in Chrome audio rendering from IPC to Mojo This replaces audio stream creation and closing by using Mojo instead of IPC. It adds a few Mojo interfaces and AudioOuputClient and AudioOutputImpl that implement the Mojo client and service. In future CLs: * Mojo is going to be used for all the other audio output stream operations (playing, pausing, ..) * AudioRendererHost and AudioMessageFilter are going to be removed and the logic inside them is going to move to AudioOuputClient and AudioOutputImpl. BUG=605954 ==========
On 2016/04/25 08:02:25, Henrik Grunell wrote: > On 2016/04/22 22:09:40, DaleCurtis wrote: > > I haven't looked over much yet, but I'm surprised to see more code added than > > deleted here. I was thinking this would reduce the amount of moving parts. Is > > this because we're in an in between phase by having both IPC and Mojo? > > Yes, this is intermediate. The old IPC will be removed in upcoming CL(s). > > Riadh: remove the explanation of what Mojo is and its benefits from the > description. Instead outline the planned CLs and what will be done in them. Yes, exactly, this is just a temporary CLs. Later, AudioRendererHost and AudioMessageFilter are going to be completely removed.
Description was changed from ========== Switch stream creation and closing in Chrome audio rendering from IPC to Mojo This replaces audio stream creation and closing by using Mojo instead of IPC. It adds a few Mojo interfaces and AudioOuputClient and AudioOutputImpl that implement the Mojo client and service. In future CLs: * Mojo is going to be used for all the other audio output stream operations (playing, pausing, ..) * AudioRendererHost and AudioMessageFilter are going to be removed and the logic inside them is going to move to AudioOuputClient and AudioOutputImpl. BUG=605954 ========== to ========== Switch stream creation and closing in Chrome audio rendering from IPC to Mojo This replaces audio stream creation and closing by using Mojo instead of IPC. It adds a few Mojo interfaces and AudioOuputClient and AudioOutputImpl that implement the Mojo client and service. In future CLs: * Mojo is will be used for all the other audio output stream operations (playing, pausing, ..) * AudioRendererHost and AudioMessageFilter will be removed and the logic inside them will be moved to AudioOuputClient and AudioOutputImpl. BUG=605954 ==========
Description was changed from ========== Switch stream creation and closing in Chrome audio rendering from IPC to Mojo This replaces audio stream creation and closing by using Mojo instead of IPC. It adds a few Mojo interfaces and AudioOuputClient and AudioOutputImpl that implement the Mojo client and service. In future CLs: * Mojo is will be used for all the other audio output stream operations (playing, pausing, ..) * AudioRendererHost and AudioMessageFilter will be removed and the logic inside them will be moved to AudioOuputClient and AudioOutputImpl. BUG=605954 ========== to ========== Switch stream creation and closing in Chrome audio rendering from IPC to Mojo This replaces audio stream creation and closing by using Mojo instead of IPC. It adds a few Mojo interfaces and AudioOuputClient and AudioOutputImpl that implement the Mojo client and service. In future CLs: * Mojo is will be used for all the other audio output stream operations (playing, pausing, ..) * AudioRendererHost and AudioMessageFilter will be removed and the logic inside them will be moved to AudioOuputClient and AudioOutputImpl. BUG=606707 ==========
tommi@chromium.org changed reviewers: + niklase@chromium.org
+Niklas to take a look at mojo parts (or delegate) Some comments below, I'm looking at audio_renderer_host.cc now. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... File content/browser/media/audio_output_impl.cc (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:34: base::Unretained(this))); using Unretained() always raises an alarm. Is it guaranteed that this pointer will never be dereferenced after the object has been deleted? How is that done? (can you add a comment for why this is safe?) https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:43: remove whitespace https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:84: stream_impls_[stream_id] = std::move(stream_ptr); use insert() instead? This will do a lookup+insert. If the stream_id is not expected to exist in the map, you can DCHECK the return value from insert() for correctness. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:85: return stream; this returns a "Ptr*". is that a **? Maybe the "Ptr" part is a bit misleading. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:85: return stream; returning a pointer to an object that's owned by the map, could be risky. How is it safe to return this pointer and be sure that the object is not going to be deleted while in use? https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:91: DLOG(ERROR) << "Mojo client connection error"; LOG? even fatal or dfatal? https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... File content/browser/media/audio_output_impl.h (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:27: mojo::InterfaceRequest<mojom::AudioOutputStream> request, Niklas - can you recommend someone to remove this CL from the mojo perspective? https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:29: AudioRendererHost* audio_renderer_host); document ownership/lifetime of raw pointers https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:32: void Close() override; make this private? (assuming callers of this method will have an interface pointer and not a pointer to this implementation. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:34: ~AudioOutputStreamImpl() override; dtor follows ctor(s) in class layout https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:58: mojom::AudioOutputRequest request); passed by value on purpose? https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:73: remove this empty line https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:77: media::interfaces::AudioOutputStreamParametersPtr params, intentionally by value? https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:85: std::map<int, std::unique_ptr<AudioOutputStreamImpl>> stream_impls_; since there's no lock in this class, can we use a thread checker to ensure that all method calls execute on expected threads? https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:72: base::SyncSocket::TransitDescriptor socket_descriptor_dup; Initialize please. As is, you might be returning a bogus handle on windows. https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:75: ::DuplicateHandle(GetCurrentProcess(), // hSourceProcessHandle check the return value? https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.h:163: int render_frame_id, fix indent
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
I am reviewing from media/mojo perspective. Here's my first round of comments. https://codereview.chromium.org/1896883002/diff/120001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/BUILD.... content/browser/BUILD.gn:65: "//media:media", This is not necessary. //media means //media::media. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... File content/browser/media/audio_output_impl.h (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:26: explicit AudioOutputStreamImpl( no explicit https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:51: AudioOutputImpl(mojom::AudioOutputRequest request); explicit https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:62: // that thread. Could you please explain more why this service has to run on the IO thread? Typically IO thread is very performance sensitive and we want to move stuff off the IO thread. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:65: mojo::InterfaceRequest<mojom::AudioOutput> request); You can use mojom::AudioOutputRequest now I believe. https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:345: std::unique_ptr<mojom::AudioOutputStreamPtr> stream_ptr( AudioOutputStreamPtr is an InterfacePtr which is already move-only. No need for std::unique_ptr here? https://codereview.chromium.org/1896883002/diff/120001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/1896883002/diff/120001/media/BUILD.gn#newcode454 media/BUILD.gn:454: "//mojo/public/cpp/system", See below, "media" should not depend on mojo. https://codereview.chromium.org/1896883002/diff/120001/media/audio/BUILD.gn File media/audio/BUILD.gn (right): https://codereview.chromium.org/1896883002/diff/120001/media/audio/BUILD.gn#n... media/audio/BUILD.gn:131: ] This target will be part of "media" target which should not know about "mojo", similar to the fact that "media" does not know about IPC. Typically mojo/IPC are handled in the content layer and hidden from media. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:5: module content.mojom; This file is in media/mojo where we use media.interfaces as the module. We do have a plan to switch to media.mojom in the future, but for now it'd be nice to be consistent with existing interfaces. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:9: interface AudioOutputStream { Please provide some comments about what this is for. An interface with just a Close() call seems odd. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:13: interface AudioOutput { ditto for comments. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:16: int32 render_frame_id, Hmm, media should know nothing about content (e.g. render_frame_id). If this is really necessary, then this interface should be defined in content instead in media. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:192: struct AudioOutputStreamParameters { Is this the equivalence of media::AudioParameters? If so, we typically keep using the same name for native type and mojo type for better readability. So this should also named AudioParameters. Also, please add a comment about the corresponding native type. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:193: AudioFormat format_; // Format of the stream. We don't use trailing "_" for mojo struct members. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:201: remote empty line.
Thanks a lot for the reviews. I started working on them. (but it's too late, so, I will continue doing that tomorrow)
Thanks a lot for the reviews. I started working on them. (but it's too late, so, I will continue doing that tomorrow)
I took another round at only the media/ part. https://codereview.chromium.org/1896883002/diff/140001/media/mojo/common/medi... File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/1896883002/diff/140001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:443: Remove empty line. https://codereview.chromium.org/1896883002/diff/140001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/140001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:194: // media.interfaces.AudioOutput. CreateStream which doesn't need the vector. "media::interfaces::AudioOutput::CreateStream()" probably makes it a bit clearer. At least remove the space before CreateStream. https://codereview.chromium.org/1896883002/diff/140001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:197: AudioFormat format; // Format of the stream. Alignment of the comments. https://codereview.chromium.org/1896883002/diff/140001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:197: AudioFormat format; // Format of the stream. Actually, I think it's better to remove the comments from here, to avoid redundancy. Above it says "See media/base/media/base/audio_parameters.h for description", which should be enough. In the rest of the file there is only comments in one struct (I think), the other have no comments. https://codereview.chromium.org/1896883002/diff/140001/media/mojo/interfaces/... File media/mojo/interfaces/mojo_bindings.gyp (right): https://codereview.chromium.org/1896883002/diff/140001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:58: 'content_decryption_module.mojom', Hmm, I don't quite understand why all these .mojom files are in this target. Also, the two cases above don't have the .h/cc sources in the bindings, which is the case here. I defer to mojo experts and media owners to review this.
This cl is going to be split: The media part of it will stay here. And, the content part of it will be move to another cl. https://codereview.chromium.org/1896883002/diff/120001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/BUILD.... content/browser/BUILD.gn:65: "//media:media", On 2016/04/26 22:54:41, xhwang wrote: > This is not necessary. //media means //media::media. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... File content/browser/media/audio_output_impl.cc (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:34: base::Unretained(this))); I will not use AudioOutputImpl::OnDisconnect. So I will remove it. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:43: On 2016/04/26 15:29:52, tommi-chrömium wrote: > remove whitespace Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:84: stream_impls_[stream_id] = std::move(stream_ptr); On 2016/04/26 15:29:52, tommi-chrömium wrote: > use insert() instead? This will do a lookup+insert. If the stream_id is not > expected to exist in the map, you can DCHECK the return value from insert() for > correctness. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:85: return stream; On 2016/04/26 15:29:52, tommi-chrömium wrote: > this returns a "Ptr*". is that a **? Maybe the "Ptr" part is a bit misleading. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:85: return stream; On 2016/04/26 15:29:52, tommi-chrömium wrote: > returning a pointer to an object that's owned by the map, could be risky. How > is it safe to return this pointer and be sure that the object is not going to be > deleted while in use? Because the map is changed in this function only. The map is going to be deleted only when the AudioOutputImpl is destructed (which could happens when we have a connection error or when the current message loop is destructed) The mojom::AudioOutputStreamPtr is going to be only iuused to call the callback and if in this Later in hte rendrer when the AudioOutputStreamPtr is used to perform operations on AudioOutputStream, https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:91: DLOG(ERROR) << "Mojo client connection error"; On 2016/04/26 15:29:52, tommi-chrömium wrote: > LOG? even fatal or dfatal? This could be caused by an error in mojo connection. But most of the time it's just going to be called when the current message loop is destructed. But as we don't need it (I used just for debugging), I will remove it. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... File content/browser/media/audio_output_impl.h (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:24: // All the instance of this class are going to be owned by AudioOutputImpl. On 2016/04/22 08:23:03, Henrik Grunell wrote: > Put comment before the "class" line. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:24: // All the instance of this class are going to be owned by AudioOutputImpl. On 2016/04/22 08:23:03, Henrik Grunell wrote: > Put comment before the "class" line. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:26: explicit AudioOutputStreamImpl( On 2016/04/26 22:54:42, xhwang wrote: > no explicit Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:29: AudioRendererHost* audio_renderer_host); On 2016/04/26 15:29:53, tommi-chrömium wrote: > document ownership/lifetime of raw pointers I have done that while describing |audio_renderer_host_| https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:32: void Close() override; On 2016/04/26 15:29:53, tommi-chrömium wrote: > make this private? (assuming callers of this method will have an interface > pointer and not a pointer to this implementation. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:34: ~AudioOutputStreamImpl() override; On 2016/04/26 15:29:52, tommi-chrömium wrote: > dtor follows ctor(s) in class layout Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:39: // |audio_renderer_host_| is scoped_refptr in AudioOutputImpl. And because all On 2016/04/22 08:23:03, Henrik Grunell wrote: > You could put it as: > "AudioRendererHost is ref counted and AudioOutputImpl holds a reference to it." Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:40: // AudioOutputStreamImpl instances are going to be owned by AudioOutputImpl On 2016/04/22 08:23:03, Henrik Grunell wrote: > "going to be" means that it will happen in the future. > > Maybe something like "AudioOutputStreamImpl is owned by AudioOutputImpl". Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:51: AudioOutputImpl(mojom::AudioOutputRequest request); On 2016/04/26 22:54:42, xhwang wrote: > explicit Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:58: mojom::AudioOutputRequest request); On 2016/04/26 15:29:52, tommi-chrömium wrote: > passed by value on purpose? yes it's a mojo::InterfaceRequest<mojom::AudioOutput> which needs to be handle as if it's a smart pointer. If I start using references, the compiler will give me a lot of error. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:60: // Initialize Mojo on the BrowserThread::IO. Mojo is going to be bound to On 2016/04/22 08:23:03, Henrik Grunell wrote: > Does it create or only initialize? Both the comment and function name should > reflect what it does. > > "Mojo" -> "Mojo service" > > "BrowserThread::IO" -> "IO thread" > > "Mojo is going to be bound" -> "The service is bound" > > Rephrase/clarify "future Mojo calls". (What is a "mojo call"?) Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:62: // that thread. On 2016/04/26 22:54:41, xhwang wrote: > Could you please explain more why this service has to run on the IO thread? > Typically IO thread is very performance sensitive and we want to move stuff off > the IO thread. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:65: mojo::InterfaceRequest<mojom::AudioOutput> request); On 2016/04/26 22:54:42, xhwang wrote: > You can use mojom::AudioOutputRequest now I believe. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:73: On 2016/04/26 15:29:52, tommi-chrömium wrote: > remove this empty line Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:77: media::interfaces::AudioOutputStreamParametersPtr params, On 2016/04/26 15:29:52, tommi-chrömium wrote: > intentionally by value? AudioOutputStreamParametersPtr is just a smart pointer: InterfacePtr https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi... This method is going to be called by the mojo client and this is how it should be called. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:85: std::map<int, std::unique_ptr<AudioOutputStreamImpl>> stream_impls_; On 2016/04/26 15:29:53, tommi-chrömium wrote: > since there's no lock in this class, can we use a thread checker to ensure that > all method calls execute on expected threads? Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:72: base::SyncSocket::TransitDescriptor socket_descriptor_dup; On 2016/04/26 15:29:53, tommi-chrömium wrote: > Initialize please. As is, you might be returning a bogus handle on windows. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:75: ::DuplicateHandle(GetCurrentProcess(), // hSourceProcessHandle On 2016/04/26 15:29:53, tommi-chrömium wrote: > check the return value? Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:345: std::unique_ptr<mojom::AudioOutputStreamPtr> stream_ptr( On 2016/04/26 22:54:42, xhwang wrote: > AudioOutputStreamPtr is an InterfacePtr which is already move-only. No need for > std::unique_ptr here? Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:381: // The socket handle is going to be closed when |mojo_application_host_| is On 2016/04/22 08:23:04, Henrik Grunell wrote: >Only one is used in the end, right? yes. > Can it be passed instead? if we dont do that, both mojo and AudioOutputIPCDelegate are going to need to close the socket descriptor which causes an error. https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.h:163: int render_frame_id, On 2016/04/26 15:29:53, tommi-chrömium wrote: > fix indent Done. https://codereview.chromium.org/1896883002/diff/120001/content/renderer/media... File content/renderer/media/audio_message_filter.cc (right): https://codereview.chromium.org/1896883002/diff/120001/content/renderer/media... content/renderer/media/audio_message_filter.cc:110: // The plan is to try to completely remove AudioMessageFilter and move all On 2016/04/22 08:23:04, Henrik Grunell wrote: > You could put it as: > "This is temporary. In soon upcoming CLs AudioMessageFilter and > AudioMessageFilter::AudioOutputIPCImpl will be removed and replaced by > AudioOutputClient. Done. https://codereview.chromium.org/1896883002/diff/120001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/1896883002/diff/120001/media/BUILD.gn#newcode454 media/BUILD.gn:454: "//mojo/public/cpp/system", On 2016/04/26 22:54:42, xhwang wrote: > See below, "media" should not depend on mojo. Done. https://codereview.chromium.org/1896883002/diff/120001/media/audio/BUILD.gn File media/audio/BUILD.gn (right): https://codereview.chromium.org/1896883002/diff/120001/media/audio/BUILD.gn#n... media/audio/BUILD.gn:131: ] On 2016/04/26 22:54:42, xhwang wrote: > This target will be part of "media" target which should not know about "mojo", > similar to the fact that "media" does not know about IPC. Typically mojo/IPC are > handled in the content layer and hidden from media. Done. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:5: module content.mojom; On 2016/04/26 22:54:42, xhwang wrote: > This file is in media/mojo where we use media.interfaces as the module. We do > have a plan to switch to media.mojom in the future, but for now it'd be nice to > be consistent with existing interfaces. Done. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:9: interface AudioOutputStream { On 2016/04/26 22:54:42, xhwang wrote: > Please provide some comments about what this is for. An interface with just a > Close() call seems odd. Done. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:13: interface AudioOutput { On 2016/04/26 22:54:42, xhwang wrote: > ditto for comments. Done. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:16: int32 render_frame_id, On 2016/04/26 22:54:42, xhwang wrote: > Hmm, media should know nothing about content (e.g. render_frame_id). If this is > really necessary, then this interface should be defined in content instead in > media. At the beginning, I tried to define the interface in content, but I failed and I got a presubmit error: as the interface is going to be used in audio_output_controller.h which is in media and we cannot include things defined in content in media. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:192: struct AudioOutputStreamParameters { On 2016/04/26 22:54:42, xhwang wrote: > Is this the equivalence of media::AudioParameters? If so, we typically keep > using the same name for native type and mojo type for better readability. So > this should also named AudioParameters. Also, please add a comment about the > corresponding native type. This defines a mojo transport format for media::AudioParameters without the |mic_positions_| vector. This will be used by media.interfaces.AudioOutput. CreateStream which doesn't need the vector. A different name was used to avoid confusion. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:193: AudioFormat format_; // Format of the stream. On 2016/04/26 22:54:42, xhwang wrote: > We don't use trailing "_" for mojo struct members. Done. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:201: On 2016/04/26 22:54:42, xhwang wrote: > remote empty line. Done.
This cl is going to be split: The media part of it will stay here. And, the content part of it will be move to another cl. https://codereview.chromium.org/1896883002/diff/120001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/BUILD.... content/browser/BUILD.gn:65: "//media:media", On 2016/04/26 22:54:41, xhwang wrote: > This is not necessary. //media means //media::media. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... File content/browser/media/audio_output_impl.cc (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:34: base::Unretained(this))); I will not use AudioOutputImpl::OnDisconnect. So I will remove it. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:43: On 2016/04/26 15:29:52, tommi-chrömium wrote: > remove whitespace Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:84: stream_impls_[stream_id] = std::move(stream_ptr); On 2016/04/26 15:29:52, tommi-chrömium wrote: > use insert() instead? This will do a lookup+insert. If the stream_id is not > expected to exist in the map, you can DCHECK the return value from insert() for > correctness. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:85: return stream; On 2016/04/26 15:29:52, tommi-chrömium wrote: > this returns a "Ptr*". is that a **? Maybe the "Ptr" part is a bit misleading. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:85: return stream; On 2016/04/26 15:29:52, tommi-chrömium wrote: > returning a pointer to an object that's owned by the map, could be risky. How > is it safe to return this pointer and be sure that the object is not going to be > deleted while in use? Because the map is changed in this function only. The map is going to be deleted only when the AudioOutputImpl is destructed (which could happens when we have a connection error or when the current message loop is destructed) The mojom::AudioOutputStreamPtr is going to be only iuused to call the callback and if in this Later in hte rendrer when the AudioOutputStreamPtr is used to perform operations on AudioOutputStream, https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.cc:91: DLOG(ERROR) << "Mojo client connection error"; On 2016/04/26 15:29:52, tommi-chrömium wrote: > LOG? even fatal or dfatal? This could be caused by an error in mojo connection. But most of the time it's just going to be called when the current message loop is destructed. But as we don't need it (I used just for debugging), I will remove it. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... File content/browser/media/audio_output_impl.h (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:24: // All the instance of this class are going to be owned by AudioOutputImpl. On 2016/04/22 08:23:03, Henrik Grunell wrote: > Put comment before the "class" line. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:24: // All the instance of this class are going to be owned by AudioOutputImpl. On 2016/04/22 08:23:03, Henrik Grunell wrote: > Put comment before the "class" line. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:26: explicit AudioOutputStreamImpl( On 2016/04/26 22:54:42, xhwang wrote: > no explicit Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:29: AudioRendererHost* audio_renderer_host); On 2016/04/26 15:29:53, tommi-chrömium wrote: > document ownership/lifetime of raw pointers I have done that while describing |audio_renderer_host_| https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:32: void Close() override; On 2016/04/26 15:29:53, tommi-chrömium wrote: > make this private? (assuming callers of this method will have an interface > pointer and not a pointer to this implementation. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:34: ~AudioOutputStreamImpl() override; On 2016/04/26 15:29:52, tommi-chrömium wrote: > dtor follows ctor(s) in class layout Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:39: // |audio_renderer_host_| is scoped_refptr in AudioOutputImpl. And because all On 2016/04/22 08:23:03, Henrik Grunell wrote: > You could put it as: > "AudioRendererHost is ref counted and AudioOutputImpl holds a reference to it." Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:40: // AudioOutputStreamImpl instances are going to be owned by AudioOutputImpl On 2016/04/22 08:23:03, Henrik Grunell wrote: > "going to be" means that it will happen in the future. > > Maybe something like "AudioOutputStreamImpl is owned by AudioOutputImpl". Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:51: AudioOutputImpl(mojom::AudioOutputRequest request); On 2016/04/26 22:54:42, xhwang wrote: > explicit Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:58: mojom::AudioOutputRequest request); On 2016/04/26 15:29:52, tommi-chrömium wrote: > passed by value on purpose? yes it's a mojo::InterfaceRequest<mojom::AudioOutput> which needs to be handle as if it's a smart pointer. If I start using references, the compiler will give me a lot of error. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:60: // Initialize Mojo on the BrowserThread::IO. Mojo is going to be bound to On 2016/04/22 08:23:03, Henrik Grunell wrote: > Does it create or only initialize? Both the comment and function name should > reflect what it does. > > "Mojo" -> "Mojo service" > > "BrowserThread::IO" -> "IO thread" > > "Mojo is going to be bound" -> "The service is bound" > > Rephrase/clarify "future Mojo calls". (What is a "mojo call"?) Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:62: // that thread. On 2016/04/26 22:54:41, xhwang wrote: > Could you please explain more why this service has to run on the IO thread? > Typically IO thread is very performance sensitive and we want to move stuff off > the IO thread. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:65: mojo::InterfaceRequest<mojom::AudioOutput> request); On 2016/04/26 22:54:42, xhwang wrote: > You can use mojom::AudioOutputRequest now I believe. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:73: On 2016/04/26 15:29:52, tommi-chrömium wrote: > remove this empty line Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:77: media::interfaces::AudioOutputStreamParametersPtr params, On 2016/04/26 15:29:52, tommi-chrömium wrote: > intentionally by value? AudioOutputStreamParametersPtr is just a smart pointer: InterfacePtr https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi... This method is going to be called by the mojo client and this is how it should be called. https://codereview.chromium.org/1896883002/diff/120001/content/browser/media/... content/browser/media/audio_output_impl.h:85: std::map<int, std::unique_ptr<AudioOutputStreamImpl>> stream_impls_; On 2016/04/26 15:29:53, tommi-chrömium wrote: > since there's no lock in this class, can we use a thread checker to ensure that > all method calls execute on expected threads? Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:72: base::SyncSocket::TransitDescriptor socket_descriptor_dup; On 2016/04/26 15:29:53, tommi-chrömium wrote: > Initialize please. As is, you might be returning a bogus handle on windows. Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:75: ::DuplicateHandle(GetCurrentProcess(), // hSourceProcessHandle On 2016/04/26 15:29:53, tommi-chrömium wrote: > check the return value? Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:345: std::unique_ptr<mojom::AudioOutputStreamPtr> stream_ptr( On 2016/04/26 22:54:42, xhwang wrote: > AudioOutputStreamPtr is an InterfacePtr which is already move-only. No need for > std::unique_ptr here? Done. https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.cc:381: // The socket handle is going to be closed when |mojo_application_host_| is On 2016/04/22 08:23:04, Henrik Grunell wrote: >Only one is used in the end, right? yes. > Can it be passed instead? if we dont do that, both mojo and AudioOutputIPCDelegate are going to need to close the socket descriptor which causes an error. https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/1896883002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_renderer_host.h:163: int render_frame_id, On 2016/04/26 15:29:53, tommi-chrömium wrote: > fix indent Done. https://codereview.chromium.org/1896883002/diff/120001/content/renderer/media... File content/renderer/media/audio_message_filter.cc (right): https://codereview.chromium.org/1896883002/diff/120001/content/renderer/media... content/renderer/media/audio_message_filter.cc:110: // The plan is to try to completely remove AudioMessageFilter and move all On 2016/04/22 08:23:04, Henrik Grunell wrote: > You could put it as: > "This is temporary. In soon upcoming CLs AudioMessageFilter and > AudioMessageFilter::AudioOutputIPCImpl will be removed and replaced by > AudioOutputClient. Done. https://codereview.chromium.org/1896883002/diff/120001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/1896883002/diff/120001/media/BUILD.gn#newcode454 media/BUILD.gn:454: "//mojo/public/cpp/system", On 2016/04/26 22:54:42, xhwang wrote: > See below, "media" should not depend on mojo. Done. https://codereview.chromium.org/1896883002/diff/120001/media/audio/BUILD.gn File media/audio/BUILD.gn (right): https://codereview.chromium.org/1896883002/diff/120001/media/audio/BUILD.gn#n... media/audio/BUILD.gn:131: ] On 2016/04/26 22:54:42, xhwang wrote: > This target will be part of "media" target which should not know about "mojo", > similar to the fact that "media" does not know about IPC. Typically mojo/IPC are > handled in the content layer and hidden from media. Done. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:5: module content.mojom; On 2016/04/26 22:54:42, xhwang wrote: > This file is in media/mojo where we use media.interfaces as the module. We do > have a plan to switch to media.mojom in the future, but for now it'd be nice to > be consistent with existing interfaces. Done. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:9: interface AudioOutputStream { On 2016/04/26 22:54:42, xhwang wrote: > Please provide some comments about what this is for. An interface with just a > Close() call seems odd. Done. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:13: interface AudioOutput { On 2016/04/26 22:54:42, xhwang wrote: > ditto for comments. Done. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:16: int32 render_frame_id, On 2016/04/26 22:54:42, xhwang wrote: > Hmm, media should know nothing about content (e.g. render_frame_id). If this is > really necessary, then this interface should be defined in content instead in > media. At the beginning, I tried to define the interface in content, but I failed and I got a presubmit error: as the interface is going to be used in audio_output_controller.h which is in media and we cannot include things defined in content in media. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:192: struct AudioOutputStreamParameters { On 2016/04/26 22:54:42, xhwang wrote: > Is this the equivalence of media::AudioParameters? If so, we typically keep > using the same name for native type and mojo type for better readability. So > this should also named AudioParameters. Also, please add a comment about the > corresponding native type. This defines a mojo transport format for media::AudioParameters without the |mic_positions_| vector. This will be used by media.interfaces.AudioOutput. CreateStream which doesn't need the vector. A different name was used to avoid confusion. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:193: AudioFormat format_; // Format of the stream. On 2016/04/26 22:54:42, xhwang wrote: > We don't use trailing "_" for mojo struct members. Done. https://codereview.chromium.org/1896883002/diff/120001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:201: On 2016/04/26 22:54:42, xhwang wrote: > remote empty line. Done.
Description was changed from ========== Switch stream creation and closing in Chrome audio rendering from IPC to Mojo This replaces audio stream creation and closing by using Mojo instead of IPC. It adds a few Mojo interfaces and AudioOuputClient and AudioOutputImpl that implement the Mojo client and service. In future CLs: * Mojo is will be used for all the other audio output stream operations (playing, pausing, ..) * AudioRendererHost and AudioMessageFilter will be removed and the logic inside them will be moved to AudioOuputClient and AudioOutputImpl. BUG=606707 ========== to ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * AudioOutputStreamParameters Mojo structure used by AudioOuput. * AudioOutputStreamParameters/AudioParameters converters needed later. This is the media part: The content part is in the CL 1930393002. BUG=606707 ==========
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
Some comments. Much better, now that is splitted. Suggestion: after uploading a patch, run `git cl try` to run bots. (It might not work if you don't have try bot access, grunell@ can ask for it). https://codereview.chromium.org/1896883002/diff/200001/media/mojo/common/medi... File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:404: output.set_channels(input->channels); I'm not 100% sure of this but, couldn't we use media::GuessChannelLayout(number_of_channels) in the ctor in l.400 and do away with this new accessor? Perhaps annotated via DCHECK_EQ(input->channel_layout, media::GuessChannelLayout(number_of_channels)); https://codereview.chromium.org/1896883002/diff/200001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:443: Remove added empty line https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:5: module media.interfaces; It's customary to call these modules module media.mojom; as e.g. content.mojom, blink.mojom, etc I understand this is larger than this CL, could you please make a crbug.com for it and add xhwang@? Thanks. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:204: int32 effects; // Bitmask using PlatformEffectsMask. You shouldn't align these comments, instead, leave two blanks after the ; and start the //
https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:5: module media.interfaces; On 2016/04/29 17:14:11, mcasas wrote: > It's customary to call these modules > > module media.mojom; > > as e.g. content.mojom, blink.mojom, etc > I understand this is larger than this CL, > could you please make a http://crbug.com for it > and add xhwang@? Thanks. We already have a bug for this: https://bugs.chromium.org/p/chromium/issues/detail?id=597968
Thanks for splitting the CL! Here are some more comments about media/mojo. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:5: module media.interfaces; On 2016/04/29 17:16:24, xhwang wrote: > On 2016/04/29 17:14:11, mcasas wrote: > > It's customary to call these modules > > > > module media.mojom; > > > > as e.g. content.mojom, blink.mojom, etc > > I understand this is larger than this CL, > > could you please make a http://crbug.com for it > > and add xhwang@? Thanks. > > We already have a bug for this: > https://bugs.chromium.org/p/chromium/issues/detail?id=597968 Actually feel free to use media.mojom for new interfaces. I'll fix the old ones at some point. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:11: // AudioOutputStreamImpl in the browser. It's still odd that you create this stream, doing nothing on it, and then will Close() it. Are we planning to add more methods on this interface? https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:11: // AudioOutputStreamImpl in the browser. Please document what this interface does instead of where it'll be implemented/used. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:18: // AudioOutputImpl in the browser. Please document what this interface does instead of where it'll be implemented/used. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:22: int32 render_frame_id, This still looks wrong; media should not know about render frames. I noticed that in the implementation, you have one AudioRendererHost and one AudioOutputImpl per render process. Does it make sense to keep one AudioRendererHost per render process, but have one AudioOutputImpl per render frame? Basically AudioOutputImpl should be registered at the RenderFrame level instead of RenderProcess level: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... Then since we are already at the RenderFrame level, AudioOutputImpl should already know about the |render_frame_id|, and we don't need to pass it in this interface. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:195: // See media/base/media/base/audio_parameters.h for description. Drop extra media/base/ https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:196: struct AudioOutputStreamParameters { It seems to me a different name actually caused more confusion :) How about we keep the AudioParameters name, add a TODO after line 204 to add |mic_positions_| if needed. Then in the type converters CHECK/DCHECK that the |mic_positions_| must be empty? https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/mojo_bindings.gyp (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:62: 'media_types.mojom', drop duplicate lines https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:64: 'renderer.mojom', Most of these are never intended to work in GYP builds (also GYP is going away very very soon). Only include what you need here. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:82: '../common/mojo_shared_buffer_video_frame.h', why do you need video frame here?
https://codereview.chromium.org/1896883002/diff/200001/media/audio/audio_para... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/1896883002/diff/200001/media/audio/audio_para... media/audio/audio_parameters.h:154: void set_channels(int channels) { channels_ = channels; } Don't add this, instead add a set_channel_layout() and only use set_channels_for_discrete().
The CQ bit was checked by tommi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1896883002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1896883002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/04/29 19:42:35, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) > ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) > mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) Thanks guys for the reviews Could you please have another look to the cl?
@mcasas: good suggestion, I will do that, but first, I will need to rebase. https://codereview.chromium.org/1896883002/diff/140001/media/mojo/common/medi... File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/1896883002/diff/140001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:443: On 2016/04/28 11:44:55, Henrik Grunell wrote: > Remove empty line. Done. https://codereview.chromium.org/1896883002/diff/140001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/140001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:194: // media.interfaces.AudioOutput. CreateStream which doesn't need the vector. On 2016/04/28 11:44:56, Henrik Grunell wrote: > "media::interfaces::AudioOutput::CreateStream()" probably makes it a bit > clearer. At least remove the space before CreateStream. Done. https://codereview.chromium.org/1896883002/diff/140001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:197: AudioFormat format; // Format of the stream. On 2016/04/28 11:44:56, Henrik Grunell wrote: > Alignment of the comments. Done. https://codereview.chromium.org/1896883002/diff/140001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:197: AudioFormat format; // Format of the stream. On 2016/04/28 11:44:56, Henrik Grunell wrote: > Actually, I think it's better to remove the comments from here, to avoid > redundancy. Above it says "See media/base/media/base/audio_parameters.h for > description", which should be enough. In the rest of the file there is only > comments in one struct (I think), the other have no comments. Done. https://codereview.chromium.org/1896883002/diff/140001/media/mojo/interfaces/... File media/mojo/interfaces/mojo_bindings.gyp (right): https://codereview.chromium.org/1896883002/diff/140001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:58: 'content_decryption_module.mojom', On 2016/04/28 11:44:56, Henrik Grunell wrote: > Hmm, I don't quite understand why all these .mojom files are in this target. > > Also, the two cases above don't have the .h/cc sources in the bindings, which is > the case here. > > I defer to mojo experts and media owners to review this. Some of these files are used in media_type_converters.h and media_types.mojom. If I don't include them, I will get a link error. (I tried to do that but gyp failed. (I removed the ones that are not needed) The .h/.cc sources are in audio_output_mojom_bindings https://codereview.chromium.org/1896883002/diff/200001/media/audio/audio_para... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/1896883002/diff/200001/media/audio/audio_para... media/audio/audio_parameters.h:154: void set_channels(int channels) { channels_ = channels; } On 2016/04/29 17:48:04, DaleCurtis wrote: > Don't add this, instead add a set_channel_layout() and only use > set_channels_for_discrete() I replaced set_channels by set_channels_for_discrete and removed set_channels. However, for the set_channel_layout(), we don't have to add it as we can just set channel_layout in the constructor. Done. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/common/medi... File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:404: output.set_channels(input->channels); On 2016/04/29 17:14:11, mcasas wrote: > I'm not 100% sure of this but, couldn't we use > media::GuessChannelLayout(number_of_channels) > in the ctor in l.400 and do away with this new > accessor? Perhaps annotated via I replaced set_channels with set_channels_for_discrete : Setting the number of channels explicitly is only required with CHANNEL_LAYOUT_DISCRETE https://codereview.chromium.org/1896883002/diff/200001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:443: On 2016/04/29 17:14:11, mcasas wrote: > Remove added empty line Done. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:5: module media.interfaces; On 2016/04/29 17:14:11, mcasas wrote: > It's customary to call these modules > > module media.mojom; > > as e.g. content.mojom, blink.mojom, etc > I understand this is larger than this CL, > could you please make a http://crbug.com for it > and add xhwang@? Thanks. Done. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:5: module media.interfaces; On 2016/04/29 17:16:24, xhwang wrote: > On 2016/04/29 17:14:11, mcasas wrote: > > It's customary to call these modules > > > > module media.mojom; > > > > as e.g. content.mojom, blink.mojom, etc > > I understand this is larger than this CL, > > could you please make a http://crbug.com for it > > and add xhwang@? Thanks. > > We already have a bug for this: > https://bugs.chromium.org/p/chromium/issues/detail?id=597968 Done. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:5: module media.interfaces; On 2016/04/29 17:47:56, xhwang wrote: > On 2016/04/29 17:16:24, xhwang wrote: > > On 2016/04/29 17:14:11, mcasas wrote: > > > It's customary to call these modules > > > > > > module media.mojom; > > > > > > as e.g. content.mojom, blink.mojom, etc > > > I understand this is larger than this CL, > > > could you please make a http://crbug.com for it > > > and add xhwang@? Thanks. > > > > We already have a bug for this: > > https://bugs.chromium.org/p/chromium/issues/detail?id=597968 > > Actually feel free to use media.mojom for new interfaces. I'll fix the old ones > at some point. Done. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:11: // AudioOutputStreamImpl in the browser. On 2016/04/29 17:47:56, xhwang wrote: > It's still odd that you create this stream, doing nothing on it, and then will > Close() it. Are we planning to add more methods on this interface? yes, we are going to add play, pause, setvolume https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:11: // AudioOutputStreamImpl in the browser. On 2016/04/29 17:47:56, xhwang wrote: > It's still odd that you create this stream, doing nothing on it, and then will > Close() it. Are we planning to add more methods on this interface? Done. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:18: // AudioOutputImpl in the browser. On 2016/04/29 17:47:56, xhwang wrote: > Please document what this interface does instead of where it'll be > implemented/used. Done. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:22: int32 render_frame_id, On 2016/04/29 17:47:56, xhwang wrote: > This still looks wrong; media should not know about render frames. I noticed > that in the implementation, you have one AudioRendererHost and one > AudioOutputImpl per render process. Does it make sense to keep one > AudioRendererHost per render process, but have one AudioOutputImpl per render > frame? > > Basically AudioOutputImpl should be registered at the RenderFrame level instead > of RenderProcess level: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > Then since we are already at the RenderFrame level, AudioOutputImpl should > already know about the |render_frame_id|, and we don't need to pass it in this > interface. Done. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:195: // See media/base/media/base/audio_parameters.h for description. On 2016/04/29 17:47:57, xhwang wrote: > Drop extra media/base/ Done. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:196: struct AudioOutputStreamParameters { On 2016/04/29 17:47:57, xhwang wrote: > It seems to me a different name actually caused more confusion :) > > How about we keep the AudioParameters name, add a TODO after line 204 to add > |mic_positions_| if needed. Then in the type converters CHECK/DCHECK that the > |mic_positions_| must be empty? Done. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:204: int32 effects; // Bitmask using PlatformEffectsMask. On 2016/04/29 17:14:11, mcasas wrote: > You shouldn't align these comments, instead, > leave two blanks after the ; and start the // grunnel told me that: Actually, I think it's better to remove the comments from here, to avoid redundancy. Above it says "See media/base/media/base/audio_parameters.h for description", which should be enough. In the rest of the file there is only comments in one struct (I think), the other have no comments. So, i removed them. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/mojo_bindings.gyp (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:62: 'media_types.mojom', On 2016/04/29 17:47:57, xhwang wrote: > drop duplicate lines Done. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:64: 'renderer.mojom', On 2016/04/29 17:47:57, xhwang wrote: > Most of these are never intended to work in GYP builds (also GYP is going away > very very soon). Only include what you need here. Some of these files are used in media_type_converters.h and media_types.mojom. If I don't include them, I will get a link error. (I tried to do that but gyp failed. (I removed the ones that are not needed) But it doesn't matter a lot, as gyp is going to be disabled soon. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:82: '../common/mojo_shared_buffer_video_frame.h', On 2016/04/29 17:47:57, xhwang wrote: > why do you need video frame here? MojoSharedBufferVideoFrame is used in media_type_converters.cc. If I don't include it, I will get a link error form gyp. (I tried to do that but gyp failed. But it doesn't matter a lot, as gyp is going to be disabled soon.
Description was changed from ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * AudioOutputStreamParameters Mojo structure used by AudioOuput. * AudioOutputStreamParameters/AudioParameters converters needed later. This is the media part: The content part is in the CL 1930393002. BUG=606707 ========== to ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * AudioOutputStreamParameters Mojo structure used by AudioOuput. * AudioOutputStreamParameters/AudioParameters converters needed later. This is the media part: The content part is in the CL https://codereview.chromium.org/1930393002/ BUG=606707 ==========
Description was changed from ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * AudioOutputStreamParameters Mojo structure used by AudioOuput. * AudioOutputStreamParameters/AudioParameters converters needed later. This is the media part: The content part is in the CL https://codereview.chromium.org/1930393002/ BUG=606707 ========== to ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * AudioOutputStreamParameters Mojo structure used by AudioOuput. * AudioOutputStreamParameters/AudioParameters converters needed later. This is the media part: The content part is in the CL codereview.chromium.org/1930393002 BUG=606707 ==========
Description was changed from ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * AudioOutputStreamParameters Mojo structure used by AudioOuput. * AudioOutputStreamParameters/AudioParameters converters needed later. This is the media part: The content part is in the CL codereview.chromium.org/1930393002 BUG=606707 ========== to ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * AudioOutputStreamParameters Mojo structure used by AudioOuput. * AudioOutputStreamParameters/AudioParameters converters needed later. This is the media part: The content part is in the CL http://codereview.chromium.org/1930393002 BUG=606707 ==========
Looking much better now. Just a few more comments/nits. https://chromiumcodereview.appspot.com/1896883002/diff/200001/media/mojo/inte... File media/mojo/interfaces/mojo_bindings.gyp (right): https://chromiumcodereview.appspot.com/1896883002/diff/200001/media/mojo/inte... media/mojo/interfaces/mojo_bindings.gyp:64: 'renderer.mojom', On 2016/05/02 15:54:11, rchtara wrote: > On 2016/04/29 17:47:57, xhwang wrote: > > Most of these are never intended to work in GYP builds (also GYP is going away > > very very soon). Only include what you need here. > Some of these files are used in media_type_converters.h and media_types.mojom. > If I don't include them, I will get a link error. (I tried to do that but gyp > failed. (I removed the ones that are not needed) > But it doesn't matter a lot, as gyp is going to be disabled soon. sigh... Then should these be part of a target that includes media_type_converters.*? https://chromiumcodereview.appspot.com/1896883002/diff/260001/media/mojo/comm... File media/mojo/common/media_type_converters.cc (right): https://chromiumcodereview.appspot.com/1896883002/diff/260001/media/mojo/comm... media/mojo/common/media_type_converters.cc:11: #include "base/numerics/safe_conversions.h" Add include for AudioParameters? https://chromiumcodereview.appspot.com/1896883002/diff/260001/media/mojo/comm... File media/mojo/common/media_type_converters.h (right): https://chromiumcodereview.appspot.com/1896883002/diff/260001/media/mojo/comm... media/mojo/common/media_type_converters.h:34: media::AudioParameters> { Do you still need to forward declare media::AudioParameters? https://chromiumcodereview.appspot.com/1896883002/diff/260001/media/mojo/inte... File media/mojo/interfaces/mojo_bindings.gyp (right): https://chromiumcodereview.appspot.com/1896883002/diff/260001/media/mojo/inte... media/mojo/interfaces/mojo_bindings.gyp:68: 'target_name': 'audio_output_mojom_bindings', The naming of the target doesn't really match what's inlcuded in "sources".
rchtara@chromium.org changed reviewers: + mkwst@chromium.org
Hello mike, Could you please have a look to this cl? Thanks a lot Riadh
I have resolved the remaning issues. Could you guys please have another look to it? Riadh https://codereview.chromium.org/1896883002/diff/260001/media/mojo/common/medi... File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/1896883002/diff/260001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:11: #include "base/numerics/safe_conversions.h" On 2016/05/03 00:14:33, xhwang wrote: > Add include for AudioParameters? Done. https://codereview.chromium.org/1896883002/diff/260001/media/mojo/common/medi... File media/mojo/common/media_type_converters.h (right): https://codereview.chromium.org/1896883002/diff/260001/media/mojo/common/medi... media/mojo/common/media_type_converters.h:34: media::AudioParameters> { On 2016/05/03 00:14:33, xhwang wrote: > Do you still need to forward declare media::AudioParameters? yes, i just have forget to include that after the rebase. And, I had a build error because of that. I will add it back in the next patch https://codereview.chromium.org/1896883002/diff/260001/media/mojo/interfaces/... File media/mojo/interfaces/mojo_bindings.gyp (right): https://codereview.chromium.org/1896883002/diff/260001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:68: 'target_name': 'audio_output_mojom_bindings', On 2016/05/03 00:14:34, xhwang wrote: > The naming of the target doesn't really match what's inlcuded in "sources". yes, but I will need it for the other cl. https://codereview.chromium.org/1930393002/patch/1/10008 I would like to get rid of all the media staff in this cl.
rchtara@chromium.org changed reviewers: + tsepez@chromium.org
On 2016/05/03 13:46:55, rchtara wrote: Hello tsepez, Could you please have a look to this cl? Thanks a lot Riadh
On 2016/05/03 13:46:55, rchtara wrote: Hello tsepez, Could you please have a look to this cl? Thanks a lot Riadh
https://chromiumcodereview.appspot.com/1896883002/diff/320001/media/mojo/inte... File media/mojo/interfaces/audio_output.mojom (right): https://chromiumcodereview.appspot.com/1896883002/diff/320001/media/mojo/inte... media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, Media should not know about render frames (which is in content/ layer). I chat with rchtara@ about this offline. Basically we have two options here: 1. Move this mojom file to content/ layer so that we can use |render_frame_id|. 2. As I suggested earlier, we can have AudioOutputImpl on the Render frame layer (hosted in RenderFrameHostImpl). Since we are already at the RenderFrame level, AudioOutputImpl should already know about the |render_frame_id|, and we don't need to pass it in this interface. Then AudioOutputImpl can delegate the real tasks to the AudioRendererHost hosted in RenderProcessHostImpl. Currently we only have a getter for AudioRendererHost on RenderProcessHostImpl, not RenderProcessHost. We should probably just add a method on RenderProcessHost to get the AudioRendererHost to get this working. In general I feel passing |render_frame_id| around in IPC is bad. If it's render frame specific, it should be handled in render frame level. So I prefer the second option. But since I am not familiar with audio code, I'll defer to other owners to help make the decision. +dalecurtis / grunell / tommi: WDYT?
It's a bit more overhead on render side perhaps (not sure) to do this on the frame level, but apart from that I don't have a strong opinion about process vs frame level. Dale? Den 3 maj 2016 7:26 em skrev <xhwang@chromium.org>: > > > https://chromiumcodereview.appspot.com/1896883002/diff/320001/media/mojo/inte... > File media/mojo/interfaces/audio_output.mojom (right): > > > https://chromiumcodereview.appspot.com/1896883002/diff/320001/media/mojo/inte... > media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, > Media should not know about render frames (which is in content/ layer). > I chat with rchtara@ about this offline. Basically we have two options > here: > > 1. Move this mojom file to content/ layer so that we can use > |render_frame_id|. > > 2. As I suggested earlier, we can have AudioOutputImpl on the Render > frame layer (hosted in RenderFrameHostImpl). Since we are already at the > RenderFrame level, AudioOutputImpl should > already know about the |render_frame_id|, and we don't need to pass it > in this interface. Then AudioOutputImpl can delegate the real tasks to > the AudioRendererHost hosted in RenderProcessHostImpl. Currently we only > have a getter for AudioRendererHost on RenderProcessHostImpl, not > RenderProcessHost. We should probably just add a method on > RenderProcessHost to get the AudioRendererHost to get this working. > > In general I feel passing |render_frame_id| around in IPC is bad. If > it's render frame specific, it should be handled in render frame level. > So I prefer the second option. But since I am not familiar with audio > code, I'll defer to other owners to help make the decision. > > +dalecurtis / grunell / tommi: WDYT? > > https://chromiumcodereview.appspot.com/1896883002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, On 2016/05/03 at 17:26:13, xhwang wrote: > Media should not know about render frames (which is in content/ layer). I chat with rchtara@ about this offline. Basically we have two options here: > > 1. Move this mojom file to content/ layer so that we can use |render_frame_id|. > > 2. As I suggested earlier, we can have AudioOutputImpl on the Render frame layer (hosted in RenderFrameHostImpl). Since we are already at the RenderFrame level, AudioOutputImpl should > already know about the |render_frame_id|, and we don't need to pass it in this interface. Then AudioOutputImpl can delegate the real tasks to the AudioRendererHost hosted in RenderProcessHostImpl. Currently we only have a getter for AudioRendererHost on RenderProcessHostImpl, not RenderProcessHost. We should probably just add a method on RenderProcessHost to get the AudioRendererHost to get this working. > > In general I feel passing |render_frame_id| around in IPC is bad. If it's render frame specific, it should be handled in render frame level. So I prefer the second option. But since I am not familiar with audio code, I'll defer to other owners to help make the decision. > > +dalecurtis / grunell / tommi: WDYT? media/ can't know about render frames, it's a content/ level concept, so there's not much room for discussion. We have to have a content level piece handle the render frame ids if they're needed, which I think they are at present (for audio logging, tab mute control, tab audio notifications, oom killer, power save blocking, etc). The other option is only to break apart that functionality such that another piece which lives in the content layer coordinates the needs of associating render_frame_id+pid to a given AudioOutput. I'm actually a little confused by the need for stream_id in this CL. I thought Mojo allowed us to avoid doing any ID tracking at all.
https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:22: int32 render_frame_id, we are going to move the interface to the content to avoid problems. https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/mojo_bindings.gyp (right): https://codereview.chromium.org/1896883002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:64: 'renderer.mojom', On 2016/04/29 17:47:57, xhwang wrote: > Most of these are never intended to work in GYP builds (also GYP is going away > very very soon). Only include what you need here. There available as dependencies for audio_output_mojom_bindings target bellow Done. https://codereview.chromium.org/1896883002/diff/260001/media/mojo/interfaces/... File media/mojo/interfaces/mojo_bindings.gyp (right): https://codereview.chromium.org/1896883002/diff/260001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:68: 'target_name': 'audio_output_mojom_bindings', On 2016/05/03 00:14:34, xhwang wrote: > The naming of the target doesn't really match what's inlcuded in "sources". Done. https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, After discussing with grunell, we think that it's probably a good idea to move audio_output.mojom to the content: the only reason we added the interface in media is that before, we used the interface in the audio_output_controller.cc/h in media. But now we don't. So, the normal place for audio_output.mojom is in content. https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, On 2016/05/03 21:07:37, DaleCurtis wrote: > On 2016/05/03 at 17:26:13, xhwang wrote: > > Media should not know about render frames (which is in content/ layer). I chat > with rchtara@ about this offline. Basically we have two options here: > > > > 1. Move this mojom file to content/ layer so that we can use > |render_frame_id|. > > > > 2. As I suggested earlier, we can have AudioOutputImpl on the Render frame > layer (hosted in RenderFrameHostImpl). Since we are already at the RenderFrame > level, AudioOutputImpl should > > already know about the |render_frame_id|, and we don't need to pass it in this > interface. Then AudioOutputImpl can delegate the real tasks to the > AudioRendererHost hosted in RenderProcessHostImpl. Currently we only have a > getter for AudioRendererHost on RenderProcessHostImpl, not RenderProcessHost. We > should probably just add a method on RenderProcessHost to get the > AudioRendererHost to get this working. > > > > In general I feel passing |render_frame_id| around in IPC is bad. If it's > render frame specific, it should be handled in render frame level. So I prefer > the second option. But since I am not familiar with audio code, I'll defer to > other owners to help make the decision. > > > > +dalecurtis / grunell / tommi: WDYT? > > media/ can't know about render frames, it's a content/ level concept, so there's > not much room for discussion. We have to have a content level piece handle the > render frame ids if they're needed, which I think they are at present (for audio > logging, tab mute control, tab audio notifications, oom killer, power save > blocking, etc). > > The other option is only to break apart that functionality such that another > piece which lives in the content layer coordinates the needs of associating > render_frame_id+pid to a given AudioOutput. After discussing with grunell, we think that it's probably a good idea to move audio_output.mojom to the content: the only reason we added the interface in media is that before, we used the interface in the audio_output_controller.cc/h in media. But now we don't. So, the normal place for audio_output.mojom is in content. > I'm actually a little confused by the need for stream_id in this CL. I thought > Mojo allowed us to avoid doing any ID tracking at all. stream_id is used for error detection in the renderer and the browser. https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Its' also used for logs: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... So, even if we remove it we are going to still need something for identifying the streams in logs in the browser and renderer. But for the play, pause and set_volume we are going to use the AudioOutputStreamPtr without needing to send the stream_id.
In general, the mojo bits look reasonable. Once //media folks are happy with the shape of the interface, I'll stamp it % some nits. https://codereview.chromium.org/1896883002/diff/340001/media/mojo/common/medi... File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/1896883002/diff/340001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:46: #define ASSERT_ENUM_EQ_COMPLEX(media_enum, media_prefix, mojo_prefix, value) \ Nit: Is there any time when |media_enum| isn't == |media_prefix|? https://codereview.chromium.org/1896883002/diff/340001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:76: AUDIO_FORMAT_LAST); This duplicates the item at line 69. https://codereview.chromium.org/1896883002/diff/340001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:410: // CHANNEL_LAYOUT_DISCRETE. The comment matches the first part of this `if` condition, what about the second (`input->channels == ChannelLayout..`)? https://codereview.chromium.org/1896883002/diff/340001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/340001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:207: // TODO(rchtara): add |mic_positions_| if needed. Nit: Drop trailing '_'.
rchtara@chromium.org changed reviewers: - tsepez@chromium.org
https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, On 2016/05/04 08:38:43, rchtara wrote: > On 2016/05/03 21:07:37, DaleCurtis wrote: > > On 2016/05/03 at 17:26:13, xhwang wrote: > > > Media should not know about render frames (which is in content/ layer). I > chat > > with rchtara@ about this offline. Basically we have two options here: > > > > > > 1. Move this mojom file to content/ layer so that we can use > > |render_frame_id|. > > > > > > 2. As I suggested earlier, we can have AudioOutputImpl on the Render frame > > layer (hosted in RenderFrameHostImpl). Since we are already at the RenderFrame > > level, AudioOutputImpl should > > > already know about the |render_frame_id|, and we don't need to pass it in > this > > interface. Then AudioOutputImpl can delegate the real tasks to the > > AudioRendererHost hosted in RenderProcessHostImpl. Currently we only have a > > getter for AudioRendererHost on RenderProcessHostImpl, not RenderProcessHost. > We > > should probably just add a method on RenderProcessHost to get the > > AudioRendererHost to get this working. > > > > > > In general I feel passing |render_frame_id| around in IPC is bad. If it's > > render frame specific, it should be handled in render frame level. So I prefer > > the second option. But since I am not familiar with audio code, I'll defer to > > other owners to help make the decision. > > > > > > +dalecurtis / grunell / tommi: WDYT? > > > > media/ can't know about render frames, it's a content/ level concept, so > there's > > not much room for discussion. We have to have a content level piece handle the > > render frame ids if they're needed, which I think they are at present (for > audio > > logging, tab mute control, tab audio notifications, oom killer, power save > > blocking, etc). > > > > The other option is only to break apart that functionality such that another > > piece which lives in the content layer coordinates the needs of associating > > render_frame_id+pid to a given AudioOutput. > > After discussing with grunell, we think that it's probably a good idea to move > audio_output.mojom to the content: the only reason we added the interface in > media is that before, we used the interface in the audio_output_controller.cc/h > in media. But now we don't. > So, the normal place for audio_output.mojom is in content. > > I'm actually a little confused by the need for stream_id in this CL. I thought > > Mojo allowed us to avoid doing any ID tracking at all. > stream_id is used for error detection in the renderer and the browser. > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > Its' also used for logs: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > So, even if we remove it we are going to still need something for identifying > the streams in logs in the browser and renderer. > > But for the play, pause and set_volume we are going to use the > AudioOutputStreamPtr without needing to send the stream_id. Another reason for not doing that is that in the current design we need it to retrieve the callback in AudioRendererHost::DoComeleteCreation: AudioOutputImpl::CreateStream will send media::mojom::AudioOutput::CreateStreamCallback as an argument to audio_renderer_host::CreateStream which will create an AudioEntry and store the callback inside it. Then, the audio_output_controller created by the AudioEntry will call AudioRendererHost::DoCompeleteCreation which is going to use the stream_id to find the audio_entry and call the right callback. https://codereview.chromium.org/1930393002/patch/60001/70004 In the previous architecture, we didn't use the stream_id to retrieve the callback : the AudioEntry didn't store the callback and just passed it to the controller then the audio_output_controller sent it back to the AudioRendererHost as an argument to DoCompleteCreation. This doesn't involve using the stream_id. However, it requires that the AudioOutputController is involved in Mojo which is not possible as the AudioOutputController is in media which cannot know about mojo and this is why we switched to the new architecture. https://codereview.chromium.org/1896883002/patch/60001/70011 https://codereview.chromium.org/1896883002/patch/60001/70004 So it's hard to remove stream_id. This is going to require a completely new audio rendering architecture and is going to break many things which is very risky. I will leave on the 3rd of June and me and grunell would like to land this CL and the play/pause/setvolume CL before I leave, and removing the stream_id is going to require much more time than I have. So, I prefer focusing on mojofying other stream operations than to work on removing the stream_id.
Hello, @dalecurtis and @xhwang: I added a CloseStream to the AudioOutput interface to close streams that are authorized but not created yet, otherwise we are going to get problems (chrome craches) with webrtc when user don't authorize the stream. What do you please think about that? @Mike West: solved the issues you have pointed. Could you please have another look. Thanks a lot Riadh https://codereview.chromium.org/1896883002/diff/340001/media/mojo/common/medi... File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/1896883002/diff/340001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:46: #define ASSERT_ENUM_EQ_COMPLEX(media_enum, media_prefix, mojo_prefix, value) \ On 2016/05/04 10:56:16, Mike West (OOO until the 6th) wrote: > Nit: Is there any time when |media_enum| isn't == |media_prefix|? Done. https://codereview.chromium.org/1896883002/diff/340001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:76: AUDIO_FORMAT_LAST); On 2016/05/04 10:56:17, Mike West (OOO until the 6th) wrote: > This duplicates the item at line 69. Done. https://codereview.chromium.org/1896883002/diff/340001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:410: // CHANNEL_LAYOUT_DISCRETE. On 2016/05/04 10:56:17, Mike West (OOO until the 6th) wrote: > The comment matches the first part of this `if` condition, what about the second > (`input->channels == ChannelLayout..`)? otherwise it will be computed from channel_layout_ in the AudioParameters constructor. https://codereview.chromium.org/1896883002/diff/340001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/340001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:207: // TODO(rchtara): add |mic_positions_| if needed. On 2016/05/04 10:56:17, Mike West (OOO until the 6th) wrote: > Nit: Drop trailing '_'. Done.
https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, On 2016/05/05 at 07:32:10, rchtara wrote: > On 2016/05/04 08:38:43, rchtara wrote: > > On 2016/05/03 21:07:37, DaleCurtis wrote: > > > On 2016/05/03 at 17:26:13, xhwang wrote: > > > > Media should not know about render frames (which is in content/ layer). I > > chat > > > with rchtara@ about this offline. Basically we have two options here: > > > > > > > > 1. Move this mojom file to content/ layer so that we can use > > > |render_frame_id|. > > > > > > > > 2. As I suggested earlier, we can have AudioOutputImpl on the Render frame > > > layer (hosted in RenderFrameHostImpl). Since we are already at the RenderFrame > > > level, AudioOutputImpl should > > > > already know about the |render_frame_id|, and we don't need to pass it in > > this > > > interface. Then AudioOutputImpl can delegate the real tasks to the > > > AudioRendererHost hosted in RenderProcessHostImpl. Currently we only have a > > > getter for AudioRendererHost on RenderProcessHostImpl, not RenderProcessHost. > > We > > > should probably just add a method on RenderProcessHost to get the > > > AudioRendererHost to get this working. > > > > > > > > In general I feel passing |render_frame_id| around in IPC is bad. If it's > > > render frame specific, it should be handled in render frame level. So I prefer > > > the second option. But since I am not familiar with audio code, I'll defer to > > > other owners to help make the decision. > > > > > > > > +dalecurtis / grunell / tommi: WDYT? > > > > > > media/ can't know about render frames, it's a content/ level concept, so > > there's > > > not much room for discussion. We have to have a content level piece handle the > > > render frame ids if they're needed, which I think they are at present (for > > audio > > > logging, tab mute control, tab audio notifications, oom killer, power save > > > blocking, etc). > > > > > > The other option is only to break apart that functionality such that another > > > piece which lives in the content layer coordinates the needs of associating > > > render_frame_id+pid to a given AudioOutput. > > > > After discussing with grunell, we think that it's probably a good idea to move > > audio_output.mojom to the content: the only reason we added the interface in > > media is that before, we used the interface in the audio_output_controller.cc/h > > in media. But now we don't. > > So, the normal place for audio_output.mojom is in content. > > > I'm actually a little confused by the need for stream_id in this CL. I thought > > > Mojo allowed us to avoid doing any ID tracking at all. > > stream_id is used for error detection in the renderer and the browser. > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > Its' also used for logs: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > So, even if we remove it we are going to still need something for identifying > > the streams in logs in the browser and renderer. > > > > But for the play, pause and set_volume we are going to use the > > AudioOutputStreamPtr without needing to send the stream_id. > > Another reason for not doing that is that in the current design we need it to retrieve the callback in AudioRendererHost::DoComeleteCreation: > AudioOutputImpl::CreateStream will send media::mojom::AudioOutput::CreateStreamCallback as an argument to audio_renderer_host::CreateStream which will create an AudioEntry and store the callback inside it. Then, the audio_output_controller created by the AudioEntry will call AudioRendererHost::DoCompeleteCreation which is going to use the stream_id to find the audio_entry and call the right callback. > > https://codereview.chromium.org/1930393002/patch/60001/70004 > > In the previous architecture, we didn't use the stream_id to retrieve the callback : > the AudioEntry didn't store the callback and just passed it to the controller then the audio_output_controller sent it back to the AudioRendererHost as an argument to DoCompleteCreation. This doesn't involve using the stream_id. However, it requires that the AudioOutputController is involved in Mojo which is not possible as the AudioOutputController is in media which cannot know about mojo and this is why we switched to the new architecture. > https://codereview.chromium.org/1896883002/patch/60001/70011 > https://codereview.chromium.org/1896883002/patch/60001/70004 > > > So it's hard to remove stream_id. This is going to require a completely new audio rendering architecture and is going to break many things which is very risky. > I will leave on the 3rd of June and me and grunell would like to land this CL and the play/pause/setvolume CL before I leave, and removing the stream_id is going to require much more time than I have. So, I prefer focusing on mojofying other stream operations than to work on removing the stream_id. Do we need the stream id to be sent between the renderer and browser though? I.e. it seems like the control / rendering channel shouldn't need a stream_id, but to avoid leaking mojo details into AudioOutputController we do need the stream id as you've mentioned. Thus it seems like the mojo endpoint should track a mapping of MojoPtr => stream_id on the browser side that it hands to other browser side components. If you just remove it from the mojom and construct a new mapping on the service side, does it require as much changes as you were thinking?
miu@chromium.org changed reviewers: + miu@chromium.org
https://codereview.chromium.org/1896883002/diff/360001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/360001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:204: int32 bits_per_sample; Drive-by question: Is bits_per_sample really needed? If all audio data is transferred in planar float format, this field is irrelevant.
https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, On 2016/05/05 17:41:57, DaleCurtis wrote: > On 2016/05/05 at 07:32:10, rchtara wrote: > > On 2016/05/04 08:38:43, rchtara wrote: > > > On 2016/05/03 21:07:37, DaleCurtis wrote: > > > > On 2016/05/03 at 17:26:13, xhwang wrote: > > > > > Media should not know about render frames (which is in content/ layer). > I > > > chat > > > > with rchtara@ about this offline. Basically we have two options here: > > > > > > > > > > 1. Move this mojom file to content/ layer so that we can use > > > > |render_frame_id|. > > > > > > > > > > 2. As I suggested earlier, we can have AudioOutputImpl on the Render > frame > > > > layer (hosted in RenderFrameHostImpl). Since we are already at the > RenderFrame > > > > level, AudioOutputImpl should > > > > > already know about the |render_frame_id|, and we don't need to pass it > in > > > this > > > > interface. Then AudioOutputImpl can delegate the real tasks to the > > > > AudioRendererHost hosted in RenderProcessHostImpl. Currently we only have > a > > > > getter for AudioRendererHost on RenderProcessHostImpl, not > RenderProcessHost. > > > We > > > > should probably just add a method on RenderProcessHost to get the > > > > AudioRendererHost to get this working. > > > > > > > > > > In general I feel passing |render_frame_id| around in IPC is bad. If > it's > > > > render frame specific, it should be handled in render frame level. So I > prefer > > > > the second option. But since I am not familiar with audio code, I'll defer > to > > > > other owners to help make the decision. > > > > > > > > > > +dalecurtis / grunell / tommi: WDYT? > > > > > > > > media/ can't know about render frames, it's a content/ level concept, so > > > there's > > > > not much room for discussion. We have to have a content level piece handle > the > > > > render frame ids if they're needed, which I think they are at present (for > > > audio > > > > logging, tab mute control, tab audio notifications, oom killer, power save > > > > blocking, etc). > > > > > > > > The other option is only to break apart that functionality such that > another > > > > piece which lives in the content layer coordinates the needs of > associating > > > > render_frame_id+pid to a given AudioOutput. > > > > > > After discussing with grunell, we think that it's probably a good idea to > move > > > audio_output.mojom to the content: the only reason we added the interface in > > > media is that before, we used the interface in the > audio_output_controller.cc/h > > > in media. But now we don't. > > > So, the normal place for audio_output.mojom is in content. > > > > I'm actually a little confused by the need for stream_id in this CL. I > thought > > > > Mojo allowed us to avoid doing any ID tracking at all. > > > stream_id is used for error detection in the renderer and the browser. > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > > > Its' also used for logs: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > So, even if we remove it we are going to still need something for > identifying > > > the streams in logs in the browser and renderer. > > > > > > But for the play, pause and set_volume we are going to use the > > > AudioOutputStreamPtr without needing to send the stream_id. > > > > Another reason for not doing that is that in the current design we need it to > retrieve the callback in AudioRendererHost::DoComeleteCreation: > > AudioOutputImpl::CreateStream will send > media::mojom::AudioOutput::CreateStreamCallback as an argument to > audio_renderer_host::CreateStream which will create an AudioEntry and store the > callback inside it. Then, the audio_output_controller created by the AudioEntry > will call AudioRendererHost::DoCompeleteCreation which is going to use the > stream_id to find the audio_entry and call the right callback. > > > > https://codereview.chromium.org/1930393002/patch/60001/70004 > > > > In the previous architecture, we didn't use the stream_id to retrieve the > callback : > > the AudioEntry didn't store the callback and just passed it to the controller > then the audio_output_controller sent it back to the AudioRendererHost as an > argument to DoCompleteCreation. This doesn't involve using the stream_id. > However, it requires that the AudioOutputController is involved in Mojo which is > not possible as the AudioOutputController is in media which cannot know about > mojo and this is why we switched to the new architecture. > > https://codereview.chromium.org/1896883002/patch/60001/70011 > > https://codereview.chromium.org/1896883002/patch/60001/70004 > > > > > > So it's hard to remove stream_id. This is going to require a completely new > audio rendering architecture and is going to break many things which is very > risky. > > I will leave on the 3rd of June and me and grunell would like to land this CL > and the play/pause/setvolume CL before I leave, and removing the stream_id is > going to require much more time than I have. So, I prefer focusing on mojofying > other stream operations than to work on removing the stream_id. > > Do we need the stream id to be sent between the renderer and browser though? > I.e. it seems like the control / rendering channel shouldn't need a stream_id, > but to avoid leaking mojo details into AudioOutputController we do need the > stream id as you've mentioned. Thus it seems like the mojo endpoint should track > a mapping of MojoPtr => stream_id on the browser side that it hands to other > browser side components. > > If you just remove it from the mojom and construct a new mapping on the service > side, does it require as much changes as you were thinking? If we do so, the id of streams in the log provided by the renderer and the browser are not going to be the same (they are many of logs in audio_renderer_host.cc https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... at least one in the audio_message_filter. https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Also stream_ids are going to be used to detect many kinds of errors in the browser and the renderer, so if we have different mapping in the renderer and the browser, all these error mechanisms are going to fail: for example: *delegate corresponding to stream_id not found in the delegates_ lookup table in audio_message_filter.cc: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... *entry corresponding to stream_id not found in the audio_entries_ map in audio_message_filter.cc: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
https://codereview.chromium.org/1896883002/diff/360001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/360001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:204: int32 bits_per_sample; On 2016/05/05 20:16:39, miu wrote: > Drive-by question: Is bits_per_sample really needed? If all audio data is > transferred in planar float format, this field is irrelevant. No sure about that: it's set here for example: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_p... So it's needed. I tried to keep here the structure of AudioParameters in audio_parameters.h as much as I can: we are going to use this mojo structure when we are going to mojofy the AudioMsg_NotifyDeviceAuthorized and probably later when mojofying other audio staffs. They are many references to the AudioParameters (1451 exactly) so, I think if you want to remove bits_per_sample, this could be better done in a separate cl.
https://codereview.chromium.org/1896883002/diff/340001/media/mojo/common/medi... File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/1896883002/diff/340001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:410: // CHANNEL_LAYOUT_DISCRETE. On 2016/05/05 at 15:59:44, rchtara wrote: > On 2016/05/04 10:56:17, Mike West (OOO until the 6th) wrote: > > The comment matches the first part of this `if` condition, what about the second > > (`input->channels == ChannelLayout..`)? > > otherwise it will be computed from channel_layout_ in the AudioParameters constructor. Say that in the comment?
https://codereview.chromium.org/1896883002/diff/340001/media/mojo/common/medi... File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/1896883002/diff/340001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:410: // CHANNEL_LAYOUT_DISCRETE. On 2016/05/06 13:24:42, Mike West wrote: > On 2016/05/05 at 15:59:44, rchtara wrote: > > On 2016/05/04 10:56:17, Mike West (OOO until the 6th) wrote: > > > The comment matches the first part of this `if` condition, what about the > second > > > (`input->channels == ChannelLayout..`)? > > > > otherwise it will be computed from channel_layout_ in the AudioParameters > constructor. > > Say that in the comment? Done.
Looking mostly good from mojo's perspective % some comments. I'll defer to other reviewers to make sure this is sane from "audio"'s perspective. https://codereview.chromium.org/1896883002/diff/420001/content/common/media/a... File content/common/media/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/420001/content/common/media/a... content/common/media/audio_output.mojom:5: module media.mojom; This should be in content.mojom now. https://codereview.chromium.org/1896883002/diff/420001/content/common/media/a... content/common/media/audio_output.mojom:33: CloseStream(int32 stream_id); Typically in mojo world when the connection is dropped the interface implementation will be closed. But I don't know enough detail here to give better suggestions. Also, this CloseStream() call is related to authorization. But it seems authorization is still TODO. Shall we add this call later? From this current API I can't really see how CloseStream() could be used. https://codereview.chromium.org/1896883002/diff/420001/content/content_render... File content/content_renderer.gypi (right): https://codereview.chromium.org/1896883002/diff/420001/content/content_render... content/content_renderer.gypi:32: '../mojo/mojo_public.gyp:mojo_js_bindings', Why do you need these changes?
dalecurtis@chromium.org changed reviewers: + creis@chromium.org
We should have one of the content/ mojo owners take a look too, +creis for suggestions; I'm not sure if they're planning to embark into the Mojo world carrying around render frame integer IDs forever. https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, On 2016/05/06 at 07:29:47, rchtara wrote: > On 2016/05/05 17:41:57, DaleCurtis wrote: > > On 2016/05/05 at 07:32:10, rchtara wrote: > > > On 2016/05/04 08:38:43, rchtara wrote: > > > > On 2016/05/03 21:07:37, DaleCurtis wrote: > > > > > On 2016/05/03 at 17:26:13, xhwang wrote: > > > > > > Media should not know about render frames (which is in content/ layer). > > I > > > > chat > > > > > with rchtara@ about this offline. Basically we have two options here: > > > > > > > > > > > > 1. Move this mojom file to content/ layer so that we can use > > > > > |render_frame_id|. > > > > > > > > > > > > 2. As I suggested earlier, we can have AudioOutputImpl on the Render > > frame > > > > > layer (hosted in RenderFrameHostImpl). Since we are already at the > > RenderFrame > > > > > level, AudioOutputImpl should > > > > > > already know about the |render_frame_id|, and we don't need to pass it > > in > > > > this > > > > > interface. Then AudioOutputImpl can delegate the real tasks to the > > > > > AudioRendererHost hosted in RenderProcessHostImpl. Currently we only have > > a > > > > > getter for AudioRendererHost on RenderProcessHostImpl, not > > RenderProcessHost. > > > > We > > > > > should probably just add a method on RenderProcessHost to get the > > > > > AudioRendererHost to get this working. > > > > > > > > > > > > In general I feel passing |render_frame_id| around in IPC is bad. If > > it's > > > > > render frame specific, it should be handled in render frame level. So I > > prefer > > > > > the second option. But since I am not familiar with audio code, I'll defer > > to > > > > > other owners to help make the decision. > > > > > > > > > > > > +dalecurtis / grunell / tommi: WDYT? > > > > > > > > > > media/ can't know about render frames, it's a content/ level concept, so > > > > there's > > > > > not much room for discussion. We have to have a content level piece handle > > the > > > > > render frame ids if they're needed, which I think they are at present (for > > > > audio > > > > > logging, tab mute control, tab audio notifications, oom killer, power save > > > > > blocking, etc). > > > > > > > > > > The other option is only to break apart that functionality such that > > another > > > > > piece which lives in the content layer coordinates the needs of > > associating > > > > > render_frame_id+pid to a given AudioOutput. > > > > > > > > After discussing with grunell, we think that it's probably a good idea to > > move > > > > audio_output.mojom to the content: the only reason we added the interface in > > > > media is that before, we used the interface in the > > audio_output_controller.cc/h > > > > in media. But now we don't. > > > > So, the normal place for audio_output.mojom is in content. > > > > > I'm actually a little confused by the need for stream_id in this CL. I > > thought > > > > > Mojo allowed us to avoid doing any ID tracking at all. > > > > stream_id is used for error detection in the renderer and the browser. > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > > > > > Its' also used for logs: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > So, even if we remove it we are going to still need something for > > identifying > > > > the streams in logs in the browser and renderer. > > > > > > > > But for the play, pause and set_volume we are going to use the > > > > AudioOutputStreamPtr without needing to send the stream_id. > > > > > > Another reason for not doing that is that in the current design we need it to > > retrieve the callback in AudioRendererHost::DoComeleteCreation: > > > AudioOutputImpl::CreateStream will send > > media::mojom::AudioOutput::CreateStreamCallback as an argument to > > audio_renderer_host::CreateStream which will create an AudioEntry and store the > > callback inside it. Then, the audio_output_controller created by the AudioEntry > > will call AudioRendererHost::DoCompeleteCreation which is going to use the > > stream_id to find the audio_entry and call the right callback. > > > > > > https://codereview.chromium.org/1930393002/patch/60001/70004 > > > > > > In the previous architecture, we didn't use the stream_id to retrieve the > > callback : > > > the AudioEntry didn't store the callback and just passed it to the controller > > then the audio_output_controller sent it back to the AudioRendererHost as an > > argument to DoCompleteCreation. This doesn't involve using the stream_id. > > However, it requires that the AudioOutputController is involved in Mojo which is > > not possible as the AudioOutputController is in media which cannot know about > > mojo and this is why we switched to the new architecture. > > > https://codereview.chromium.org/1896883002/patch/60001/70011 > > > https://codereview.chromium.org/1896883002/patch/60001/70004 > > > > > > > > > So it's hard to remove stream_id. This is going to require a completely new > > audio rendering architecture and is going to break many things which is very > > risky. > > > I will leave on the 3rd of June and me and grunell would like to land this CL > > and the play/pause/setvolume CL before I leave, and removing the stream_id is > > going to require much more time than I have. So, I prefer focusing on mojofying > > other stream operations than to work on removing the stream_id. > > > > Do we need the stream id to be sent between the renderer and browser though? > > I.e. it seems like the control / rendering channel shouldn't need a stream_id, > > but to avoid leaking mojo details into AudioOutputController we do need the > > stream id as you've mentioned. Thus it seems like the mojo endpoint should track > > a mapping of MojoPtr => stream_id on the browser side that it hands to other > > browser side components. > > > > If you just remove it from the mojom and construct a new mapping on the service > > side, does it require as much changes as you were thinking? > > If we do so, the id of streams in the log provided by the renderer and the browser are not going to be the same (they are many of logs in audio_renderer_host.cc https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > at least one in the audio_message_filter. > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > Also stream_ids are going to be used to detect many kinds of errors in the browser and the renderer, so if we have different mapping in the renderer and the browser, all these error mechanisms are going to fail: for example: > *delegate corresponding to stream_id not found in the delegates_ lookup table in audio_message_filter.cc: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > *entry corresponding to stream_id not found in the audio_entries_ map in audio_message_filter.cc: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... Those stream ids are worthless on the renderer side since they aren't accurate across processes. If we want to keep a stream id it shouldn't be passed in from the renderer side, only returned from the browser side (where it can be kept correct across processes). I'm fine with keeping the stream id in the return, but it shouldn't be passed in when calling CreateStream(). https://codereview.chromium.org/1896883002/diff/360001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/360001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:204: int32 bits_per_sample; On 2016/05/06 at 08:57:33, rchtara wrote: > On 2016/05/05 20:16:39, miu wrote: > > Drive-by question: Is bits_per_sample really needed? If all audio data is > > transferred in planar float format, this field is irrelevant. > No sure about that: it's set here for example: > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_p... > So it's needed. > > I tried to keep here the structure of AudioParameters in audio_parameters.h as much as I can: we are going to use this mojo structure when we are going to mojofy the AudioMsg_NotifyDeviceAuthorized and probably later when mojofying other audio staffs. > > They are many references to the AudioParameters (1451 exactly) so, I think if you want to remove bits_per_sample, this could be better done in a separate cl. It's irrelevant, so I'd drop it and just add a DCHECK that you only receive bits_per_sample==16 on the renderer side in case someone ever changes it.
https://codereview.chromium.org/1896883002/diff/360001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/360001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:204: int32 bits_per_sample; On 2016/05/06 17:31:19, DaleCurtis wrote: > On 2016/05/06 at 08:57:33, rchtara wrote: > > On 2016/05/05 20:16:39, miu wrote: > > > Drive-by question: Is bits_per_sample really needed? If all audio data is > > > transferred in planar float format, this field is irrelevant. > > No sure about that: it's set here for example: > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_p... > > So it's needed. > > > > I tried to keep here the structure of AudioParameters in audio_parameters.h > as much as I can: we are going to use this mojo structure when we are going to > mojofy the AudioMsg_NotifyDeviceAuthorized and probably later when mojofying > other audio staffs. > > > > They are many references to the AudioParameters (1451 exactly) so, I think if > you want to remove bits_per_sample, this could be better done in a separate cl. > > It's irrelevant, so I'd drop it and just add a DCHECK that you only receive > bits_per_sample==16 on the renderer side in case someone ever changes it. BTW--I filed a "FixIt/Polish" bug for this (explains more about why this is not needed): https://crbug.com/609890
creis@chromium.org changed reviewers: + jam@chromium.org - creis@chromium.org
On 2016/05/06 17:31:19, DaleCurtis wrote: > We should have one of the content/ mojo owners take a look too, +creis for > suggestions; I'm not sure if they're planning to embark into the Mojo world > carrying around render frame integer IDs forever. +jam for Mojo vs RenderFrame routing ID question. I don't know of any plans to eliminate routing IDs, since they're fundamental to a lot of things in content/. Then again, I'm not up to speed with the Mojo plans or the context in the discussion below. > > https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... > File media/mojo/interfaces/audio_output.mojom (right): > > https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... > media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, > On 2016/05/06 at 07:29:47, rchtara wrote: > > On 2016/05/05 17:41:57, DaleCurtis wrote: > > > On 2016/05/05 at 07:32:10, rchtara wrote: > > > > On 2016/05/04 08:38:43, rchtara wrote: > > > > > On 2016/05/03 21:07:37, DaleCurtis wrote: > > > > > > On 2016/05/03 at 17:26:13, xhwang wrote: > > > > > > > Media should not know about render frames (which is in content/ > layer). > > > I > > > > > chat > > > > > > with rchtara@ about this offline. Basically we have two options here: > > > > > > > > > > > > > > 1. Move this mojom file to content/ layer so that we can use > > > > > > |render_frame_id|. > > > > > > > > > > > > > > 2. As I suggested earlier, we can have AudioOutputImpl on the Render > > > frame > > > > > > layer (hosted in RenderFrameHostImpl). Since we are already at the > > > RenderFrame > > > > > > level, AudioOutputImpl should > > > > > > > already know about the |render_frame_id|, and we don't need to pass > it > > > in > > > > > this > > > > > > interface. Then AudioOutputImpl can delegate the real tasks to the > > > > > > AudioRendererHost hosted in RenderProcessHostImpl. Currently we only > have > > > a > > > > > > getter for AudioRendererHost on RenderProcessHostImpl, not > > > RenderProcessHost. > > > > > We > > > > > > should probably just add a method on RenderProcessHost to get the > > > > > > AudioRendererHost to get this working. > > > > > > > > > > > > > > In general I feel passing |render_frame_id| around in IPC is bad. If > > > it's > > > > > > render frame specific, it should be handled in render frame level. So > I > > > prefer > > > > > > the second option. But since I am not familiar with audio code, I'll > defer > > > to > > > > > > other owners to help make the decision. > > > > > > > > > > > > > > +dalecurtis / grunell / tommi: WDYT? > > > > > > > > > > > > media/ can't know about render frames, it's a content/ level concept, > so > > > > > there's > > > > > > not much room for discussion. We have to have a content level piece > handle > > > the > > > > > > render frame ids if they're needed, which I think they are at present > (for > > > > > audio > > > > > > logging, tab mute control, tab audio notifications, oom killer, power > save > > > > > > blocking, etc). > > > > > > > > > > > > The other option is only to break apart that functionality such that > > > another > > > > > > piece which lives in the content layer coordinates the needs of > > > associating > > > > > > render_frame_id+pid to a given AudioOutput. > > > > > > > > > > After discussing with grunell, we think that it's probably a good idea > to > > > move > > > > > audio_output.mojom to the content: the only reason we added the > interface in > > > > > media is that before, we used the interface in the > > > audio_output_controller.cc/h > > > > > in media. But now we don't. > > > > > So, the normal place for audio_output.mojom is in content. > > > > > > I'm actually a little confused by the need for stream_id in this CL. I > > > thought > > > > > > Mojo allowed us to avoid doing any ID tracking at all. > > > > > stream_id is used for error detection in the renderer and the browser. > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > > > > > > > Its' also used for logs: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > So, even if we remove it we are going to still need something for > > > identifying > > > > > the streams in logs in the browser and renderer. > > > > > > > > > > But for the play, pause and set_volume we are going to use the > > > > > AudioOutputStreamPtr without needing to send the stream_id. > > > > > > > > Another reason for not doing that is that in the current design we need it > to > > > retrieve the callback in AudioRendererHost::DoComeleteCreation: > > > > AudioOutputImpl::CreateStream will send > > > media::mojom::AudioOutput::CreateStreamCallback as an argument to > > > audio_renderer_host::CreateStream which will create an AudioEntry and store > the > > > callback inside it. Then, the audio_output_controller created by the > AudioEntry > > > will call AudioRendererHost::DoCompeleteCreation which is going to use the > > > stream_id to find the audio_entry and call the right callback. > > > > > > > > https://codereview.chromium.org/1930393002/patch/60001/70004 > > > > > > > > In the previous architecture, we didn't use the stream_id to retrieve the > > > callback : > > > > the AudioEntry didn't store the callback and just passed it to the > controller > > > then the audio_output_controller sent it back to the AudioRendererHost as an > > > argument to DoCompleteCreation. This doesn't involve using the stream_id. > > > However, it requires that the AudioOutputController is involved in Mojo > which is > > > not possible as the AudioOutputController is in media which cannot know > about > > > mojo and this is why we switched to the new architecture. > > > > https://codereview.chromium.org/1896883002/patch/60001/70011 > > > > https://codereview.chromium.org/1896883002/patch/60001/70004 > > > > > > > > > > > > So it's hard to remove stream_id. This is going to require a completely > new > > > audio rendering architecture and is going to break many things which is very > > > risky. > > > > I will leave on the 3rd of June and me and grunell would like to land this > CL > > > and the play/pause/setvolume CL before I leave, and removing the stream_id > is > > > going to require much more time than I have. So, I prefer focusing on > mojofying > > > other stream operations than to work on removing the stream_id. > > > > > > Do we need the stream id to be sent between the renderer and browser though? > > > I.e. it seems like the control / rendering channel shouldn't need a > stream_id, > > > but to avoid leaking mojo details into AudioOutputController we do need the > > > stream id as you've mentioned. Thus it seems like the mojo endpoint should > track > > > a mapping of MojoPtr => stream_id on the browser side that it hands to other > > > browser side components. > > > > > > If you just remove it from the mojom and construct a new mapping on the > service > > > side, does it require as much changes as you were thinking? > > > > If we do so, the id of streams in the log provided by the renderer and the > browser are not going to be the same (they are many of logs in > audio_renderer_host.cc > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > at least one in the audio_message_filter. > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > > > Also stream_ids are going to be used to detect many kinds of errors in the > browser and the renderer, so if we have different mapping in the renderer and > the browser, all these error mechanisms are going to fail: for example: > > *delegate corresponding to stream_id not found in the delegates_ lookup table > in audio_message_filter.cc: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > *entry corresponding to stream_id not found in the audio_entries_ map in > audio_message_filter.cc: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > Those stream ids are worthless on the renderer side since they aren't accurate > across processes. If we want to keep a stream id it shouldn't be passed in from > the renderer side, only returned from the browser side (where it can be kept > correct across processes). I'm fine with keeping the stream id in the return, > but it shouldn't be passed in when calling CreateStream(). >
On 2016/05/06 18:17:42, Charlie Reis wrote: > On 2016/05/06 17:31:19, DaleCurtis wrote: > > We should have one of the content/ mojo owners take a look too, +creis for > > suggestions; I'm not sure if they're planning to embark into the Mojo world > > carrying around render frame integer IDs forever. > > +jam for Mojo vs RenderFrame routing ID question. I don't know of any plans to > eliminate routing IDs, since they're fundamental to a lot of things in content/. > Then again, I'm not up to speed with the Mojo plans or the context in the > discussion below. routing IDs are a legacy of chrome IPC. With Mojo, we shouldn't need to pass them anywhere. The pipe's owner identifies which RenderFrame or RenderFrameHost it belongs to. This change is too small to review; for chrome IPC we never just landed a _messages.h file alone. Please combine it with the change to switch to mojo, and then it would be possible for us when looking at the code to see how the routing ID is used (if it all), and suggest alternatives.
Thanks a lot guys for the reviews. OK, I have removed render_frame_id from this CL and will work too on removing it from the other CL (1930393002) https://codereview.chromium.org/1930393002. @miu: thanks for filing a bug for removing dead bits_per_sample.
https://codereview.chromium.org/1896883002/diff/420001/content/common/media/a... File content/common/media/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/420001/content/common/media/a... content/common/media/audio_output.mojom:5: module media.mojom; On 2016/05/06 16:27:29, xhwang wrote: > This should be in content.mojom now. Done. https://codereview.chromium.org/1896883002/diff/420001/content/common/media/a... content/common/media/audio_output.mojom:33: CloseStream(int32 stream_id); On 2016/05/06 16:27:29, xhwang wrote: > Typically in mojo world when the connection is dropped the interface > implementation will be closed. But I don't know enough detail here to give > better suggestions. We were planning to take advantage of that to report errors from the browser to the renderer as reporting errors is more complicated than Closing the stream. * closing stream is the simplest operation we are going to do: we dont even have to return anything from the renderer to the browser. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... * SendErrorMessage is used 7 times in audio_renderer_host.cc (after creating stream, playing it,pausing it). So the plan is to drop the connection after we found an error in the browser, so the renderer is going to be notified that there is an error. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > Also, this CloseStream() call is related to authorization. But it seems > authorization is still TODO. Shall we add this call later? From this current API > I can't really see how CloseStream() could be used. The reason why it's included here is that I want to get rid of all the AudioHostMsg_CloseStream after this cl and cl 1930393002 land. PS: This is something temporary and the new plan will be to remove CloseStream form AudioOutput when RequestDeviceAuthorization lands and replace it with a new create method in AudioOutputStream: A AudioOutputStreamPtr will be directly returned after RequestDeviceAuthorization call. Then you can call Create (which is a new member method of AudioOutputStream) on the AudioOutputStreamPtr to do the equivalent of AudioHostMsg_CreateStream. If you want to close the stream you can call Close with or without calling Create. See please updated interface (https://docs.google.com/document/d/1wE5dNDPZE1OGMuEkKvRiRxlrV_50DJqUvQBkvJEqy...) https://codereview.chromium.org/1896883002/diff/420001/content/content_render... File content/content_renderer.gypi (right): https://codereview.chromium.org/1896883002/diff/420001/content/content_render... content/content_renderer.gypi:32: '../mojo/mojo_public.gyp:mojo_js_bindings', On 2016/05/06 16:27:29, xhwang wrote: > Why do you need these changes? They are needed for the other cl, but I will remove them for this one
On 2016/05/09 15:12:45, rchtara wrote: > Thanks a lot guys for the reviews. > OK, I have removed render_frame_id from this CL and will work too on removing it > from the other CL (1930393002) https://codereview.chromium.org/1930393002. > @miu: thanks for filing a bug for removing dead bits_per_sample. per my earlier comment: this change is too small to land. what's the point of trying to land a mojo interface alone? combine it with change that uses it please, that makes things clearer to review.
On 2016/05/09 16:21:00, jam wrote: > On 2016/05/09 15:12:45, rchtara wrote: > > Thanks a lot guys for the reviews. > > OK, I have removed render_frame_id from this CL and will work too on removing > it > > from the other CL (1930393002) https://codereview.chromium.org/1930393002. > > @miu: thanks for filing a bug for removing dead bits_per_sample. > > per my earlier comment: this change is too small to land. what's the point of > trying to land a mojo interface alone? combine it with change that uses it > please, that makes things clearer to review. There was a discussion in the review earlier where rchtara was asked to split the cl up into two smaller CLs to make them easier to review. A suggestion was made to split it up the way it is now, so that's what he did. If at all possible, I would like to go with the current approach since we have a lot of reviewers and it would be nice to not have to have more back-and-forth restarts of the cl. Other reviewers: wdyt?
https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, The stream_id is now generated in the renderer in AudioMessageFilter::AudioOutputIPCImpl::CreateStream A delegate will be created for this stream_id. Then AudioHostMsg_CreateStream(stream_id_, render_frame_id_, params) is going to be sent. Later when AudioMsg_NotifyStreamCreated is back, the stream_id will be used to retrieve the corresponding delegate. If the stream_id is generated in the browser, it's not going to be possible to find its corresponding delegate when AudioMsg_NotifyStreamCreated comes.
https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, On 2016/05/09 at 17:22:07, rchtara wrote: > The stream_id is now generated in the renderer in > AudioMessageFilter::AudioOutputIPCImpl::CreateStream > A delegate will be created for this stream_id. > Then AudioHostMsg_CreateStream(stream_id_, render_frame_id_, params) is going to be sent. > Later when AudioMsg_NotifyStreamCreated is back, the stream_id will be used to retrieve the corresponding delegate. > If the stream_id is generated in the browser, it's not going to be possible to find its corresponding delegate when AudioMsg_NotifyStreamCreated comes. This doesn't seem correct, IDs are not necessary to identify mojo endpoints. Presumably we'll still have something like AudioMessageFilter which lives in content that acts as an MojoAudioOutputFactory. AudioOutputDevice will have some way of calling Create() which would do something like: MojoAudioOutputFactory::Create(params) { shell::GetInterface<interfaces::AudioOutput>(interface_provider_, &audio_output_ptr); return new MojoAudioOutput(audio_output_ptr); } MojoAudioOutput { MojoAudioOutput() { audio_output_ptr->Create(params, base::Bind(OnCreated, this, ...)); } OnCreated(...) { } } The AudioOutputDevice would then use this returned MojoAudioOutput object for general control mechanisms and playback. There is no longer a AudioMsg_NotifyStreamCreated in this world.
On 2016/05/09 16:54:54, tommi-chrömium wrote: > On 2016/05/09 16:21:00, jam wrote: > > On 2016/05/09 15:12:45, rchtara wrote: > > > Thanks a lot guys for the reviews. > > > OK, I have removed render_frame_id from this CL and will work too on > removing > > it > > > from the other CL (1930393002) https://codereview.chromium.org/1930393002. > > > @miu: thanks for filing a bug for removing dead bits_per_sample. > > > > per my earlier comment: this change is too small to land. what's the point of > > trying to land a mojo interface alone? combine it with change that uses it > > please, that makes things clearer to review. > > There was a discussion in the review earlier where rchtara was asked to split > the cl up into two smaller CLs to make them easier to review. > A suggestion was made to split it up the way it is now, so that's what he did. > If at all possible, I would like to go with the current approach since we have a > lot of reviewers and it would be nice to not have to have more back-and-forth > restarts of the cl. > > Other reviewers: wdyt? can you point me to which comment (as you mention this has gone on for a long time so there are too many comments to look through)? While my intent is not to slow this down, it really makes things hard to review this when it's in isolation. i.e. right now I see this is adding type convertors. but i don't see how they'll be called. type convertors are an old concept, and the preferred mechanism now is type maps. see https://www.chromium.org/developers/design-documents/mojo/type-mapping it's also unfortunate to see media_types.mojom which duplicates other structs in media. the goal should be to write these structs once (in mojoms). Then other code in media can use them, even if they're not doing IPC. This avoids duplicate structs and conversions.
On 2016/05/09 20:26:32, jam wrote: > On 2016/05/09 16:54:54, tommi-chrömium wrote: > > On 2016/05/09 16:21:00, jam wrote: > > > On 2016/05/09 15:12:45, rchtara wrote: > > > > Thanks a lot guys for the reviews. > > > > OK, I have removed render_frame_id from this CL and will work too on > > removing > > > it > > > > from the other CL (1930393002) https://codereview.chromium.org/1930393002. > > > > @miu: thanks for filing a bug for removing dead bits_per_sample. > > > > > > per my earlier comment: this change is too small to land. what's the point > of > > > trying to land a mojo interface alone? combine it with change that uses it > > > please, that makes things clearer to review. > > > > There was a discussion in the review earlier where rchtara was asked to split > > the cl up into two smaller CLs to make them easier to review. > > A suggestion was made to split it up the way it is now, so that's what he did. > > > If at all possible, I would like to go with the current approach since we have > a > > lot of reviewers and it would be nice to not have to have more back-and-forth > > restarts of the cl. > > > > Other reviewers: wdyt? > > can you point me to which comment (as you mention this has gone on for a long > time so there are too many comments to look through)? While my intent is not to > slow this down, it really makes things hard to review this when it's in > isolation. > > i.e. right now I see this is adding type convertors. but i don't see how they'll > be called. type convertors are an old concept, and the preferred mechanism now > is type maps. see > https://www.chromium.org/developers/design-documents/mojo/type-mapping > > it's also unfortunate to see media_types.mojom which duplicates other structs in > media. the goal should be to write these structs once (in mojoms). Then other > code in media can use them, even if they're not doing IPC. This avoids duplicate > structs and conversions. The request for splitting was in an off-review mail or chat rchtara@ had with one of the reviewers. The positive confirmation is in https://codereview.chromium.org/1896883002/#msg35 and https://codereview.chromium.org/1896883002/#msg33 The usage of this interface is in the CL linked in the description.
Miguel and Dale, any thoughts for or against splitting? On Tue, May 10, 2016, 08:07 <grunell@chromium.org> wrote: > On 2016/05/09 20:26:32, jam wrote: > > On 2016/05/09 16:54:54, tommi-chrömium wrote: > > > On 2016/05/09 16:21:00, jam wrote: > > > > On 2016/05/09 15:12:45, rchtara wrote: > > > > > Thanks a lot guys for the reviews. > > > > > OK, I have removed render_frame_id from this CL and will work too > on > > > removing > > > > it > > > > > from the other CL (1930393002) > https://codereview.chromium.org/1930393002. > > > > > @miu: thanks for filing a bug for removing dead bits_per_sample. > > > > > > > > per my earlier comment: this change is too small to land. what's the > point > > of > > > > trying to land a mojo interface alone? combine it with change that > uses it > > > > please, that makes things clearer to review. > > > > > > There was a discussion in the review earlier where rchtara was asked to > split > > > the cl up into two smaller CLs to make them easier to review. > > > A suggestion was made to split it up the way it is now, so that's what > he > did. > > > > > If at all possible, I would like to go with the current approach since > we > have > > a > > > lot of reviewers and it would be nice to not have to have more > back-and-forth > > > restarts of the cl. > > > > > > Other reviewers: wdyt? > > > > can you point me to which comment (as you mention this has gone on for a > long > > time so there are too many comments to look through)? While my intent is > not > to > > slow this down, it really makes things hard to review this when it's in > > isolation. > > > > i.e. right now I see this is adding type convertors. but i don't see how > they'll > > be called. type convertors are an old concept, and the preferred > mechanism now > > is type maps. see > > https://www.chromium.org/developers/design-documents/mojo/type-mapping > > > > it's also unfortunate to see media_types.mojom which duplicates other > structs > in > > media. the goal should be to write these structs once (in mojoms). Then > other > > code in media can use them, even if they're not doing IPC. This avoids > duplicate > > structs and conversions. > > The request for splitting was in an off-review mail or chat rchtara@ had > with > one of the reviewers. The positive confirmation is in > https://codereview.chromium.org/1896883002/#msg35 > and > https://codereview.chromium.org/1896883002/#msg33 > > The usage of this interface is in the CL linked in the description. > > https://codereview.chromium.org/1896883002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/05/10 07:51:43, tommi-chrömium wrote: > Miguel and Dale, any thoughts for or against splitting? > > On Tue, May 10, 2016, 08:07 <mailto:grunell@chromium.org> wrote: > > > On 2016/05/09 20:26:32, jam wrote: > > > On 2016/05/09 16:54:54, tommi-chrömium wrote: > > > > On 2016/05/09 16:21:00, jam wrote: > > > > > On 2016/05/09 15:12:45, rchtara wrote: > > > > > > Thanks a lot guys for the reviews. > > > > > > OK, I have removed render_frame_id from this CL and will work too > > on > > > > removing > > > > > it > > > > > > from the other CL (1930393002) > > https://codereview.chromium.org/1930393002. > > > > > > @miu: thanks for filing a bug for removing dead bits_per_sample. > > > > > > > > > > per my earlier comment: this change is too small to land. what's the > > point > > > of > > > > > trying to land a mojo interface alone? combine it with change that > > uses it > > > > > please, that makes things clearer to review. > > > > > > > > There was a discussion in the review earlier where rchtara was asked to > > split > > > > the cl up into two smaller CLs to make them easier to review. > > > > A suggestion was made to split it up the way it is now, so that's what > > he > > did. > > > > > > > If at all possible, I would like to go with the current approach since > > we > > have > > > a > > > > lot of reviewers and it would be nice to not have to have more > > back-and-forth > > > > restarts of the cl. > > > > > > > > Other reviewers: wdyt? > > > > > > can you point me to which comment (as you mention this has gone on for a > > long > > > time so there are too many comments to look through)? While my intent is > > not > > to > > > slow this down, it really makes things hard to review this when it's in > > > isolation. > > > > > > i.e. right now I see this is adding type convertors. but i don't see how > > they'll > > > be called. type convertors are an old concept, and the preferred > > mechanism now > > > is type maps. see > > > https://www.chromium.org/developers/design-documents/mojo/type-mapping > > > > > > it's also unfortunate to see media_types.mojom which duplicates other > > structs > > in > > > media. the goal should be to write these structs once (in mojoms). Then > > other > > > code in media can use them, even if they're not doing IPC. This avoids > > duplicate > > > structs and conversions. > > > > The request for splitting was in an off-review mail or chat rchtara@ had > > with > > one of the reviewers. The positive confirmation is in > > https://codereview.chromium.org/1896883002/#msg35 > > and > > https://codereview.chromium.org/1896883002/#msg33 > > > > The usage of this interface is in the CL linked in the description. > > > > https://codereview.chromium.org/1896883002/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I did agree with the splitting (suggested, too), so that the author(s) would not need to deal with conflicting sets of reviewers (i.e. content/, media/, security etc). Since the split was two-way I think is relatively straightforward to find uses of what new mojo interfaces. Alternatively, the bundled mega-CL could be kept for reference.
On 2016/05/10 14:41:44, mcasas wrote: > On 2016/05/10 07:51:43, tommi-chrömium wrote: > > Miguel and Dale, any thoughts for or against splitting? > > > > On Tue, May 10, 2016, 08:07 <mailto:grunell@chromium.org> wrote: > > > > > On 2016/05/09 20:26:32, jam wrote: > > > > On 2016/05/09 16:54:54, tommi-chrömium wrote: > > > > > On 2016/05/09 16:21:00, jam wrote: > > > > > > On 2016/05/09 15:12:45, rchtara wrote: > > > > > > > Thanks a lot guys for the reviews. > > > > > > > OK, I have removed render_frame_id from this CL and will work too > > > on > > > > > removing > > > > > > it > > > > > > > from the other CL (1930393002) > > > https://codereview.chromium.org/1930393002. > > > > > > > @miu: thanks for filing a bug for removing dead bits_per_sample. > > > > > > > > > > > > per my earlier comment: this change is too small to land. what's the > > > point > > > > of > > > > > > trying to land a mojo interface alone? combine it with change that > > > uses it > > > > > > please, that makes things clearer to review. > > > > > > > > > > There was a discussion in the review earlier where rchtara was asked to > > > split > > > > > the cl up into two smaller CLs to make them easier to review. > > > > > A suggestion was made to split it up the way it is now, so that's what > > > he > > > did. > > > > > > > > > If at all possible, I would like to go with the current approach since > > > we > > > have > > > > a > > > > > lot of reviewers and it would be nice to not have to have more > > > back-and-forth > > > > > restarts of the cl. > > > > > > > > > > Other reviewers: wdyt? > > > > > > > > can you point me to which comment (as you mention this has gone on for a > > > long > > > > time so there are too many comments to look through)? While my intent is > > > not > > > to > > > > slow this down, it really makes things hard to review this when it's in > > > > isolation. > > > > > > > > i.e. right now I see this is adding type convertors. but i don't see how > > > they'll > > > > be called. type convertors are an old concept, and the preferred > > > mechanism now > > > > is type maps. see > > > > https://www.chromium.org/developers/design-documents/mojo/type-mapping > > > > > > > > it's also unfortunate to see media_types.mojom which duplicates other > > > structs > > > in > > > > media. the goal should be to write these structs once (in mojoms). Then > > > other > > > > code in media can use them, even if they're not doing IPC. This avoids > > > duplicate > > > > structs and conversions. > > > > > > The request for splitting was in an off-review mail or chat rchtara@ had > > > with > > > one of the reviewers. The positive confirmation is in > > > https://codereview.chromium.org/1896883002/#msg35 > > > and > > > https://codereview.chromium.org/1896883002/#msg33 > > > > > > The usage of this interface is in the CL linked in the description. > > > > > > https://codereview.chromium.org/1896883002/ > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > > email to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > I did agree with the splitting (suggested, too), so that > the author(s) would not need to deal with conflicting > sets of reviewers (i.e. content/, media/, security etc). > Since the split was two-way I think is relatively > straightforward to find uses of what new mojo interfaces. > Alternatively, the bundled mega-CL could be kept for > reference. OK for the record I don't think it's useful to send changes with just mojoms for the reasons I outlined earlier, but if you guys feel strongly about it I'll defer to you. However I still have outstanding comments about duplication of structs. One of the goals of mojo is to have less duplication, and having mojom duplicate C++ structs is making things worse. There are two solutions here: 1) (per my earlier message) define the structure once in Mojo. this is the canonical way moving forward. 2) if these interfaces are only used in C++, then the existing C++ structs can be sent over mojo by reusing the IPC::ParamTraits. See the bottom of https://www.chromium.org/developers/design-documents/mojo/type-mapping.
@jam: as Tommi said, the reason why the cl was split the cl is that some reviews pinged me to do so: is that many people reviewer The other part is https://codereview.chromium.org/1930393002 If at all possible, I would like please to continue with the current approach. Concerning the type mapping, I asked a question regarding that in the chromium-mojo mailing list a few weeks ago and was told: "I notice that AudioParameters has a vector member. We haven't supported custom mapping for vector/array yet, but will soon work on it." See please comment 2 for more details https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f... My internship is going to end soon (in less 4 weeks) so there no way for me to wait for the mapping to start supporting vectors. And this is the first cl in my internship project and I plan to land at least two bigger and more important ones. @everyone: the plan we are going to have for the Mojofication of audio rendering project is the following : 1) Create Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo. 2) Switch stream creation and closing in Chrome audio rendering from IPC to Mojo. 3) Switch all other stream operations (request device authorization, play, setVolume, pause) from IPC to Mojo. 4) Refactor: merge audio_renderer_host.cc and audio_output_impl.cc and merge audio_message_filter.cc and audio_output_client.cc. Remove the stream_id for the Createstream arguments.
There seems to be consensus now, but so my position is clear: lets avoid the intern run-around for once and keep the split :) Speaking for myself, I can close my eyes and see most of this code, so it's not that problematic to review as a split. I'm sure it's a similar position for tommi@ and the if the others aren't there yet they aren't far behind :p
https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/320001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:26: int32 render_frame_id, Cool! we are going to remove the stream_ids. After discussing with Grunell, we made a plan for the project: 1) Create Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo. 2) Switch stream creation and closing in Chrome audio rendering from IPC to Mojo. 3) Switch all other stream operations (request device authorization, play, setVolume, pause) from IPC to Mojo. 4)Refactor: merge audio_renderer_host.cc and audio_output_impl.cc and merge audio_message_filter.cc and audio_output_client.cc. Remove the stream_id for the Createstream arguments, generate it in the browser and return it to the renderer (it's going to be just used for logging). As play and pause stream are not going to be mojofied until the step 3, they will still rely on having the same stream_id in the renderer and the browser. So we need to keep stream_id the same until we mojofy them.(Otherwise, after the cl for the step 2 will land and before the cl for the step 3 will land, because at this time create and close stream are going to be mojofied while play is going to be still ipc based. So if the stream_ids in the renderer and browser are not going to be the same, we are going to have trouble playing the right stream when play ipc come from the renderer to the browser) So, this why we are going to do that in the refactoring step: step 4. Let's keep the interface like that temporarily and remove stream_ids later. https://codereview.chromium.org/1896883002/diff/360001/media/mojo/interfaces/... File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1896883002/diff/360001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:204: int32 bits_per_sample; great, I will do that in the second cl. https://codereview.chromium.org/1896883002/diff/360001/media/mojo/interfaces/... media/mojo/interfaces/media_types.mojom:204: int32 bits_per_sample; great
media_types.mojom lgtm
On 2016/05/10 15:47:44, rchtara wrote: > @jam: as Tommi said, the reason why the cl was split the cl is that some > reviews pinged me to do so: is that many people reviewer > The other part is https://codereview.chromium.org/1930393002 > If at all possible, I would like please to continue with the current approach. > > Concerning the type mapping, I asked a question regarding that in the > chromium-mojo mailing list a few weeks ago and was told: > "I notice that AudioParameters has a vector member. We haven't supported custom > mapping for vector/array yet, but will soon work on it." > See please comment 2 for more details > https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f... Type mapping is useful if you are calling the interface from say blink and content (where you want each to use a different struct that uses std::string or WTF::String) or C++ and JS. However in this case you have one struct (media::AudioParameters) that is only sent from C++ code. Since you don't want to define the struct in Mojo per the link above, you can use option 2 from my earlier comment. This reuses the existing IPC::ParamTraits for media::AudioParameters (in content/common/media/media_param_traits.h). https://www.chromium.org/developers/design-documents/mojo/type-mapping has information on this at the bottom. > > My internship is going to end soon (in less 4 weeks) so there no way for me to > wait for the mapping to start supporting vectors. And this is the first cl in my > internship project and I plan to land at least two bigger and more important > ones. > > > @everyone: the plan we are going to have for the Mojofication of audio rendering > project is the following : > > 1) Create Mojo interfaces needed for switching audio rendering stream creation > and closing from IPC to Mojo. > 2) Switch stream creation and closing in Chrome audio rendering from IPC to > Mojo. > 3) Switch all other stream operations (request device authorization, play, > setVolume, pause) from IPC to Mojo. > 4) Refactor: merge audio_renderer_host.cc and audio_output_impl.cc and merge > audio_message_filter.cc and audio_output_client.cc. Remove the stream_id for the > Createstream arguments.
On 2016/05/10 18:04:13, jam wrote: > On 2016/05/10 15:47:44, rchtara wrote: > > @jam: as Tommi said, the reason why the cl was split the cl is that some > > reviews pinged me to do so: is that many people reviewer > > The other part is https://codereview.chromium.org/1930393002 > > If at all possible, I would like please to continue with the current approach. > > > > Concerning the type mapping, I asked a question regarding that in the > > chromium-mojo mailing list a few weeks ago and was told: > > "I notice that AudioParameters has a vector member. We haven't supported > custom > > mapping for vector/array yet, but will soon work on it." > > See please comment 2 for more details > > > https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f... > > Type mapping is useful if you are calling the interface from say blink and > content (where you want each to use a different struct that uses std::string or > WTF::String) or C++ and JS. However in this case you have one struct > (media::AudioParameters) that is only sent from C++ code. Since you don't want > to define the struct in Mojo per the link above, you can use option 2 from my > earlier comment. This reuses the existing IPC::ParamTraits for > media::AudioParameters (in content/common/media/media_param_traits.h). > https://www.chromium.org/developers/design-documents/mojo/type-mapping has > information on this at the bottom. Thanks for the pointer. Yes, it's only C++. To be clear: the existing vector in AudioParameters will not be a problem with this option? > > > > My internship is going to end soon (in less 4 weeks) so there no way for me to > > wait for the mapping to start supporting vectors. And this is the first cl in > my > > internship project and I plan to land at least two bigger and more important > > ones. > > > > > > @everyone: the plan we are going to have for the Mojofication of audio > rendering > > project is the following : > > > > 1) Create Mojo interfaces needed for switching audio rendering stream creation > > and closing from IPC to Mojo. > > 2) Switch stream creation and closing in Chrome audio rendering from IPC to > > Mojo. > > 3) Switch all other stream operations (request device authorization, play, > > setVolume, pause) from IPC to Mojo. > > 4) Refactor: merge audio_renderer_host.cc and audio_output_impl.cc and merge > > audio_message_filter.cc and audio_output_client.cc. Remove the stream_id for > the > > Createstream arguments.
On 2016/05/10 15:57:29, DaleCurtis_OOO_Until_May_16 wrote: > There seems to be consensus now, but so my position is clear: lets avoid the > intern run-around for once and keep the split :) Speaking for myself, I can > close my eyes and see most of this code, so it's not that problematic to review > as a split. I'm sure it's a similar position for tommi@ and the if the others > aren't there yet they aren't far behind :p OK, everyone is OK with the split . Let's keep it.
On 2016/05/10 20:30:51, Henrik Grunell wrote: > On 2016/05/10 18:04:13, jam wrote: > > On 2016/05/10 15:47:44, rchtara wrote: > > > @jam: as Tommi said, the reason why the cl was split the cl is that some > > > reviews pinged me to do so: is that many people reviewer > > > The other part is https://codereview.chromium.org/1930393002 > > > If at all possible, I would like please to continue with the current > approach. > > > > > > Concerning the type mapping, I asked a question regarding that in the > > > chromium-mojo mailing list a few weeks ago and was told: > > > "I notice that AudioParameters has a vector member. We haven't supported > > custom > > > mapping for vector/array yet, but will soon work on it." > > > See please comment 2 for more details > > > > > > https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f... > > > > Type mapping is useful if you are calling the interface from say blink and > > content (where you want each to use a different struct that uses std::string > or > > WTF::String) or C++ and JS. However in this case you have one struct > > (media::AudioParameters) that is only sent from C++ code. Since you don't want > > to define the struct in Mojo per the link above, you can use option 2 from my > > earlier comment. This reuses the existing IPC::ParamTraits for > > media::AudioParameters (in content/common/media/media_param_traits.h). > > https://www.chromium.org/developers/design-documents/mojo/type-mapping has > > information on this at the bottom. > > Thanks for the pointer. Yes, it's only C++. > > To be clear: the existing vector in AudioParameters will not be a problem with > this option? Correct. Since rchtara has already spent a lot of time on this, I'm making an example of this cl that uses ParamTraits. It turns out that it needed a bunch of changes in content, which I'm making. I'll upload it shortly.
ok here's a version of this patch using paramtraits so that the media class is reused: https://codereview.chromium.org/1970643002/ I had to make a bunch of unrelated changes to make content and its dependencies support sharing param traits with mojo. I've split those changes into https://codereview.chromium.org/1966983003 and I'll land that. After you can update this cl from my first link. Sounds good?
On 2016/05/11 05:39:15, jam wrote: > ok here's a version of this patch using paramtraits so that the media class is > reused: https://codereview.chromium.org/1970643002/ > > I had to make a bunch of unrelated changes to make content and its dependencies > support sharing param traits with mojo. I've split those changes into > https://codereview.chromium.org/1966983003 and I'll land that. After you can > update this cl from my first link. Sounds good? Thanks a lot John! That's very appreciated. It sounds good to me.
Awesome, thanks for your help jam. Unfortunately, Dale told me that we need to move the interface back to media. (In a chat: Dale: You removed the render frame id from the .mojom, do we still need the interface to live in content? Me: why not? Dale: because it fits more with the current push to fragment the content layer more. and media contains almost all the code currently, and in the future will contain all of the code. the gpu team most recently removed all gpu code from content and created media/gpu for this purpose. so it makes more sense to keep what we can in media/
@jam: correct me please if I'm wrong: I noticed that media_param_traits.h which is in content is used in audio_parameters.typemap. So if we move the interfaces to media we will break the dependency rule which says that we cannot include things from the content to media. Right? So we probably cannot move the interfaces to media? Thanks https://codereview.chromium.org/1970643002/patch/80001/90015
On 2016/05/11 09:31:21, rchtara wrote: > @jam: correct me please if I'm wrong: I noticed that media_param_traits.h which > is in content is used in audio_parameters.typemap. So if we move the interfaces > to media we will break the dependency rule which says that we cannot include > things from the content to media. Right? > So we probably cannot move the interfaces to media? > Thanks > https://codereview.chromium.org/1970643002/patch/80001/90015 hey, sorry I meant to add some comments to the cl. Yes the interface should be in /media. That can be done by moving content/common/media/media_param_traits.* to media/base/ipc, similar to how param traits were already moved from content/common to media/gpu/ipc. Once those are moved, the typemap and new interfaces can be in media/mojo/interfaces. I didn't want to make this change bigger by moving these files, which are depended on by the typemap as you point out.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1896883002/diff/520001/media/mojo/common/medi... File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/1896883002/diff/520001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:375: media::interfaces::AudioParametersPtr TypeConverter< Can we use StructTraits for this instead of adding new type converters?
https://codereview.chromium.org/1896883002/diff/520001/media/mojo/common/medi... File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/1896883002/diff/520001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:375: media::interfaces::AudioParametersPtr TypeConverter< On 2016/05/11 17:15:05, dcheng wrote: > Can we use StructTraits for this instead of adding new type converters? this should go away ( see my earlier comments)
https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:20: // TODO(rchtara): Add method to request a device authorization to this Nit: "request device authorization" https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:34: CloseStream(int32 stream_id); I suppose there should only be one way of closing the stream? Here or in AudioOutputStream. If this really will be needed for the Authorization case, let's add it when authorization is added. I.e. remove CloseStream for now. https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... File media/mojo/interfaces/audio_parameters.mojom (right): https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... media/mojo/interfaces/audio_parameters.mojom:5: module media.interfaces; In audio_output.mojom, the module is media.mojom. Should they be the same? https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... File media/mojo/interfaces/mojo_bindings.gyp (right): https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:8: 'target_name': 'audio_output_mojom', I'm still confused by that the other parts in this file have targets called "xyz_mojom_bindings" with "type: none" and that contain .mojom files. Then a target "xyz_api" which depends on the bindings target. But here the naming is the other way around. It seems like this target should be called "audio_output_mojom_bindings" and the other "audio_output_<something suitable>". What <something suitable> should be I don't know. "api" as the others? Can someone with mojo experience suggest?
Description was changed from ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * AudioOutputStreamParameters Mojo structure used by AudioOuput. * AudioOutputStreamParameters/AudioParameters converters needed later. This is the media part: The content part is in the CL http://codereview.chromium.org/1930393002 BUG=606707 ========== to ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. This is the media part: The content part is in the CL http://codereview.chromium.org/1930393002 BUG=606707 ==========
You don't need any of the content changes in this cl, so remove them. When you do need to depend on the typemap in content, it'll just be the content/common target that has the dependency. You won't need it in browser/renderer https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... File media/mojo/interfaces/mojo_bindings.gyp (right): https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:8: 'target_name': 'audio_output_mojom', On 2016/05/12 10:34:37, Henrik Grunell wrote: > I'm still confused by that the other parts in this file have targets called > "xyz_mojom_bindings" with "type: none" and that contain .mojom files. Then a > target "xyz_api" which depends on the bindings target. But here the naming is > the other way around. > > It seems like this target should be called "audio_output_mojom_bindings" and the > other "audio_output_<something suitable>". What <something suitable> should be I > don't know. "api" as the others? > > Can someone with mojo experience suggest? the way it is now is consistent with other gyp files (although they're not all consistent :( ). i.e. see https://code.google.com/p/chromium/codesearch#chromium/src/components/leveldb... This ugliness will go away once we leave gyp. But in the meantime, we need this so that targets which need the generated headers, but don't want to actually link with them, can have something to depend on.
and to avoid timezone delays, lgtm with removing content/ changes.
question for media folks in general: for other structs that were added in media_types.mojom, can they also be removed in favor of typemaps to the C++ type to avoid duplication?
@jam: Thanks a lot for your help and for quickly making and landing cls 1968343002 and 1966983003. I had a problem with the android build bots including: https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp... I added many "//media/base/ipc" to many buid.gn files. android_compile_dbg has finished compiling right now which is great. But I'm not sure: I worried that added too many of buid.gn. Could you please have look and tell me if everything is fine?
On 2016/05/12 16:44:50, rchtara wrote: > @jam: Thanks a lot for your help and for quickly making and landing cls > 1968343002 and 1966983003. > I had a problem with the android build bots including: > https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp... > I added many "//media/base/ipc" to many buid.gn files. android_compile_dbg has > finished compiling right now which is great. But I'm not sure: I worried that > added too many of buid.gn. > Could you please have look and tell me if everything is fine? you need a deps = [ "//media/base/ipc" ] line in the typemap. see https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-St...
https://codereview.chromium.org/1896883002/diff/520001/media/mojo/common/medi... File media/mojo/common/media_type_converters.cc (right): https://codereview.chromium.org/1896883002/diff/520001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:375: media::interfaces::AudioParametersPtr TypeConverter< On 2016/05/11 17:15:05, dcheng wrote: > Can we use StructTraits for this instead of adding new type converters? Done. https://codereview.chromium.org/1896883002/diff/520001/media/mojo/common/medi... media/mojo/common/media_type_converters.cc:375: media::interfaces::AudioParametersPtr TypeConverter< On 2016/05/11 17:31:48, jam wrote: > On 2016/05/11 17:15:05, dcheng wrote: > > Can we use StructTraits for this instead of adding new type converters? > > this should go away ( see my earlier comments) Done. https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... File media/mojo/interfaces/audio_output.mojom (right): https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:20: // TODO(rchtara): Add method to request a device authorization to this On 2016/05/12 10:34:37, Henrik Grunell wrote: > Nit: "request device authorization" Done. https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... media/mojo/interfaces/audio_output.mojom:34: CloseStream(int32 stream_id); After a discussion with grunell: we decided to keep temporally the ipc for closing the streams that are authorized and not created yet : so we don't have to add the AudioOutpu.CloseStream. These streams will be closed using AudioOutputStream.Close later when RequestDeviceAuthorization will be mojofied https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... File media/mojo/interfaces/audio_parameters.mojom (right): https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... media/mojo/interfaces/audio_parameters.mojom:5: module media.interfaces; On 2016/05/12 10:34:37, Henrik Grunell wrote: > In audio_output.mojom, the module is media.mojom. Should they be the same? According to mcasas, yes . So i renamed it https://codereview.chromium.org/1896883002/#msg33 https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... File media/mojo/interfaces/mojo_bindings.gyp (right): https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:8: 'target_name': 'audio_output_mojom', On 2016/05/12 16:38:51, jam wrote: > On 2016/05/12 10:34:37, Henrik Grunell wrote: > > I'm still confused by that the other parts in this file have targets called > > "xyz_mojom_bindings" with "type: none" and that contain .mojom files. Then a > > target "xyz_api" which depends on the bindings target. But here the naming is > > the other way around. > > > > It seems like this target should be called "audio_output_mojom_bindings" and > the > > other "audio_output_<something suitable>". What <something suitable> should be > I > > don't know. "api" as the others? > > > > Can someone with mojo experience suggest? > > the way it is now is consistent with other gyp files (although they're not all > consistent :( ). i.e. see > https://code.google.com/p/chromium/codesearch#chromium/src/components/leveldb... > > This ugliness will go away once we leave gyp. But in the meantime, we need this > so that targets which need the generated headers, but don't want to actually > link with them, can have something to depend on. Done. https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:8: 'target_name': 'audio_output_mojom', On 2016/05/12 10:34:37, Henrik Grunell wrote: > I'm still confused by that the other parts in this file have targets called > "xyz_mojom_bindings" with "type: none" and that contain .mojom files. Then a > target "xyz_api" which depends on the bindings target. But here the naming is > the other way around. > > It seems like this target should be called "audio_output_mojom_bindings" and the > other "audio_output_<something suitable>". What <something suitable> should be I > don't know. "api" as the others? > > Can someone with mojo experience suggest? Done.
On 2016/05/12 17:13:38, jam wrote: > On 2016/05/12 16:44:50, rchtara wrote: > > @jam: Thanks a lot for your help and for quickly making and landing cls > > 1968343002 and 1966983003. > > I had a problem with the android build bots including: > > > https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp... > > I added many "//media/base/ipc" to many buid.gn files. android_compile_dbg has > > finished compiling right now which is great. But I'm not sure: I worried that > > added too many of buid.gn. > > Could you please have look and tell me if everything is fine? > > you need a > > deps = [ "//media/base/ipc" ] > > line in the typemap. see > https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-St... Awesome it's working now. Thanks a lot jam
Thanks a lot for your patience on this Riadh! It's been a lot of reviewing involved here and many different aspects to consider. lgtm.
https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... File media/mojo/interfaces/mojo_bindings.gyp (right): https://codereview.chromium.org/1896883002/diff/540001/media/mojo/interfaces/... media/mojo/interfaces/mojo_bindings.gyp:8: 'target_name': 'audio_output_mojom', On 2016/05/12 10:34:37, Henrik Grunell wrote: > I'm still confused by that the other parts in this file have targets called > "xyz_mojom_bindings" with "type: none" and that contain .mojom files. Then a > target "xyz_api" which depends on the bindings target. But here the naming is > the other way around. > > It seems like this target should be called "audio_output_mojom_bindings" and the > other "audio_output_<something suitable>". What <something suitable> should be I > don't know. "api" as the others? > > Can someone with mojo experience suggest? As jam: >the way it is now is consistent with other gyp files (although they're not all consistent
IPC LGTM.
lgtm
media/mojo lgtm
Description was changed from ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. This is the media part: The content part is in the CL http://codereview.chromium.org/1930393002 BUG=606707 ========== to ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * Type mapping for AudioParameters. This is the media part: The content part is in the CL http://codereview.chromium.org/1930393002 BUG=606707 ==========
The CQ bit was checked by rchtara@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1896883002/#ps780001 (title: "rename media.interfaces - media.mojom")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1896883002/780001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1896883002/780001
Message was sent while issue was closed.
Description was changed from ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * Type mapping for AudioParameters. This is the media part: The content part is in the CL http://codereview.chromium.org/1930393002 BUG=606707 ========== to ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * Type mapping for AudioParameters. This is the media part: The content part is in the CL http://codereview.chromium.org/1930393002 BUG=606707 ==========
Message was sent while issue was closed.
Committed patchset #40 (id:780001)
Message was sent while issue was closed.
Description was changed from ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * Type mapping for AudioParameters. This is the media part: The content part is in the CL http://codereview.chromium.org/1930393002 BUG=606707 ========== to ========== Mojo interfaces needed for switching audio rendering stream creation and closing from IPC to Mojo This adds: * AudioOutput which allows managing audio output streams. * AudioOutputStream which handles audio output stream operations. * Type mapping for AudioParameters. This is the media part: The content part is in the CL http://codereview.chromium.org/1930393002 BUG=606707 Committed: https://crrev.com/78870627c0044f56533da4746e4b3231d04d87f1 Cr-Commit-Position: refs/heads/master@{#393537} ==========
Message was sent while issue was closed.
Patchset 40 (id:??) landed as https://crrev.com/78870627c0044f56533da4746e4b3231d04d87f1 Cr-Commit-Position: refs/heads/master@{#393537}
Message was sent while issue was closed.
Message was sent while issue was closed.
Please do not commit CLs that only contain mojo interface files. Each interface must be committed along with its implementation, so the security of it can be reviewed.
Message was sent while issue was closed.
On 2016/05/25 17:39:31, nasko (slow) wrote: > Please do not commit CLs that only contain mojo interface files. Each interface > must be committed along with its implementation, so the security of it can be > reviewed. This is good to know. It has been through security review however (mkwst@) and approved, so security reviewers may need to sync up on how to handle this. |
