Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 1 month ago by aleloi Modified:
4 years, 1 month ago Reviewers:
the sun, ossu, kwiberg-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded audio mixer and removed audio device module in AudioState::Config.
The audio_device_module field was currently unused. The audio_mixer
field is going to be used to pass an AudioMixer to AudioState.
In the hopefully-not-very-far future, the toplevel WebRTC API will allow passing
a custom AudioMixer, e.g. for spatialized audio (audio in space). If no
mixer is passed, a default mixer is created (the one in modules/audio_mixer).
The only object which will have a permanent reference to the mixer is AudioState.
AudioState is created in WebRTCVoiceEngine with a configuration object,
which already contains a VoiceEngine pointer. In this CL, we extend this
config object with a mixer pointer.
In summary: in an upcoming CL, a mixer will be either created in or passed to
WebRTCVoiceEngine. This mixer will be passed to the ctor of AudioState in a
config struct.
BUG=webrtc:6346
NOTRY=True
Committed: https://crrev.com/81da488ab6c9c79e4a74e29d18c574285312241f
Cr-Commit-Position: refs/heads/master@{#14973}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Removed default initializer. #
Total comments: 2
Created: 4 years, 1 month ago
Dependent Patchsets: Messages
Total messages: 26 (11 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Added audio mixer and removed audio device module in AudioState::Config. The audio_device_module field was currently unused. The audio_mixer field is going to be used to pass an AudioMixer to AudioState. BUG=webrtc:6346 ========== to ========== Added audio mixer and removed audio device module in AudioState::Config. The audio_device_module field was currently unused. The audio_mixer field is going to be used to pass an AudioMixer to AudioState. BUG=webrtc:6346 NOTRY=True ==========
Description was changed from ========== Added audio mixer and removed audio device module in AudioState::Config. The audio_device_module field was currently unused. The audio_mixer field is going to be used to pass an AudioMixer to AudioState. BUG=webrtc:6346 NOTRY=True ========== to ========== Added audio mixer and removed audio device module in AudioState::Config. The audio_device_module field was currently unused. The audio_mixer field is going to be used to pass an AudioMixer to AudioState. In the not-very-far future, the toplevel WebRTC API will allow passing a custom AudioMixer, e.g. for spatialized audio (audio in space). If no mixer is passed, a default mixer is created (the one in modules/audio_mixer). The only object which will have a permanent reference to the mixer is AudioState. AudioState is created in WebRTCVoiceEngine with a configuration object, which already contains a VoiceEngine pointer. In this CL, we extend this config object with a mixer pointer. In summary: in an upcoming CL, a mixer will be either created in or passed to WebRTCVoiceEngine. This mixer will be passed to the ctor of AudioState in a config struct. BUG=webrtc:6346 NOTRY=True ==========
aleloi@webrtc.org changed reviewers: + kwiberg@webrtc.org
Description was changed from ========== Added audio mixer and removed audio device module in AudioState::Config. The audio_device_module field was currently unused. The audio_mixer field is going to be used to pass an AudioMixer to AudioState. In the not-very-far future, the toplevel WebRTC API will allow passing a custom AudioMixer, e.g. for spatialized audio (audio in space). If no mixer is passed, a default mixer is created (the one in modules/audio_mixer). The only object which will have a permanent reference to the mixer is AudioState. AudioState is created in WebRTCVoiceEngine with a configuration object, which already contains a VoiceEngine pointer. In this CL, we extend this config object with a mixer pointer. In summary: in an upcoming CL, a mixer will be either created in or passed to WebRTCVoiceEngine. This mixer will be passed to the ctor of AudioState in a config struct. BUG=webrtc:6346 NOTRY=True ========== to ========== Added audio mixer and removed audio device module in AudioState::Config. The audio_device_module field was currently unused. The audio_mixer field is going to be used to pass an AudioMixer to AudioState. In the hopefully-not-very-far future, the toplevel WebRTC API will allow passing a custom AudioMixer, e.g. for spatialized audio (audio in space). If no mixer is passed, a default mixer is created (the one in modules/audio_mixer). The only object which will have a permanent reference to the mixer is AudioState. AudioState is created in WebRTCVoiceEngine with a configuration object, which already contains a VoiceEngine pointer. In this CL, we extend this config object with a mixer pointer. In summary: in an upcoming CL, a mixer will be either created in or passed to WebRTCVoiceEngine. This mixer will be passed to the ctor of AudioState in a config struct. BUG=webrtc:6346 NOTRY=True ==========
Hi again! Can you take a look at this too? There are only two lines of code changes. I've made an effort to describe the broader context in the CL description. https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... File webrtc/api/call/audio_state.h (left): https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... webrtc/api/call/audio_state.h:37: AudioDeviceModule* audio_device_module = nullptr; This field is currently unused, and AudioState gets the audio_device_module from |voice_engine|. When voice engine no longer has an audio_device_module it can be added back. I think it's more confusing if I comment it out rather than deleting it.
https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... File webrtc/api/call/audio_state.h (left): https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... webrtc/api/call/audio_state.h:37: AudioDeviceModule* audio_device_module = nullptr; On 2016/10/31 11:45:01, aleloi wrote: > This field is currently unused, and AudioState gets the audio_device_module from > |voice_engine|. When voice engine no longer has an audio_device_module it can be > added back. I think it's more confusing if I comment it out rather than deleting > it. Acknowledged. https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... File webrtc/api/call/audio_state.h (right): https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... webrtc/api/call/audio_state.h:39: rtc::scoped_refptr<AudioMixer> audio_mixer = nullptr; You don't need to explicitly initialize scoped_refptr to null. (AFAIK this is true of all smart pointers---they tend to be tricky to implement otherwise, and it's good for usability.) Why is this reference counted rather than uniquely owned? And what consequences does that have? For example, is the user allowed to use the mixer for something else while it's being used for this AudioState?
https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... File webrtc/api/call/audio_state.h (right): https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... webrtc/api/call/audio_state.h:39: rtc::scoped_refptr<AudioMixer> audio_mixer = nullptr; On 2016/10/31 12:31:06, kwiberg-webrtc wrote: > You don't need to explicitly initialize scoped_refptr to null. (AFAIK this is > true of all smart pointers---they tend to be tricky to implement otherwise, and > it's good for usability.) > > Why is this reference counted rather than uniquely owned? And what consequences > does that have? For example, is the user allowed to use the mixer for something > else while it's being used for this AudioState? Thanks, removed default initializer (it is called a default initializer, right?). The motivation for having a mixer refcounted is because we want to allow a user to provide their own mixer. Then we don't know who owns it and don't want to destroy it by mistake. It's not needed for the current mixer implementation. As for consequences, the mixer is written in a way to allow multi-threaded access of the public API methods. Since we can't control what a user does with the mixer, we don't forbid anything except that only one thread should call Mix at a time (but that is a clear mis-use; if that were allowed one listener would get fragments of the audio).
https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... File webrtc/api/call/audio_state.h (right): https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... webrtc/api/call/audio_state.h:39: rtc::scoped_refptr<AudioMixer> audio_mixer = nullptr; On 2016/10/31 12:44:01, aleloi wrote: > On 2016/10/31 12:31:06, kwiberg-webrtc wrote: > > You don't need to explicitly initialize scoped_refptr to null. (AFAIK this is > > true of all smart pointers---they tend to be tricky to implement otherwise, > and > > it's good for usability.) > > > > Why is this reference counted rather than uniquely owned? And what > consequences > > does that have? For example, is the user allowed to use the mixer for > something > > else while it's being used for this AudioState? > Thanks, removed default initializer (it is called a default initializer, > right?). I think the term is "default member initializer". It's probably better to not omit the "member" part, since otherwise it's very similar to "default initialization", which is an entirely different thing. If you have an hour or three that you don't know what to do with, see http://en.cppreference.com/w/cpp/language/initialization. > The motivation for having a mixer refcounted is because we want to allow a user > to provide their own mixer. Then we don't know who owns it and don't want to > destroy it by mistake. It's not needed for the current mixer implementation. As > for consequences, the mixer is written in a way to allow multi-threaded access > of the public API methods. > > Since we can't control what a user does with the mixer, we don't forbid anything > except that only one thread should call Mix at a time (but that is a clear > mis-use; if that were allowed one listener would get fragments of the audio). So let me propose an alternative solution: passing the mixer in a unique_ptr. That way you know it's not being used by anyone else, which makes life very simple for everyone. No need for the mixer to do its own locking, for example. Will the restrictions brought about by using unique_ptr be in the way of some reasonable use case?
https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... File webrtc/api/call/audio_state.h (right): https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... webrtc/api/call/audio_state.h:39: rtc::scoped_refptr<AudioMixer> audio_mixer = nullptr; On 2016/10/31 13:30:33, kwiberg-webrtc wrote: > On 2016/10/31 12:44:01, aleloi wrote: > > On 2016/10/31 12:31:06, kwiberg-webrtc wrote: > > > You don't need to explicitly initialize scoped_refptr to null. (AFAIK this > is > > > true of all smart pointers---they tend to be tricky to implement otherwise, > > and > > > it's good for usability.) > > > > > > Why is this reference counted rather than uniquely owned? And what > > consequences > > > does that have? For example, is the user allowed to use the mixer for > > something > > > else while it's being used for this AudioState? > > Thanks, removed default initializer (it is called a default initializer, > > right?). > > I think the term is "default member initializer". It's probably better to not > omit the "member" part, since otherwise it's very similar to "default > initialization", which is an entirely different thing. > > If you have an hour or three that you don't know what to do with, see > http://en.cppreference.com/w/cpp/language/initialization. > > > The motivation for having a mixer refcounted is because we want to allow a > user > > to provide their own mixer. Then we don't know who owns it and don't want to > > destroy it by mistake. It's not needed for the current mixer implementation. > As > > for consequences, the mixer is written in a way to allow multi-threaded access > > of the public API methods. > > > > Since we can't control what a user does with the mixer, we don't forbid > anything > > except that only one thread should call Mix at a time (but that is a clear > > mis-use; if that were allowed one listener would get fragments of the audio). > > So let me propose an alternative solution: passing the mixer in a unique_ptr. > That way you know it's not being used by anyone else, which makes life very > simple for everyone. No need for the mixer to do its own locking, for example. > > Will the restrictions brought about by using unique_ptr be in the way of some > reasonable use case? Avoiding shared pointers is always good! I've asked what constraints there are on external mixers. Depending on the outcome, I'll add a CL before this that removes RefCountInterface inheritance from the mixer. I think we can wait a few days.
https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... File webrtc/api/call/audio_state.h (right): https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... webrtc/api/call/audio_state.h:39: rtc::scoped_refptr<AudioMixer> audio_mixer = nullptr; On 2016/10/31 14:56:40, aleloi wrote: > On 2016/10/31 13:30:33, kwiberg-webrtc wrote: > > On 2016/10/31 12:44:01, aleloi wrote: > > > On 2016/10/31 12:31:06, kwiberg-webrtc wrote: > > > > You don't need to explicitly initialize scoped_refptr to null. (AFAIK this > > is > > > > true of all smart pointers---they tend to be tricky to implement > otherwise, > > > and > > > > it's good for usability.) > > > > > > > > Why is this reference counted rather than uniquely owned? And what > > > consequences > > > > does that have? For example, is the user allowed to use the mixer for > > > something > > > > else while it's being used for this AudioState? > > > Thanks, removed default initializer (it is called a default initializer, > > > right?). > > > > I think the term is "default member initializer". It's probably better to not > > omit the "member" part, since otherwise it's very similar to "default > > initialization", which is an entirely different thing. > > > > If you have an hour or three that you don't know what to do with, see > > http://en.cppreference.com/w/cpp/language/initialization. > > > > > The motivation for having a mixer refcounted is because we want to allow a > > user > > > to provide their own mixer. Then we don't know who owns it and don't want to > > > destroy it by mistake. It's not needed for the current mixer implementation. > > As > > > for consequences, the mixer is written in a way to allow multi-threaded > access > > > of the public API methods. > > > > > > Since we can't control what a user does with the mixer, we don't forbid > > anything > > > except that only one thread should call Mix at a time (but that is a clear > > > mis-use; if that were allowed one listener would get fragments of the > audio). > > > > So let me propose an alternative solution: passing the mixer in a unique_ptr. > > That way you know it's not being used by anyone else, which makes life very > > simple for everyone. No need for the mixer to do its own locking, for example. > > > > Will the restrictions brought about by using unique_ptr be in the way of some > > reasonable use case? > > Avoiding shared pointers is always good! I've asked what constraints there are > on external mixers. Depending on the outcome, I'll add a CL before this that > removes RefCountInterface inheritance from the mixer. I think we can wait a few > days. Excellent. It's not always possible to avoid shared ownership, but it has a very real cost in increased code complexity, so we should only use it if we have a good reason.
ossu@webrtc.org changed reviewers: + ossu@webrtc.org
https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... File webrtc/api/call/audio_state.h (right): https://codereview.webrtc.org/2456363002/diff/40001/webrtc/api/call/audio_sta... webrtc/api/call/audio_state.h:39: rtc::scoped_refptr<AudioMixer> audio_mixer = nullptr; On 2016/11/01 00:45:59, kwiberg-webrtc wrote: > On 2016/10/31 14:56:40, aleloi wrote: > > On 2016/10/31 13:30:33, kwiberg-webrtc wrote: > > > On 2016/10/31 12:44:01, aleloi wrote: > > > > On 2016/10/31 12:31:06, kwiberg-webrtc wrote: > > > > > You don't need to explicitly initialize scoped_refptr to null. (AFAIK > this > > > is > > > > > true of all smart pointers---they tend to be tricky to implement > > otherwise, > > > > and > > > > > it's good for usability.) > > > > > > > > > > Why is this reference counted rather than uniquely owned? And what > > > > consequences > > > > > does that have? For example, is the user allowed to use the mixer for > > > > something > > > > > else while it's being used for this AudioState? > > > > Thanks, removed default initializer (it is called a default initializer, > > > > right?). > > > > > > I think the term is "default member initializer". It's probably better to > not > > > omit the "member" part, since otherwise it's very similar to "default > > > initialization", which is an entirely different thing. > > > > > > If you have an hour or three that you don't know what to do with, see > > > http://en.cppreference.com/w/cpp/language/initialization. > > > > > > > The motivation for having a mixer refcounted is because we want to allow a > > > user > > > > to provide their own mixer. Then we don't know who owns it and don't want > to > > > > destroy it by mistake. It's not needed for the current mixer > implementation. > > > As > > > > for consequences, the mixer is written in a way to allow multi-threaded > > access > > > > of the public API methods. > > > > > > > > Since we can't control what a user does with the mixer, we don't forbid > > > anything > > > > except that only one thread should call Mix at a time (but that is a clear > > > > mis-use; if that were allowed one listener would get fragments of the > > audio). > > > > > > So let me propose an alternative solution: passing the mixer in a > unique_ptr. > > > That way you know it's not being used by anyone else, which makes life very > > > simple for everyone. No need for the mixer to do its own locking, for > example. > > > > > > Will the restrictions brought about by using unique_ptr be in the way of > some > > > reasonable use case? > > > > Avoiding shared pointers is always good! I've asked what constraints there are > > on external mixers. Depending on the outcome, I'll add a CL before this that > > removes RefCountInterface inheritance from the mixer. I think we can wait a > few > > days. > > Excellent. It's not always possible to avoid shared ownership, but it has a very > real cost in increased code complexity, so we should only use it if we have a > good reason. Agreed. unique_ptr sounds good to me! I'd like to add that we also shouldn't make something reference counted just because we don't know how it's going to be owned/used. Shared ownership is not a replacement for actually figuring out ownership yourself. I used it like that in a previous project and it wasn't long before we had cyclical ownership and nothing would get destructed. Took a while to untangle.
Ping pling! I want to leave the scoped_refptr for the time being. In part because I'd like to finish webrtc:6346 in a foreseeable future, but there are other reasons (discussed offline) :)
On 2016/11/08 11:58:35, aleloi wrote: > Ping pling! I want to leave the scoped_refptr for the time being. In part > because I'd like to finish webrtc:6346 in a foreseeable future, but there are > other reasons (discussed offline) :) Alright. LGTM then, if we can't get rid of the refcounting right now.
lgtm
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 ========== Added audio mixer and removed audio device module in AudioState::Config. The audio_device_module field was currently unused. The audio_mixer field is going to be used to pass an AudioMixer to AudioState. In the hopefully-not-very-far future, the toplevel WebRTC API will allow passing a custom AudioMixer, e.g. for spatialized audio (audio in space). If no mixer is passed, a default mixer is created (the one in modules/audio_mixer). The only object which will have a permanent reference to the mixer is AudioState. AudioState is created in WebRTCVoiceEngine with a configuration object, which already contains a VoiceEngine pointer. In this CL, we extend this config object with a mixer pointer. In summary: in an upcoming CL, a mixer will be either created in or passed to WebRTCVoiceEngine. This mixer will be passed to the ctor of AudioState in a config struct. BUG=webrtc:6346 NOTRY=True ========== to ========== Added audio mixer and removed audio device module in AudioState::Config. The audio_device_module field was currently unused. The audio_mixer field is going to be used to pass an AudioMixer to AudioState. In the hopefully-not-very-far future, the toplevel WebRTC API will allow passing a custom AudioMixer, e.g. for spatialized audio (audio in space). If no mixer is passed, a default mixer is created (the one in modules/audio_mixer). The only object which will have a permanent reference to the mixer is AudioState. AudioState is created in WebRTCVoiceEngine with a configuration object, which already contains a VoiceEngine pointer. In this CL, we extend this config object with a mixer pointer. In summary: in an upcoming CL, a mixer will be either created in or passed to WebRTCVoiceEngine. This mixer will be passed to the ctor of AudioState in a config struct. BUG=webrtc:6346 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Added audio mixer and removed audio device module in AudioState::Config. The audio_device_module field was currently unused. The audio_mixer field is going to be used to pass an AudioMixer to AudioState. In the hopefully-not-very-far future, the toplevel WebRTC API will allow passing a custom AudioMixer, e.g. for spatialized audio (audio in space). If no mixer is passed, a default mixer is created (the one in modules/audio_mixer). The only object which will have a permanent reference to the mixer is AudioState. AudioState is created in WebRTCVoiceEngine with a configuration object, which already contains a VoiceEngine pointer. In this CL, we extend this config object with a mixer pointer. In summary: in an upcoming CL, a mixer will be either created in or passed to WebRTCVoiceEngine. This mixer will be passed to the ctor of AudioState in a config struct. BUG=webrtc:6346 NOTRY=True ========== to ========== Added audio mixer and removed audio device module in AudioState::Config. The audio_device_module field was currently unused. The audio_mixer field is going to be used to pass an AudioMixer to AudioState. In the hopefully-not-very-far future, the toplevel WebRTC API will allow passing a custom AudioMixer, e.g. for spatialized audio (audio in space). If no mixer is passed, a default mixer is created (the one in modules/audio_mixer). The only object which will have a permanent reference to the mixer is AudioState. AudioState is created in WebRTCVoiceEngine with a configuration object, which already contains a VoiceEngine pointer. In this CL, we extend this config object with a mixer pointer. In summary: in an upcoming CL, a mixer will be either created in or passed to WebRTCVoiceEngine. This mixer will be passed to the ctor of AudioState in a config struct. BUG=webrtc:6346 NOTRY=True Committed: https://crrev.com/81da488ab6c9c79e4a74e29d18c574285312241f Cr-Commit-Position: refs/heads/master@{#14973} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/81da488ab6c9c79e4a74e29d18c574285312241f Cr-Commit-Position: refs/heads/master@{#14973}
Message was sent while issue was closed.
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2456363002/diff/60001/webrtc/api/call/audio_sta... File webrtc/api/call/audio_state.h (right): https://codereview.webrtc.org/2456363002/diff/60001/webrtc/api/call/audio_sta... webrtc/api/call/audio_state.h:19: class AudioDeviceModule; Please remove - not needed anymore.
Message was sent while issue was closed.
https://codereview.webrtc.org/2456363002/diff/60001/webrtc/api/call/audio_sta... File webrtc/api/call/audio_state.h (right): https://codereview.webrtc.org/2456363002/diff/60001/webrtc/api/call/audio_sta... webrtc/api/call/audio_state.h:19: class AudioDeviceModule; On 2016/11/09 10:51:44, the sun wrote: > Please remove - not needed anymore. Done in https://codereview.webrtc.org/2491483002/! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
