Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2420913002:
Move audio frame memory handling inside AudioMixer. (Closed)
DescriptionMove audio frame memory handling inside AudioMixer.
Simplify the AudioMixer::Source interface and update the mixer
implementation to the new interface.
Instead of asking a mixer source to provide a pointer to an AudioFrame
during each mixing iteration, a mixer should supply a pointer to its
own AudioFrame.
This simplifies lifetime issues as sources do not give away an
internal pointer.
Implementation: when an audio source is added, the mixer allocates a
new AudioFrame. The audio frame is kept together in the internal class
SourceStatus together with the audio source pointer until the source
is removed.
NOTRY=True
BUG=webrtc:6346
Committed: https://crrev.com/6c278491adae678cdd95c71c444a02103012df56
Cr-Commit-Position: refs/heads/master@{#14713}
Patch Set 1 : . #
Total comments: 6
Patch Set 2 : Rebase. #
Total comments: 17
Patch Set 3 : Responses to reviewer comments: renamings and updated comments. #
Total comments: 8
Patch Set 4 : Removed confusing local varibale name. #Patch Set 5 : Removed superflous member initializer. #
Total comments: 3
Patch Set 6 : Specified what fields should be updated by the mixer and sources. #Patch Set 7 : Rebase. #Patch Set 8 : Updated interface usages (I landed another CL in the wrong order...). #
Messages
Total messages: 28 (11 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Move audio frame memory handling inside AudioMixer. TODO(aleloi): better summary later What does this change do? Simplify the AudioMixer::Source interface and update the mixer implementation to the new interface. Instead of asking a mixer source to provide a pointer to an AudioFrame during each mixing iteration, a mixer should supply a pointer to its own AudioFrame. This simplifies lifetime issues as sources do not give away an internal pointer Implementation: when an audio source is added, the mixer allocates a new AudioFrame. The audio frame is kept together in the internal class SourceStatus together with the audio source pointer until the source is removed. BUG=webrtc:6346 ========== to ========== Move audio frame memory handling inside AudioMixer. Simplify the AudioMixer::Source interface and update the mixer implementation to the new interface. Instead of asking a mixer source to provide a pointer to an AudioFrame during each mixing iteration, a mixer should supply a pointer to its own AudioFrame. This simplifies lifetime issues as sources do not give away an internal pointer. Implementation: when an audio source is added, the mixer allocates a new AudioFrame. The audio frame is kept together in the internal class SourceStatus together with the audio source pointer until the source is removed. BUG=webrtc:6346 ==========
aleloi@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org, solenberg@webrtc.org
Here is the AudioMixer audio frame memory handling API change. https://codereview.webrtc.org/2420913002/diff/20001/webrtc/api/audio/audio_mi... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2420913002/diff/20001/webrtc/api/audio/audio_mi... webrtc/api/audio/audio_mixer.h:49: virtual ~Source() {} [Unrelated to this CL]: I think the reasoning behind not declaring the AudioMixer destructor protected applies for Source as well: it's a pure interface, so implementors should choose whether to allow destroying a Source by a base pointer or not. WDYT? https://codereview.webrtc.org/2420913002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2420913002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:275: // Get audio from the audio sources and put it in the SourceFrame vector. This comment doesn't have to do anything with the rest of the CL. I thought it was confusing.
https://codereview.webrtc.org/2420913002/diff/20001/webrtc/api/audio/audio_mi... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2420913002/diff/20001/webrtc/api/audio/audio_mi... webrtc/api/audio/audio_mixer.h:49: virtual ~Source() {} On 2016/10/14 15:34:44, aleloi wrote: > [Unrelated to this CL]: I think the reasoning behind not declaring the > AudioMixer destructor protected applies for Source as well: it's a pure > interface, so implementors should choose whether to allow destroying a Source by > a base pointer or not. WDYT? Implementations can't make that choice. If you have a base pointer and try calling methods on it (and the destructor is just another method for the purposes of this discussion), the compiler uses the base class's access specifiers to determine whether to allow the call. (It has to be that way, because the access specifiers are checked at compile time, and we don't know which subclass it is until run time.) And leaving the destructor publicly accessible is dangerous for a refcounted object, for the obvious reason. So making it protected is the right thing to do, for all classes that inherit from rtc::RefCountInterface. https://codereview.webrtc.org/2420913002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2420913002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:275: // Get audio from the audio sources and put it in the SourceFrame vector. On 2016/10/14 15:34:44, aleloi wrote: > This comment doesn't have to do anything with the rest of the CL. I thought it > was confusing. Acknowledged. https://codereview.webrtc.org/2420913002/diff/40001/webrtc/api/audio/audio_mi... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2420913002/diff/40001/webrtc/api/audio/audio_mi... webrtc/api/audio/audio_mixer.h:46: AudioFrame* audio_frame) = 0; You never say what the method is supposed to *do*, or what |sample_rate_hz| means... and I think the rest can be said with less words. Something like this? "Overwrites |audio_frame| with new audio (either 1 or 2 interleaved channels) of the given sample rate." (Note: I'm just guessing, so you need to fact check in case this suggestion sounds good.) Should the comment additionally be explicit about how much new audio we're talking about? Is it 10 ms? https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:291: // from there) I don't think you need this comment. This is the way it has to work for the code to be non-buggy. (Not letting raw pointers to an object outlive the object.) https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:360: const auto non_anonymous_iter = OK, I give up. Explain this name for me please... "Hello! My name is Non-Anonymous." :-) https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:33: SourceStatusWithFrame(Source* audio_source, bool is_mixed, float gain) I don't think you need to change the name. That we have an audio frame here is an implementation detail. https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:37: audio_frame() {} You don't need to mention audio_frame here; since if you don't, it'll default to using the default member initializer on line 43. https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:47: SourceStatusWithFrameList; Prefer to use "using" instead of "typedef". It has much better syntax. https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:47: SourceStatusWithFrameList; Did you start using unique_ptr here because you can't (or don't want to) move or swap AudioFrames? (I'm asking because I'm hoping that that can be fixed, allowing us to eliminate a level of indirection.) https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc (right): https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc:64: MOCK_METHOD0(Ssrc, int()); Does this change belong in this CL? You rename it in the previous CL...
Patchset #3 (id:60001) has been deleted
https://codereview.webrtc.org/2420913002/diff/20001/webrtc/api/audio/audio_mi... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2420913002/diff/20001/webrtc/api/audio/audio_mi... webrtc/api/audio/audio_mixer.h:49: virtual ~Source() {} On 2016/10/14 19:59:32, kwiberg-webrtc wrote: > On 2016/10/14 15:34:44, aleloi wrote: > > [Unrelated to this CL]: I think the reasoning behind not declaring the > > AudioMixer destructor protected applies for Source as well: it's a pure > > interface, so implementors should choose whether to allow destroying a Source > by > > a base pointer or not. WDYT? > > Implementations can't make that choice. If you have a base pointer and try > calling methods on it (and the destructor is just another method for the > purposes of this discussion), the compiler uses the base class's access > specifiers to determine whether to allow the call. (It has to be that way, > because the access specifiers are checked at compile time, and we don't know > which subclass it is until run time.) > > And leaving the destructor publicly accessible is dangerous for a refcounted > object, for the obvious reason. So making it protected is the right thing to do, > for all classes that inherit from rtc::RefCountInterface. Thanks for the explanation! Previously I ran in the problem of deleting an AudioMixer twice: an instance was created in the tests without being put in a scoped_ptr. Then a mixer method was called with rtc::Bind. Bind updated the reference counters and caused the object to be destroyed after the call ended. The mixer was destroyed a second time at end of scope. I didn't realize that it could have been avoided by making the dtor protected. An AudioMixer::Source is not ref-counted though, so I think there is no reason to keep the destructor protected. https://codereview.webrtc.org/2420913002/diff/40001/webrtc/api/audio/audio_mi... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2420913002/diff/40001/webrtc/api/audio/audio_mi... webrtc/api/audio/audio_mixer.h:46: AudioFrame* audio_frame) = 0; On 2016/10/14 19:59:32, kwiberg-webrtc wrote: > You never say what the method is supposed to *do*, or what |sample_rate_hz| > means... and I think the rest can be said with less words. Something like this? > > "Overwrites |audio_frame| with new audio (either 1 or 2 interleaved channels) > of the given sample rate." > > (Note: I'm just guessing, so you need to fact check in case this suggestion > sounds good.) > > Should the comment additionally be explicit about how much new audio we're > talking about? Is it 10 ms? I've updated the comment. I guess that it's clear and we don't need to add that implementing classes shouldn't store the audio frame pointer somewhere between calls? https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:291: // from there) On 2016/10/14 19:59:33, kwiberg-webrtc wrote: > I don't think you need this comment. This is the way it has to work for the code > to be non-buggy. (Not letting raw pointers to an object outlive the object.) I see your point. Removed in next patch set. https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:360: const auto non_anonymous_iter = On 2016/10/14 19:59:33, kwiberg-webrtc wrote: > OK, I give up. Explain this name for me please... > > "Hello! My name is Non-Anonymous." :-) I missed to remove all mentions of (non)-anonymity when I removed anonymous mixing a few CLs back. I've renamed it into simply 'iter'. https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:33: SourceStatusWithFrame(Source* audio_source, bool is_mixed, float gain) On 2016/10/14 19:59:33, kwiberg-webrtc wrote: > I don't think you need to change the name. That we have an audio frame here is > an implementation detail. OK, I've changed it back. https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:37: audio_frame() {} On 2016/10/14 19:59:33, kwiberg-webrtc wrote: > You don't need to mention audio_frame here; since if you don't, it'll default to > using the default member initializer on line 43. Thanks! I mis-applied a guideline from Scott Meyers here: 'always initialize every field during construction' turned into 'always write initialization lists for every field'. https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:47: SourceStatusWithFrameList; On 2016/10/14 19:59:33, kwiberg-webrtc wrote: > Did you start using unique_ptr here because you can't (or don't want to) move or > swap AudioFrames? (I'm asking because I'm hoping that that can be fixed, > allowing us to eliminate a level of indirection.) Yes, exactly. We could probably add move constructors and move assignments to AudioFrame, but I am not sure about what implications that would have. https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:47: SourceStatusWithFrameList; On 2016/10/14 19:59:33, kwiberg-webrtc wrote: > Prefer to use "using" instead of "typedef". It has much better syntax. Done. https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc (right): https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc:64: MOCK_METHOD0(Ssrc, int()); On 2016/10/14 19:59:33, kwiberg-webrtc wrote: > Does this change belong in this CL? You rename it in the previous CL... Oops, I submitted the Ssrc renaming in the previous CL without even checking if it compiles. It's now fixed. https://codereview.webrtc.org/2420913002/diff/80001/webrtc/api/audio/audio_mi... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2420913002/diff/80001/webrtc/api/audio/audio_mi... webrtc/api/audio/audio_mixer.h:68: ~AudioMixer() override {} Unrelated change: the dtor of a refcounted class should be protected to not risk destroying the object twice.
lgtm https://codereview.webrtc.org/2420913002/diff/20001/webrtc/api/audio/audio_mi... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2420913002/diff/20001/webrtc/api/audio/audio_mi... webrtc/api/audio/audio_mixer.h:49: virtual ~Source() {} On 2016/10/17 11:06:39, aleloi wrote: > On 2016/10/14 19:59:32, kwiberg-webrtc wrote: > > On 2016/10/14 15:34:44, aleloi wrote: > > > [Unrelated to this CL]: I think the reasoning behind not declaring the > > > AudioMixer destructor protected applies for Source as well: it's a pure > > > interface, so implementors should choose whether to allow destroying a > Source > > by > > > a base pointer or not. WDYT? > > > > Implementations can't make that choice. If you have a base pointer and try > > calling methods on it (and the destructor is just another method for the > > purposes of this discussion), the compiler uses the base class's access > > specifiers to determine whether to allow the call. (It has to be that way, > > because the access specifiers are checked at compile time, and we don't know > > which subclass it is until run time.) > > > > And leaving the destructor publicly accessible is dangerous for a refcounted > > object, for the obvious reason. So making it protected is the right thing to > do, > > for all classes that inherit from rtc::RefCountInterface. > > Thanks for the explanation! Previously I ran in the problem of deleting an > AudioMixer twice: an instance was created in the tests without being put in a > scoped_ptr. Then a mixer method was called with rtc::Bind. Bind updated the > reference counters and caused the object to be destroyed after the call ended. > The mixer was destroyed a second time at end of scope. > > I didn't realize that it could have been avoided by making the dtor protected. > > An AudioMixer::Source is not ref-counted though, so I think there is no reason > to keep the destructor protected. Oh, right. It's the AudioMixer destructor that should be protected. https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2420913002/diff/40001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:47: SourceStatusWithFrameList; On 2016/10/17 11:06:39, aleloi wrote: > On 2016/10/14 19:59:33, kwiberg-webrtc wrote: > > Did you start using unique_ptr here because you can't (or don't want to) move > or > > swap AudioFrames? (I'm asking because I'm hoping that that can be fixed, > > allowing us to eliminate a level of indirection.) > > Yes, exactly. We could probably add move constructors and move assignments to > AudioFrame, but I am not sure about what implications that would have. I don't think anything bad would happen. But since it has a big array embedded in the object rather than on the heap, efficient move will take some refactoring; it's certainly out of scope for this CL. https://codereview.webrtc.org/2420913002/diff/80001/webrtc/api/audio/audio_mi... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2420913002/diff/80001/webrtc/api/audio/audio_mi... webrtc/api/audio/audio_mixer.h:68: ~AudioMixer() override {} On 2016/10/17 11:06:39, aleloi wrote: > Unrelated change: the dtor of a refcounted class should be protected to not risk > destroying the object twice. Acknowledged.
lgtm % comments https://codereview.webrtc.org/2420913002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2420913002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:275: OutputFrequency(), audio_source_audio_frame); Strange and long name. How about just add the 6 extra characters and let the compiler do the optimization? :) https://codereview.webrtc.org/2420913002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2420913002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:40: AudioFrame audio_frame{}; Do you really need the {} ? Also, I thought this type of initialization was not permitted according to the style guide, but I may be mixing things up.
https://codereview.webrtc.org/2420913002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2420913002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:275: OutputFrequency(), audio_source_audio_frame); On 2016/10/18 14:06:17, the sun wrote: > Strange and long name. How about just add the 6 extra characters and let the > compiler do the optimization? :) Done. https://codereview.webrtc.org/2420913002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2420913002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:40: AudioFrame audio_frame{}; On 2016/10/18 14:06:17, the sun wrote: > Do you really need the {} ? > Also, I thought this type of initialization was not permitted according to the > style guide, but I may be mixing things up. It's a (brace) default member initializer (http://en.cppreference.com/w/cpp/language/data_members#Member_initialization). It makes the audio frame to be initialized with its default constructor. I am not sure, but I think the frame will not get initialized otherwise. That was how I understood the documentation section from the link above. I think the style guide is OK with this (https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...)
https://codereview.webrtc.org/2420913002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2420913002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:40: AudioFrame audio_frame{}; On 2016/10/18 14:17:48, aleloi wrote: > On 2016/10/18 14:06:17, the sun wrote: > > Do you really need the {} ? > > Also, I thought this type of initialization was not permitted according to the > > style guide, but I may be mixing things up. > > It's a (brace) default member initializer > (http://en.cppreference.com/w/cpp/language/data_members#Member_initialization). > It makes the audio frame to be initialized with its default constructor. I am > not sure, but I think the frame will not get initialized otherwise. That was how > I understood the documentation section from the link above. > > I think the style guide is OK with this > (https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...) The default ctor will be called without the {}.
https://codereview.webrtc.org/2420913002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2420913002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:40: AudioFrame audio_frame{}; On 2016/10/18 14:21:45, the sun wrote: > On 2016/10/18 14:17:48, aleloi wrote: > > On 2016/10/18 14:06:17, the sun wrote: > > > Do you really need the {} ? > > > Also, I thought this type of initialization was not permitted according to > the > > > style guide, but I may be mixing things up. > > > > It's a (brace) default member initializer > > > (http://en.cppreference.com/w/cpp/language/data_members#Member_initialization). > > It makes the audio frame to be initialized with its default constructor. I am > > not sure, but I think the frame will not get initialized otherwise. That was > how > > I understood the documentation section from the link above. > > > > I think the style guide is OK with this > > > (https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...) > > The default ctor will be called without the {}. Right, because it is a class and then the default constructor is run unless there is a default member initializer or if the member is present in an initialization list. Fixed.
LGTM with one comment. https://codereview.webrtc.org/2420913002/diff/120001/webrtc/api/audio/audio_m... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2420913002/diff/120001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:38: // Overwrites |audio_frame|. The data_ field is overwritten with So, the data_ field is overwritten, but what other fields in audio_frame must the implementation update?
https://codereview.webrtc.org/2420913002/diff/120001/webrtc/api/audio/audio_m... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2420913002/diff/120001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:38: // Overwrites |audio_frame|. The data_ field is overwritten with On 2016/10/18 22:16:13, hlundin-webrtc wrote: > So, the data_ field is overwritten, but what other fields in audio_frame must > the implementation update? That is a good question. The simple answer is 'all of them except for id_'. These fields *must always* be updated. There are asserts and checks for them: samples_per_channel_ number_of_channels_ num_channels_ These fields are read in the AudioTransport function that asks for the mixed sound. When I connected the mixer, I found tests for at least one of them as well (can't recall which now, but will look it up when we connect the mixer for real). They are only filled in correctly if there is a single audio source. Otherwise they are set to -1. elapsed_time_ms_ ntp_time_ms_ These fields are filled in by our default mixer implementation, but are (currently) never read. At least I think they are never read: maybe they are in AudioProcessing during echo detection or limiting. vad_activity_ (set to kVadPassive if no frames are mixed or kVadActive if at least one is) speech_type_ (set to kNormalSpeech) timestamp_ (ticks up by sample_size_ starting from 0 in the mixer). I am reluctant and a bit afraid to specify too much in the interface, at least before the new mixer is connected. It seems that most of the above could change. The slightly longer answer is thus that 'nothing should break if an impl ignores all but the rate and channel fields'.
On 2016/10/19 09:56:47, aleloi wrote: > https://codereview.webrtc.org/2420913002/diff/120001/webrtc/api/audio/audio_m... > File webrtc/api/audio/audio_mixer.h (right): > > https://codereview.webrtc.org/2420913002/diff/120001/webrtc/api/audio/audio_m... > webrtc/api/audio/audio_mixer.h:38: // Overwrites |audio_frame|. The data_ field > is overwritten with > On 2016/10/18 22:16:13, hlundin-webrtc wrote: > > So, the data_ field is overwritten, but what other fields in audio_frame must > > the implementation update? > > That is a good question. The simple answer is 'all of them except for id_'. > > These fields *must always* be updated. There are asserts and checks for them: > > samples_per_channel_ > number_of_channels_ > num_channels_ > > These fields are read in the AudioTransport function that asks for the mixed > sound. When I connected the mixer, I found tests for at least one of them as > well (can't recall which now, but will look it up when we connect the mixer for > real). They are only filled in correctly if there is a single audio source. > Otherwise they are set to -1. > > elapsed_time_ms_ > ntp_time_ms_ > > These fields are filled in by our default mixer implementation, but are > (currently) never read. At least I think they are never read: maybe they are in > AudioProcessing during echo detection or limiting. > > vad_activity_ (set to kVadPassive if no frames are mixed or kVadActive if at > least one is) > speech_type_ (set to kNormalSpeech) > timestamp_ (ticks up by sample_size_ starting from 0 in the mixer). > > > I am reluctant and a bit afraid to specify too much in the interface, at least > before the new mixer is connected. It seems that most of the above could change. > > The slightly longer answer is thus that 'nothing should break if an impl ignores > all but the rate and channel fields'. Tangential: are we even using id_?
I seached for it with "git grep '[-].id_'" and "[.]id_". It seems that the only place that reads id_ is the old AudioConferenceMixer. It's there to distinguish audio frames by source. There is also an audio_coding module test called AudioCodingModuleTestOldApi.VerifyOutputFrame that compares the id. The id_ can be removed after the new mixer has replaced the old one. I have updated the Mix and GetAudioFrameWithInfo comments to specify that all fields must be updated. https://codereview.webrtc.org/2420913002/diff/120001/webrtc/api/audio/audio_m... File webrtc/api/audio/audio_mixer.h (right): https://codereview.webrtc.org/2420913002/diff/120001/webrtc/api/audio/audio_m... webrtc/api/audio/audio_mixer.h:38: // Overwrites |audio_frame|. The data_ field is overwritten with On 2016/10/19 09:56:46, aleloi wrote: > On 2016/10/18 22:16:13, hlundin-webrtc wrote: > > So, the data_ field is overwritten, but what other fields in audio_frame must > > the implementation update? > > That is a good question. The simple answer is 'all of them except for id_'. > > These fields *must always* be updated. There are asserts and checks for them: > > samples_per_channel_ > number_of_channels_ > num_channels_ > > These fields are read in the AudioTransport function that asks for the mixed > sound. When I connected the mixer, I found tests for at least one of them as > well (can't recall which now, but will look it up when we connect the mixer for > real). They are only filled in correctly if there is a single audio source. > Otherwise they are set to -1. > > elapsed_time_ms_ > ntp_time_ms_ > > These fields are filled in by our default mixer implementation, but are > (currently) never read. At least I think they are never read: maybe they are in > AudioProcessing during echo detection or limiting. > > vad_activity_ (set to kVadPassive if no frames are mixed or kVadActive if at > least one is) > speech_type_ (set to kNormalSpeech) > timestamp_ (ticks up by sample_size_ starting from 0 in the mixer). > > > I am reluctant and a bit afraid to specify too much in the interface, at least > before the new mixer is connected. It seems that most of the above could change. > > The slightly longer answer is thus that 'nothing should break if an impl ignores > all but the rate and channel fields'. I just realized that I answered the wrong question. The above answer explains what the *mixer* does with the AudioFrame provided in Mix and not what is expected by GetAudioFrameWithInfo. For GetAudioFrameWithInfo, *every* field except for 'id_' must be filled in. vad_activity_ and speech_type_ is used to determine whether the frame will be mixed. elapsed_time and ntp_time are copied to the mixed result if there is only one source.
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, kwiberg@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2420913002/#ps180001 (title: "Updated interface usages (I landed another CL in the wrong order...).")
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: android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Move audio frame memory handling inside AudioMixer. Simplify the AudioMixer::Source interface and update the mixer implementation to the new interface. Instead of asking a mixer source to provide a pointer to an AudioFrame during each mixing iteration, a mixer should supply a pointer to its own AudioFrame. This simplifies lifetime issues as sources do not give away an internal pointer. Implementation: when an audio source is added, the mixer allocates a new AudioFrame. The audio frame is kept together in the internal class SourceStatus together with the audio source pointer until the source is removed. BUG=webrtc:6346 ========== to ========== Move audio frame memory handling inside AudioMixer. Simplify the AudioMixer::Source interface and update the mixer implementation to the new interface. Instead of asking a mixer source to provide a pointer to an AudioFrame during each mixing iteration, a mixer should supply a pointer to its own AudioFrame. This simplifies lifetime issues as sources do not give away an internal pointer. Implementation: when an audio source is added, the mixer allocates a new AudioFrame. The audio frame is kept together in the internal class SourceStatus together with the audio source pointer until the source is removed. NOTRY=True BUG=webrtc:6346 ==========
The CQ bit was checked by aleloi@google.com
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 ========== Move audio frame memory handling inside AudioMixer. Simplify the AudioMixer::Source interface and update the mixer implementation to the new interface. Instead of asking a mixer source to provide a pointer to an AudioFrame during each mixing iteration, a mixer should supply a pointer to its own AudioFrame. This simplifies lifetime issues as sources do not give away an internal pointer. Implementation: when an audio source is added, the mixer allocates a new AudioFrame. The audio frame is kept together in the internal class SourceStatus together with the audio source pointer until the source is removed. NOTRY=True BUG=webrtc:6346 ========== to ========== Move audio frame memory handling inside AudioMixer. Simplify the AudioMixer::Source interface and update the mixer implementation to the new interface. Instead of asking a mixer source to provide a pointer to an AudioFrame during each mixing iteration, a mixer should supply a pointer to its own AudioFrame. This simplifies lifetime issues as sources do not give away an internal pointer. Implementation: when an audio source is added, the mixer allocates a new AudioFrame. The audio frame is kept together in the internal class SourceStatus together with the audio source pointer until the source is removed. NOTRY=True BUG=webrtc:6346 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Move audio frame memory handling inside AudioMixer. Simplify the AudioMixer::Source interface and update the mixer implementation to the new interface. Instead of asking a mixer source to provide a pointer to an AudioFrame during each mixing iteration, a mixer should supply a pointer to its own AudioFrame. This simplifies lifetime issues as sources do not give away an internal pointer. Implementation: when an audio source is added, the mixer allocates a new AudioFrame. The audio frame is kept together in the internal class SourceStatus together with the audio source pointer until the source is removed. NOTRY=True BUG=webrtc:6346 ========== to ========== Move audio frame memory handling inside AudioMixer. Simplify the AudioMixer::Source interface and update the mixer implementation to the new interface. Instead of asking a mixer source to provide a pointer to an AudioFrame during each mixing iteration, a mixer should supply a pointer to its own AudioFrame. This simplifies lifetime issues as sources do not give away an internal pointer. Implementation: when an audio source is added, the mixer allocates a new AudioFrame. The audio frame is kept together in the internal class SourceStatus together with the audio source pointer until the source is removed. NOTRY=True BUG=webrtc:6346 Committed: https://crrev.com/6c278491adae678cdd95c71c444a02103012df56 Cr-Commit-Position: refs/heads/master@{#14713} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6c278491adae678cdd95c71c444a02103012df56 Cr-Commit-Position: refs/heads/master@{#14713} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
