Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 2 months ago by aleloi Modified:
4 years, 2 months ago Reviewers:
the sun, hlundin-webrtc, kjellander_webrtc, kwiberg-webrtc CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), hlundin-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSplit audio mixer into interface and implementation.
The AudioMixer is now split in a mixer and audio source interface part, which has moved to webrtc/api, and a default implementation part, which lies in webrtc/modules.
This change makes it possible to create other mixer implementations and is a first step to facilitate passing down a mixer from outside of WebRTC.
It will also create less build dependencies when the new mixer has replaced the old one.
NOTRY=True
TBR=henrik.lundin@webrtc.org
BUG=webrtc:6346
Committed: https://crrev.com/201dfe90a7b77eb97d940331ba367e2927e910c2
Cr-Commit-Position: refs/heads/master@{#14705}
Patch Set 1 : visibility #
Total comments: 14
Patch Set 2 : Reorder includes. #
Total comments: 5
Patch Set 3 : Created an 'audio_frame_manipulator' target. #
Total comments: 3
Patch Set 4 : Changed visibility of audio_frame_manipulator. #
Total comments: 1
Patch Set 5 : Rebase. #
Total comments: 11
Patch Set 6 : Comments, spelling and capitalization of virtual methods. #
Total comments: 2
Patch Set 7 : Capitalize Ssrc #
Total comments: 6
Created: 4 years, 2 months ago
Dependent Patchsets: Messages
Total messages: 41 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
aleloi@webrtc.org changed reviewers: + kjellander@webrtc.org, solenberg@webrtc.org
Hi! Could you please take a look at this? In this CL, the audio mixer has been split into an interface and an implementation.
Description was changed from ========== Split audio mixer into interface and implementation. BUG=webrtc:6346 ========== to ========== Split audio mixer into interface and implementation. BUG=webrtc:6346 ==========
aleloi@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/BUILD.gn#newco... webrtc/api/BUILD.gn:322: "audio/audio_mixer.h", The mixer has a dependency on AudioFrame, which is in module_common_types.h. That should probably be added here as well, otherwise the dependency chain isn't complete? In fact, you should probably make a CL before this one, which pulls AudioFrame out and puts it in api/audio/ - that will be helpful for moving e.g. AudioProcessing into api/ as well. https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/audio/audio_mi... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/audio/audio_mi... webrtc/api/audio/audio_mixer.h:17: #include "webrtc/modules/include/module_common_types.h" This should be a forbidden inclusion? https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/BUILD.gn (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/BUILD.gn:13: "../../modules/*", Does this mean it is visible to webrtc/modules/? Why is that necessary? https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:20: #include "webrtc/api/audio/audio_mixer.h" order https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc:19: #include "webrtc/api/audio/audio_mixer.h" order
Replies to comments and questions. https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/BUILD.gn#newco... webrtc/api/BUILD.gn:322: "audio/audio_mixer.h", On 2016/10/12 15:26:10, the sun wrote: > The mixer has a dependency on AudioFrame, which is in module_common_types.h. > That should probably be added here as well, otherwise the dependency chain isn't > complete? > > In fact, you should probably make a CL before this one, which pulls AudioFrame > out and puts it in api/audio/ - that will be helpful for moving e.g. > AudioProcessing into api/ as well. There is no GN target yet for module_common_types.h. I can make one for AudioFrame as you suggested and put in in api/audio! https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/audio/audio_mi... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/audio/audio_mi... webrtc/api/audio/audio_mixer.h:17: #include "webrtc/modules/include/module_common_types.h" On 2016/10/12 15:26:10, the sun wrote: > This should be a forbidden inclusion? No, since GN currently doesn't know about module_common_types.h https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/BUILD.gn (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/BUILD.gn:13: "../../modules/*", On 2016/10/12 15:26:10, the sun wrote: > Does this mean it is visible to webrtc/modules/? Why is that necessary? The default visibility is that everything is visible everywhere. Since this is an interior implementation, I wanted to forbid depending on it. The current plan is to create a mixer in audio_state (which is in //webrtc/audio:audio). A mixer is also created in //webrtc/modules:modules_unittests. Additionally, there is a big target that contains everything, //:root and //webrtc:webrtc. It also contains the mixer. This target is named differently for different platforms, and setting visibility to just //webrtc/modules:modules didn't compile. Therefore, I set visibility to modules/*. https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:20: #include "webrtc/api/audio/audio_mixer.h" On 2016/10/12 15:26:10, the sun wrote: > order Thanks, done in upcoming patch set! https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc:19: #include "webrtc/api/audio/audio_mixer.h" On 2016/10/12 15:26:10, the sun wrote: > order Done in next patch set.
lgtm with nits addressed. https://codereview.webrtc.org/2411313003/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/BUILD.gn (right): https://codereview.webrtc.org/2411313003/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/BUILD.gn:12: visibility = [ Nice to see this being used. It's probably something we should put more widely into use along the Slim and modular effort (and put into the design doc). https://codereview.webrtc.org/2411313003/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/BUILD.gn:14: "../../audio:audio", sort https://codereview.webrtc.org/2411313003/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer.gypi (right): https://codereview.webrtc.org/2411313003/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.gypi:18: '<(webrtc_root)/api/api.gyp:audio_mixer_api', sort
lgtm https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/BUILD.gn#newco... webrtc/api/BUILD.gn:322: "audio/audio_mixer.h", On 2016/10/12 15:36:02, aleloi wrote: > On 2016/10/12 15:26:10, the sun wrote: > > The mixer has a dependency on AudioFrame, which is in module_common_types.h. > > That should probably be added here as well, otherwise the dependency chain > isn't > > complete? > > > > In fact, you should probably make a CL before this one, which pulls AudioFrame > > out and puts it in api/audio/ - that will be helpful for moving e.g. > > AudioProcessing into api/ as well. > > There is no GN target yet for module_common_types.h. I can make one for > AudioFrame as you suggested and put in in api/audio! I think what you need to do is pull AudioFrame out from module_common_types.h into its own .h, put that in api/audio/ and make a target for it. But it's ok if you do it in a follow-up. https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/audio/audio_mi... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/audio/audio_mi... webrtc/api/audio/audio_mixer.h:17: #include "webrtc/modules/include/module_common_types.h" On 2016/10/12 15:36:02, aleloi wrote: > On 2016/10/12 15:26:10, the sun wrote: > > This should be a forbidden inclusion? > > No, since GN currently doesn't know about module_common_types.h kjellander@, is there a way to set GN up so only certain files can be included from file in api/ ? https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/BUILD.gn (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/BUILD.gn:13: "../../modules/*", On 2016/10/12 15:36:02, aleloi wrote: > On 2016/10/12 15:26:10, the sun wrote: > > Does this mean it is visible to webrtc/modules/? Why is that necessary? > > The default visibility is that everything is visible everywhere. Since this is > an interior implementation, I wanted to forbid depending on it. The current plan > is to create a mixer in audio_state (which is in //webrtc/audio:audio). A mixer > is also created in //webrtc/modules:modules_unittests. Additionally, there is a > big target that contains everything, //:root and //webrtc:webrtc. It also > contains the mixer. This target is named differently for different platforms, > and setting visibility to just //webrtc/modules:modules didn't compile. > > Therefore, I set visibility to modules/*. Ah, I forgot about the unit test. It would be nice indeed if the unit test binary didn't need to pull in the .cc put could depend on a component-local lib containing the test, but perhaps that's not possible with gtest. :/
https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/audio/audio_mi... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2411313003/diff/40001/webrtc/api/audio/audio_mi... webrtc/api/audio/audio_mixer.h:17: #include "webrtc/modules/include/module_common_types.h" On 2016/10/13 07:55:00, the sun wrote: > On 2016/10/12 15:36:02, aleloi wrote: > > On 2016/10/12 15:26:10, the sun wrote: > > > This should be a forbidden inclusion? > > > > No, since GN currently doesn't know about module_common_types.h > > kjellander@, is there a way to set GN up so only certain files can be included > from file in api/ ? I can attempt to answer that: you can declare some of the header files 'public' by writing a 'public = [...]' section in the GN target. Then depending targets are only allowed to include the public headers. The problem is that GN only enforces that during 'gn check', and only targets listed in .gn are checked before committing. kjellander@, did I get something wrong?
I did run GN's include header checker on modules:modules_unittests and found a few errors in the mixer unit tests. To fix them, I declared 'audio_mixer_api' a public dependency of 'audio_mixer_impl' and broke out 'audio_frame_manipulator' to its own target. There are still lots of other include errors in modules_unittests. Could you please take a quick look at these changes as well? https://codereview.webrtc.org/2411313003/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/BUILD.gn (right): https://codereview.webrtc.org/2411313003/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/BUILD.gn:14: "../../audio:audio", On 2016/10/13 00:57:47, kjellander_webrtc wrote: > sort Done. https://codereview.webrtc.org/2411313003/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer.gypi (right): https://codereview.webrtc.org/2411313003/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer.gypi:18: '<(webrtc_root)/api/api.gyp:audio_mixer_api', On 2016/10/13 00:57:47, kjellander_webrtc wrote: > sort Done. https://codereview.webrtc.org/2411313003/diff/80001/webrtc/modules/BUILD.gn File webrtc/modules/BUILD.gn (right): https://codereview.webrtc.org/2411313003/diff/80001/webrtc/modules/BUILD.gn#n... webrtc/modules/BUILD.gn:631: "../test:test_support", This line reduced the output of "gn check <out_dir> webrtc/modules:modules_unittests" from about 4000 lines to about 800. https://codereview.webrtc.org/2411313003/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/BUILD.gn (right): https://codereview.webrtc.org/2411313003/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/BUILD.gn:29: ] This way, targets that create the mixer and include the api do not need to explicitly depend on it. Our situation seems to fit one of the cases described in "gn help public_deps": Generally if you are writing a target B and you include C's headers as part of B's public headers, or targets depending on B should consider B and C to be part of a unit, you should use public_deps instead of deps. (with B=audio_mixer_impl and C=audio_mixer_api) https://codereview.webrtc.org/2411313003/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/BUILD.gn:56: } The mixer directory contains a test file called audio_frame_manipulator_unittests.cc, which is part of the target modules:modules_unittests. The unit test includes audio_frame_manipulator.h, which was forbidden by GN's include header checker. It was forbidden, because :audio_mixer_impl only declared audio_mixer_impl.h to be a public header.
aleloi@webrtc.org changed reviewers: + kwiberg@webrtc.org
kwiberg@, can you take a look at this as an owner of webrtc/api?
https://codereview.webrtc.org/2411313003/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/BUILD.gn (right): https://codereview.webrtc.org/2411313003/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/BUILD.gn:44: "../../modules:*", On other platforms, the unittests are named differently: //webrtc/modules:modules_unittests__isolate__write_deps on Android.
webrtc/api looks good, but a few nits and a potentially bothersome suggestion. https://codereview.webrtc.org/2411313003/diff/140001/.gn File .gn (right): https://codereview.webrtc.org/2411313003/diff/140001/.gn#newcode23 .gn:23: "//webrtc/api:audio_mixer_api", Excellent! https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:31: kError // audio_frame will not be used. Nits: Comment indentation. Capitalize first word in all sentences. Comma after last enum too. https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:48: virtual AudioFrameWithInfo GetAudioFrameWithInfo(int sample_rate_hz) = 0; Is it too late to change this? Returning pointers to things that the caller may read (or worse, modify) for some time afterwards makes the code hard to reason about. One alternative that works in many cases is to take a callback (e.g. a FunctionView) that receives (a pointer to) the stuff as an argument; the callback may then manipulate the thing while it's running. However, in this case I guess it'd also be OK for the caller to pass an AudioFrame* to be filled in? https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:50: // A way for a mixer implementation do distinguish participants. to https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:51: virtual int ssrc() = 0; Nit: virtual methods should never be lower-case, because the interface can't guarantee that they'll be cheap to call.
I recommend a separate CL for Karl's suggested interface changes. https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:48: virtual AudioFrameWithInfo GetAudioFrameWithInfo(int sample_rate_hz) = 0; On 2016/10/14 07:52:22, kwiberg-webrtc wrote: > Is it too late to change this? Returning pointers to things that the caller may > read (or worse, modify) for some time afterwards makes the code hard to reason > about. > > One alternative that works in many cases is to take a callback (e.g. a > FunctionView) that receives (a pointer to) the stuff as an argument; the > callback may then manipulate the thing while it's running. > > However, in this case I guess it'd also be OK for the caller to pass an > AudioFrame* to be filled in? I think the latter suggestion is a good one. Then we could avoid the additional AudioFrame buffer inside voe::Channel and the mixer can be as dumb/clever as it wants juggling buffers (the simplest impl would just need two buffers).
https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:31: kError // audio_frame will not be used. On 2016/10/14 07:52:22, kwiberg-webrtc wrote: > Nits: Comment indentation. Capitalize first word in all sentences. Comma after > last enum too. Done. https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:48: virtual AudioFrameWithInfo GetAudioFrameWithInfo(int sample_rate_hz) = 0; On 2016/10/14 07:52:22, kwiberg-webrtc wrote: > Is it too late to change this? Returning pointers to things that the caller may > read (or worse, modify) for some time afterwards makes the code hard to reason > about. > > One alternative that works in many cases is to take a callback (e.g. a > FunctionView) that receives (a pointer to) the stuff as an argument; the > callback may then manipulate the thing while it's running. > > However, in this case I guess it'd also be OK for the caller to pass an > AudioFrame* to be filled in? I'm not sure I understand the implications of the callback suggestion. Is the main point to only allow reading/modifying the frame from inside the callback? At some point in the mixing algorithm, the mixer has to look at all frames to select which ones to use. But only one frame at a time would be available in the callback. The callback would then need to leak the frame pointers elswhere. This is how I understand the callback suggestion: Suppose GetAudioFrame takes a callback DoThisWithTheFrame, which takes a frame pointer. Mixer::GetAudio() { for each source: source.GetAudio(DoThisWithTheFrame) } DoThisWithTheFrame(frame_ptr) { add frame to mixer data structures } mix_result Mixer::Mix() { GetAudio(); select what frames to use from internal data structures mix frames together } https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:48: virtual AudioFrameWithInfo GetAudioFrameWithInfo(int sample_rate_hz) = 0; On 2016/10/14 08:41:04, the sun wrote: > On 2016/10/14 07:52:22, kwiberg-webrtc wrote: > > Is it too late to change this? Returning pointers to things that the caller > may > > read (or worse, modify) for some time afterwards makes the code hard to reason > > about. > > > > One alternative that works in many cases is to take a callback (e.g. a > > FunctionView) that receives (a pointer to) the stuff as an argument; the > > callback may then manipulate the thing while it's running. > > > > However, in this case I guess it'd also be OK for the caller to pass an > > AudioFrame* to be filled in? > > I think the latter suggestion is a good one. Then we could avoid the additional > AudioFrame buffer inside voe::Channel and the mixer can be as dumb/clever as it > wants juggling buffers (the simplest impl would just need two buffers). I agree that passing an AudioFrame* owned by the mixer to the source is a good simplification of the interface! It definitely becomes simpler. Here is how it can be implemented (it doesn't require any large changes). We already added an 'audio source wrapper' data structure currently called 'SourceStatus' which persists across mix iterations and keeps track of whether the source was mixed the last time. An AudioFrame can easily be added to it. The AudioFrame pointer is then valid from the point AddSource is called until RemoveSource is called on the same source. This change would move back to how (that part of) the mixer interface looked before the change in https://codereview.webrtc.org/2127763002/. The reason for the change in the first place was to remove the memory pool from the mixer without having to copy audio frames. There is one (solvable) problem though: I can't just add a non-pointer AudioFrame to SourceStatus, because the SourceStatuses are stored in a vector, and AudioFrame can't be copied. So the mixer would have to keep lots of AudioFrames somewhere else :( https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:50: // A way for a mixer implementation do distinguish participants. On 2016/10/14 07:52:22, kwiberg-webrtc wrote: > to Done. https://codereview.webrtc.org/2411313003/diff/140001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:51: virtual int ssrc() = 0; On 2016/10/14 07:52:22, kwiberg-webrtc wrote: > Nit: virtual methods should never be lower-case, because the interface can't > guarantee that they'll be cheap to call. Done.
https://codereview.webrtc.org/2411313003/diff/160001/webrtc/api/audio/audio_m... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2411313003/diff/160001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:21: class AudioMixer : public rtc::RefCountInterface { Add this comment before the class decl: // WORK IN PROGRESS // This class is under development and is not yet intended for for use outside // of WebRtc/Libjingle.
https://codereview.webrtc.org/2411313003/diff/160001/webrtc/api/audio/audio_m... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2411313003/diff/160001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:21: class AudioMixer : public rtc::RefCountInterface { On 2016/10/14 11:35:07, the sun wrote: > Add this comment before the class decl: > > // WORK IN PROGRESS > // This class is under development and is not yet intended for for use outside > // of WebRtc/Libjingle. Done.
Patchset #5 (id:120001) has been deleted
Patchset #7 (id:180001) has been deleted
Description was changed from ========== Split audio mixer into interface and implementation. BUG=webrtc:6346 ========== to ========== Split audio mixer into interface and implementation. The AudioMixer is now split in a mixer and audio source interface part, which has moved to webrtc/api, and a default implementation part, which lies in webrtc/modules. This change makes it possible to create other mixer implementations and is a first step to facilitate passing down a mixer from outside of WebRTC. It will also create less build dependencies when the new mixer has replaced the old one. BUG=webrtc:6346 ==========
Ping :) kwiberg@s suggestion to simplify the interface by not borrowing frames from audio sources is now implemented and approved in another CL. Does anyone have any further comments on this change?
still lgtm https://codereview.webrtc.org/2411313003/diff/200001/webrtc/api/audio/audio_m... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2411313003/diff/200001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:14: #include <memory> not needed? https://codereview.webrtc.org/2411313003/diff/200001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:75: // specify the rate and number of channels of the mix result. Why isn't the information in AudioFrame used? (Sorry, I feel I've asked this before)
lgtm https://codereview.webrtc.org/2411313003/diff/200001/webrtc/api/audio/audio_m... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2411313003/diff/200001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:21: // WORK IN PROGRESS Are we allowed to use Unicode in source files yet? If so, consider adding a 🚧 (U+1F6A7, "Construction Sign").
https://codereview.webrtc.org/2411313003/diff/200001/webrtc/api/audio/audio_m... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2411313003/diff/200001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:14: #include <memory> On 2016/10/20 09:13:05, the sun wrote: > not needed? There is a unique_ptr<AudioProcessing> argument to the constructor. https://codereview.webrtc.org/2411313003/diff/200001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:21: // WORK IN PROGRESS On 2016/10/20 09:24:12, kwiberg-webrtc wrote: > Are we allowed to use Unicode in source files yet? If so, consider adding a 🚧 > (U+1F6A7, "Construction Sign"). Hm... I wonder what that will do to peoples editors... I think I'll stick to convention and custom :) https://codereview.webrtc.org/2411313003/diff/200001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:75: // specify the rate and number of channels of the mix result. On 2016/10/20 09:13:05, the sun wrote: > Why isn't the information in AudioFrame used? (Sorry, I feel I've asked this > before) Yes, I also think that we had this discussion a while ago (can't remember where). It's for readability and simplicity: now AudioFrame is output-only. Instead of having to specify in the interface that some fields are valid and others are not, we just say that all fields of |audio_frame_...| must be overwritten.
Description was changed from ========== Split audio mixer into interface and implementation. The AudioMixer is now split in a mixer and audio source interface part, which has moved to webrtc/api, and a default implementation part, which lies in webrtc/modules. This change makes it possible to create other mixer implementations and is a first step to facilitate passing down a mixer from outside of WebRTC. It will also create less build dependencies when the new mixer has replaced the old one. BUG=webrtc:6346 ========== to ========== Split audio mixer into interface and implementation. The AudioMixer is now split in a mixer and audio source interface part, which has moved to webrtc/api, and a default implementation part, which lies in webrtc/modules. This change makes it possible to create other mixer implementations and is a first step to facilitate passing down a mixer from outside of WebRTC. It will also create less build dependencies when the new mixer has replaced the old one. TBR=henrik.lundin@webrtc.org BUG=webrtc:6346 ==========
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2411313003/#ps200001 (title: "Capitalize Ssrc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_drmemory_light on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
On 2016/10/20 11:01:44, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_drmemory_light on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...) Sorry about this, the dr memory bots are now removed. I just landed a CL to remove them from the CQ (https://codereview.webrtc.org/2434563003/) so you should be able to just try again (or add NOTRY=True if you don't want to wait, since I don't know how long it takes before the CQ picks up the new config)
Description was changed from ========== Split audio mixer into interface and implementation. The AudioMixer is now split in a mixer and audio source interface part, which has moved to webrtc/api, and a default implementation part, which lies in webrtc/modules. This change makes it possible to create other mixer implementations and is a first step to facilitate passing down a mixer from outside of WebRTC. It will also create less build dependencies when the new mixer has replaced the old one. TBR=henrik.lundin@webrtc.org BUG=webrtc:6346 ========== to ========== Split audio mixer into interface and implementation. The AudioMixer is now split in a mixer and audio source interface part, which has moved to webrtc/api, and a default implementation part, which lies in webrtc/modules. This change makes it possible to create other mixer implementations and is a first step to facilitate passing down a mixer from outside of WebRTC. It will also create less build dependencies when the new mixer has replaced the old one. NOTRY=True TBR=henrik.lundin@webrtc.org BUG=webrtc:6346 ==========
The CQ bit was checked by aleloi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Split audio mixer into interface and implementation. The AudioMixer is now split in a mixer and audio source interface part, which has moved to webrtc/api, and a default implementation part, which lies in webrtc/modules. This change makes it possible to create other mixer implementations and is a first step to facilitate passing down a mixer from outside of WebRTC. It will also create less build dependencies when the new mixer has replaced the old one. NOTRY=True TBR=henrik.lundin@webrtc.org BUG=webrtc:6346 ========== to ========== Split audio mixer into interface and implementation. The AudioMixer is now split in a mixer and audio source interface part, which has moved to webrtc/api, and a default implementation part, which lies in webrtc/modules. This change makes it possible to create other mixer implementations and is a first step to facilitate passing down a mixer from outside of WebRTC. It will also create less build dependencies when the new mixer has replaced the old one. NOTRY=True TBR=henrik.lundin@webrtc.org BUG=webrtc:6346 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Split audio mixer into interface and implementation. The AudioMixer is now split in a mixer and audio source interface part, which has moved to webrtc/api, and a default implementation part, which lies in webrtc/modules. This change makes it possible to create other mixer implementations and is a first step to facilitate passing down a mixer from outside of WebRTC. It will also create less build dependencies when the new mixer has replaced the old one. NOTRY=True TBR=henrik.lundin@webrtc.org BUG=webrtc:6346 ========== to ========== Split audio mixer into interface and implementation. The AudioMixer is now split in a mixer and audio source interface part, which has moved to webrtc/api, and a default implementation part, which lies in webrtc/modules. This change makes it possible to create other mixer implementations and is a first step to facilitate passing down a mixer from outside of WebRTC. It will also create less build dependencies when the new mixer has replaced the old one. NOTRY=True TBR=henrik.lundin@webrtc.org BUG=webrtc:6346 Committed: https://crrev.com/201dfe90a7b77eb97d940331ba367e2927e910c2 Cr-Commit-Position: refs/heads/master@{#14705} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/201dfe90a7b77eb97d940331ba367e2927e910c2 Cr-Commit-Position: refs/heads/master@{#14705} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
