Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2308573002:
Adding AudioNetworkAdaptor interfaces. (Closed)
|
Created:
4 years, 3 months ago by minyue-webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding AudioNetworkAdaptor interfaces.
AudioNetworkAdaptor is supposed to facilitate AudioEncoder to adapt to varying network conditions.
This is the first of a sequence of CLs that are to add one implementation of AudioNetworkAdaptor.
This CL illustrates the interfaces of the AudioNetworkAdaptor.
BUG=webrtc:6303
Committed: https://crrev.com/7610f85a2bfe2878450914d78e6a85639515275f
Cr-Commit-Position: refs/heads/master@{#14115}
Patch Set 1 #
Total comments: 24
Patch Set 2 : after review #Patch Set 3 : correction #
Total comments: 2
Patch Set 4 : removing non-abstract-class methods #
Dependent Patchsets: Messages
Total messages: 39 (17 generated)
Description was changed from ========== cleaning up for commit 1 ready to commit adding more half way Adding interface for AudioNetworkAdaptor. BUG= ========== to ========== Adding AudioNetworkAdaptor interfaces. AudioNetworkAdaptor is supposed to facilitate AudioEncoder to adapt to varying network conditions. This is the first of a sequence of CLs that are to add one implementation of AudioNetworkAdaptor. This CL illustrates the interfaces of the AudioNetworkAdaptor. BUG= ==========
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org
Patchset #1 (id:1) has been deleted
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Adding AudioNetworkAdaptor interfaces. AudioNetworkAdaptor is supposed to facilitate AudioEncoder to adapt to varying network conditions. This is the first of a sequence of CLs that are to add one implementation of AudioNetworkAdaptor. This CL illustrates the interfaces of the AudioNetworkAdaptor. BUG= ========== to ========== Adding AudioNetworkAdaptor interfaces. AudioNetworkAdaptor is supposed to facilitate AudioEncoder to adapt to varying network conditions. This is the first of a sequence of CLs that are to add one implementation of AudioNetworkAdaptor. This CL illustrates the interfaces of the AudioNetworkAdaptor. BUG=webrtc:6303 ==========
Hi Henrik and Karl, I'd like you to review this CL. This is the first of a series of CLs that will add network adaptation capability to AudioEncoder. Thanks in advance!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_drmemory_light on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_gyp_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Nice. Two comments/questions inline. https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h (right): https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:40: static AudioNetworkAdaptor* Create(); What is the purpose of the Create method, when the ctor is public? https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:56: virtual void StartDebugDump(FILE* file_handle) = 0; We already have quite a few debug writers in the code base. Any chance you could use webrtc/modules/audio_processing/logging/apm_data_dumper.h, for instance?
Thanks for the comments! See my reply and a new patch set will follow. https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h (right): https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:40: static AudioNetworkAdaptor* Create(); On 2016/09/05 12:55:45, hlundin-webrtc wrote: > What is the purpose of the Create method, when the ctor is public? You are right. ctor should be removed in this case. https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:42: AudioNetworkAdaptor(); this should be removed. https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:56: virtual void StartDebugDump(FILE* file_handle) = 0; On 2016/09/05 12:55:45, hlundin-webrtc wrote: > We already have quite a few debug writers in the code base. Any chance you could > use webrtc/modules/audio_processing/logging/apm_data_dumper.h, for instance? Maybe. But AudioNetworkAdaptor does not expect to dump audio, it will dump network-condition / encoder-configs. Therefore, it will be a lot simpler than apm_data_dumper.
https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h (right): https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:12: #define WEBRTC_MODULES_AUDIO_CODING_AUDIO_NETWORK_ADAPTOR_INCLUDE_AUDIO_NETWORK_ADAPTOR_H_ Is there any way to rename files and directories so that this will fit in 80 columns? https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:21: // network metrics. "advising" -> "setting"? https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:40: static AudioNetworkAdaptor* Create(); On 2016/09/05 13:12:39, minyue-webrtc wrote: > On 2016/09/05 12:55:45, hlundin-webrtc wrote: > > What is the purpose of the Create method, when the ctor is public? > > You are right. ctor should be removed in this case. +1 https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:40: static AudioNetworkAdaptor* Create(); Create functions should return some sort of smart pointer (unique_ptr if you don't specifically need something else), since they pass ownership of the pointer to the caller. This follows from the rule that ownership should never be held by raw pointers. https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:52: int min_frame_length_ms) = 0; Switch order to (min, max)?
Thanks for the review! A new patch is added. PTAL again, thanks. https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h (right): https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:40: static AudioNetworkAdaptor* Create(); On 2016/09/05 13:36:46, kwiberg-webrtc wrote: > Create functions should return some sort of smart pointer (unique_ptr if you > don't specifically need something else), since they pass ownership of the > pointer to the caller. This follows from the rule that ownership should never be > held by raw pointers. Yes, that would be great. https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:52: int min_frame_length_ms) = 0; On 2016/09/05 13:36:45, kwiberg-webrtc wrote: > Switch order to (min, max)? Yes, good find. Thanks!
PS2 was not complete. Please take a look at PS3.
https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h (right): https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:12: #define WEBRTC_MODULES_AUDIO_CODING_AUDIO_NETWORK_ADAPTOR_INCLUDE_AUDIO_NETWORK_ADAPTOR_H_ On 2016/09/05 13:36:46, kwiberg-webrtc wrote: > Is there any way to rename files and directories so that this will fit in 80 > columns? I say don't bother with that. https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:42: AudioNetworkAdaptor(); On 2016/09/05 13:12:39, minyue-webrtc wrote: > this should be removed. (kwiberg, please keep me honest here.) Now that you have no ctor declared, the compiler will make an implicitly declared one for you. So, a user will still be able to construct an ANA from using the ctor instead of Create(). If this is not what you want, you will have to explicitly declare the ctor as private. On another note, I still don't see the point with a Create() method instead of a ctor. Will it become clear in future CLs? https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:56: virtual void StartDebugDump(FILE* file_handle) = 0; On 2016/09/05 13:12:39, minyue-webrtc wrote: > On 2016/09/05 12:55:45, hlundin-webrtc wrote: > > We already have quite a few debug writers in the code base. Any chance you > could > > use webrtc/modules/audio_processing/logging/apm_data_dumper.h, for instance? > > Maybe. But AudioNetworkAdaptor does not expect to dump audio, it will dump > network-condition / encoder-configs. Therefore, it will be a lot simpler than > apm_data_dumper. Acknowledged.
See my reply on the ctor/Create() choice, WDYT? https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h (right): https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:21: // network metrics. On 2016/09/05 13:36:45, kwiberg-webrtc wrote: > "advising" -> "setting"? I used the word "advising" since AudioNetworkAdaptor does not set encoder directly. https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:42: AudioNetworkAdaptor(); On 2016/09/06 09:18:05, hlundin-webrtc wrote: > On 2016/09/05 13:12:39, minyue-webrtc wrote: > > this should be removed. > > (kwiberg, please keep me honest here.) > Now that you have no ctor declared, the compiler will make an implicitly > declared one for you. So, a user will still be able to construct an ANA from > using the ctor instead of Create(). If this is not what you want, you will have > to explicitly declare the ctor as private. > > On another note, I still don't see the point with a Create() method instead of a > ctor. Will it become clear in future CLs? Thank you for bringing up this discussion. AudioNetworkAdaptor as an abstract class cannot be instantiated, so ctor will not do a job. Create() is meant to instantiate an implementational class of it as default. But yes, we can run without default and ask the owner class to initiate the implementational class. Upon a second thought, I think we should remove Create(). Since ctor will be implicitly declared, we can put it here, although it cannot instantiate.
On 2016/09/06 10:15:27, minyue-webrtc wrote: > See my reply on the ctor/Create() choice, WDYT? > > https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... > File > webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h > (right): > > https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:21: > // network metrics. > On 2016/09/05 13:36:45, kwiberg-webrtc wrote: > > "advising" -> "setting"? > > I used the word "advising" since AudioNetworkAdaptor does not set encoder > directly. > > https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:42: > AudioNetworkAdaptor(); > On 2016/09/06 09:18:05, hlundin-webrtc wrote: > > On 2016/09/05 13:12:39, minyue-webrtc wrote: > > > this should be removed. > > > > (kwiberg, please keep me honest here.) > > Now that you have no ctor declared, the compiler will make an implicitly > > declared one for you. So, a user will still be able to construct an ANA from > > using the ctor instead of Create(). If this is not what you want, you will > have > > to explicitly declare the ctor as private. > > > > On another note, I still don't see the point with a Create() method instead of > a > > ctor. Will it become clear in future CLs? > > Thank you for bringing up this discussion. > > AudioNetworkAdaptor as an abstract class cannot be instantiated, so ctor will > not do a job. Create() is meant to instantiate an implementational class of it > as default. But yes, we can run without default and ask the owner class to > initiate the implementational class. > > Upon a second thought, I think we should remove Create(). > > Since ctor will be implicitly declared, we can put it here, although it cannot > instantiate. In addition, we should make ctor protected to reinforce the fact that this class cannot be instantiated.
https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h (right): https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:12: #define WEBRTC_MODULES_AUDIO_CODING_AUDIO_NETWORK_ADAPTOR_INCLUDE_AUDIO_NETWORK_ADAPTOR_H_ On 2016/09/06 09:18:05, hlundin-webrtc wrote: > On 2016/09/05 13:36:46, kwiberg-webrtc wrote: > > Is there any way to rename files and directories so that this will fit in 80 > > columns? > > I say don't bother with that. I'd be pushing harder for this if I had a reasonable alternative in mind, but since I don't, I'll just grumble a bit and hope I won't have to look at these too-long path names to often. https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:21: // network metrics. On 2016/09/06 10:15:27, minyue-webrtc wrote: > On 2016/09/05 13:36:45, kwiberg-webrtc wrote: > > "advising" -> "setting"? > > I used the word "advising" since AudioNetworkAdaptor does not set encoder > directly. Then rephrase slightly, because you don't advise the "encoder runtime configurations", you advise the encoder *about* a suitable runtime configuration (note the singular). Maybe An AudioNetworkAdaptor optimizes the audio experience by suggesting a suitable runtime configuration (bit rate, frame length, FEC, etc.) to the encoder based on network metrics. ? https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:42: AudioNetworkAdaptor(); On 2016/09/06 10:15:27, minyue-webrtc wrote: > On 2016/09/06 09:18:05, hlundin-webrtc wrote: > > On 2016/09/05 13:12:39, minyue-webrtc wrote: > > > this should be removed. > > > > (kwiberg, please keep me honest here.) > > Now that you have no ctor declared, the compiler will make an implicitly > > declared one for you. So, a user will still be able to construct an ANA from > > using the ctor instead of Create(). If this is not what you want, you will > have > > to explicitly declare the ctor as private. > > > > On another note, I still don't see the point with a Create() method instead of > a > > ctor. Will it become clear in future CLs? > > Thank you for bringing up this discussion. > > AudioNetworkAdaptor as an abstract class cannot be instantiated, so ctor will > not do a job. Create() is meant to instantiate an implementational class of it > as default. But yes, we can run without default and ask the owner class to > initiate the implementational class. > > Upon a second thought, I think we should remove Create(). > > Since ctor will be implicitly declared, we can put it here, although it cannot > instantiate. It may make sense to treat AudioNetworkAdaptor like any other pure interface. In which case you should have a public virtual destructor, not have any member variables, not mention any constructor (letting it be implicitly public), not using RTC_DISALLOW_COPY_AND_ASSIGN (not needed), and not have a Create function in this file. In audio_network_adaptor_impl.h, declare CreateAudioNetworkAdapter. In the corresponding .cc file, define that function; to do so, define a final subclass of AudioNetworkAdaptor in an anonymous namespace and return a unique_ptr to an instance of that. https://codereview.webrtc.org/2308573002/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor.cc (right): https://codereview.webrtc.org/2308573002/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor.cc:22: AudioNetworkAdaptor::~AudioNetworkAdaptor() = default; I think this one can be inline, since it'll be a no-op because the class has no state.
Patchset #4 (id:70001) has been deleted
Patchset #4 (id:90001) has been deleted
Thanks for the suggestions! PTAL at PS4. https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h (right): https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:12: #define WEBRTC_MODULES_AUDIO_CODING_AUDIO_NETWORK_ADAPTOR_INCLUDE_AUDIO_NETWORK_ADAPTOR_H_ On 2016/09/06 12:23:23, kwiberg-webrtc wrote: > On 2016/09/06 09:18:05, hlundin-webrtc wrote: > > On 2016/09/05 13:36:46, kwiberg-webrtc wrote: > > > Is there any way to rename files and directories so that this will fit in 80 > > > columns? > > > > I say don't bother with that. > > I'd be pushing harder for this if I had a reasonable alternative in mind, but > since I don't, I'll just grumble a bit and hope I won't have to look at these > too-long path names to often. that reads nicely. https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:42: AudioNetworkAdaptor(); On 2016/09/06 12:23:23, kwiberg-webrtc wrote: > On 2016/09/06 10:15:27, minyue-webrtc wrote: > > On 2016/09/06 09:18:05, hlundin-webrtc wrote: > > > On 2016/09/05 13:12:39, minyue-webrtc wrote: > > > > this should be removed. > > > > > > (kwiberg, please keep me honest here.) > > > Now that you have no ctor declared, the compiler will make an implicitly > > > declared one for you. So, a user will still be able to construct an ANA from > > > using the ctor instead of Create(). If this is not what you want, you will > > have > > > to explicitly declare the ctor as private. > > > > > > On another note, I still don't see the point with a Create() method instead > of > > a > > > ctor. Will it become clear in future CLs? > > > > Thank you for bringing up this discussion. > > > > AudioNetworkAdaptor as an abstract class cannot be instantiated, so ctor will > > not do a job. Create() is meant to instantiate an implementational class of it > > as default. But yes, we can run without default and ask the owner class to > > initiate the implementational class. > > > > Upon a second thought, I think we should remove Create(). > > > > Since ctor will be implicitly declared, we can put it here, although it cannot > > instantiate. > > It may make sense to treat AudioNetworkAdaptor like any other pure interface. In > which case you should have a public virtual destructor, not have any member > variables, not mention any constructor (letting it be implicitly public), not > using RTC_DISALLOW_COPY_AND_ASSIGN (not needed), and not have a Create function > in this file. > > In audio_network_adaptor_impl.h, declare CreateAudioNetworkAdapter. In the > corresponding .cc file, define that function; to do so, define a final subclass > of AudioNetworkAdaptor in an anonymous namespace and return a unique_ptr to an > instance of that. Sure. Thanks. I will follow these. For curiosity, don't we put a ctor in protected methods to make it more of an abstract class (if we accidentally remove pure virtual methods, the class remains un-instantiable)? https://codereview.webrtc.org/2308573002/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor.cc (right): https://codereview.webrtc.org/2308573002/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor.cc:22: AudioNetworkAdaptor::~AudioNetworkAdaptor() = default; On 2016/09/06 12:23:24, kwiberg-webrtc wrote: > I think this one can be inline, since it'll be a no-op because the class has no > state. Done.
https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h (right): https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:42: AudioNetworkAdaptor(); On 2016/09/06 13:22:20, minyue-webrtc wrote: > On 2016/09/06 12:23:23, kwiberg-webrtc wrote: > > On 2016/09/06 10:15:27, minyue-webrtc wrote: > > > On 2016/09/06 09:18:05, hlundin-webrtc wrote: > > > > On 2016/09/05 13:12:39, minyue-webrtc wrote: > > > > > this should be removed. > > > > > > > > (kwiberg, please keep me honest here.) > > > > Now that you have no ctor declared, the compiler will make an implicitly > > > > declared one for you. So, a user will still be able to construct an ANA > from > > > > using the ctor instead of Create(). If this is not what you want, you will > > > have > > > > to explicitly declare the ctor as private. > > > > > > > > On another note, I still don't see the point with a Create() method > instead > > of > > > a > > > > ctor. Will it become clear in future CLs? > > > > > > Thank you for bringing up this discussion. > > > > > > AudioNetworkAdaptor as an abstract class cannot be instantiated, so ctor > will > > > not do a job. Create() is meant to instantiate an implementational class of > it > > > as default. But yes, we can run without default and ask the owner class to > > > initiate the implementational class. > > > > > > Upon a second thought, I think we should remove Create(). > > > > > > Since ctor will be implicitly declared, we can put it here, although it > cannot > > > instantiate. > > > > It may make sense to treat AudioNetworkAdaptor like any other pure interface. > In > > which case you should have a public virtual destructor, not have any member > > variables, not mention any constructor (letting it be implicitly public), not > > using RTC_DISALLOW_COPY_AND_ASSIGN (not needed), and not have a Create > function > > in this file. > > > > In audio_network_adaptor_impl.h, declare CreateAudioNetworkAdapter. In the > > corresponding .cc file, define that function; to do so, define a final > subclass > > of AudioNetworkAdaptor in an anonymous namespace and return a unique_ptr to an > > instance of that. > > Sure. Thanks. I will follow these. > > For curiosity, don't we put a ctor in protected methods to make it more of an > abstract class (if we accidentally remove pure virtual methods, the class > remains un-instantiable)? In principle yes, but since an interface has only pure virtual methods, accidentally removing all of them results in an empty interface, so that particular mistake is unlikely to occur. Meaning that the advantages of making the constructor protected don't make up for the cost.
https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h (right): https://codereview.webrtc.org/2308573002/diff/10001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:42: AudioNetworkAdaptor(); On 2016/09/06 13:28:50, kwiberg-webrtc wrote: > On 2016/09/06 13:22:20, minyue-webrtc wrote: > > On 2016/09/06 12:23:23, kwiberg-webrtc wrote: > > > On 2016/09/06 10:15:27, minyue-webrtc wrote: > > > > On 2016/09/06 09:18:05, hlundin-webrtc wrote: > > > > > On 2016/09/05 13:12:39, minyue-webrtc wrote: > > > > > > this should be removed. > > > > > > > > > > (kwiberg, please keep me honest here.) > > > > > Now that you have no ctor declared, the compiler will make an implicitly > > > > > declared one for you. So, a user will still be able to construct an ANA > > from > > > > > using the ctor instead of Create(). If this is not what you want, you > will > > > > have > > > > > to explicitly declare the ctor as private. > > > > > > > > > > On another note, I still don't see the point with a Create() method > > instead > > > of > > > > a > > > > > ctor. Will it become clear in future CLs? > > > > > > > > Thank you for bringing up this discussion. > > > > > > > > AudioNetworkAdaptor as an abstract class cannot be instantiated, so ctor > > will > > > > not do a job. Create() is meant to instantiate an implementational class > of > > it > > > > as default. But yes, we can run without default and ask the owner class to > > > > initiate the implementational class. > > > > > > > > Upon a second thought, I think we should remove Create(). > > > > > > > > Since ctor will be implicitly declared, we can put it here, although it > > cannot > > > > instantiate. > > > > > > It may make sense to treat AudioNetworkAdaptor like any other pure > interface. > > In > > > which case you should have a public virtual destructor, not have any member > > > variables, not mention any constructor (letting it be implicitly public), > not > > > using RTC_DISALLOW_COPY_AND_ASSIGN (not needed), and not have a Create > > function > > > in this file. > > > > > > In audio_network_adaptor_impl.h, declare CreateAudioNetworkAdapter. In the > > > corresponding .cc file, define that function; to do so, define a final > > subclass > > > of AudioNetworkAdaptor in an anonymous namespace and return a unique_ptr to > an > > > instance of that. > > > > Sure. Thanks. I will follow these. > > > > For curiosity, don't we put a ctor in protected methods to make it more of an > > abstract class (if we accidentally remove pure virtual methods, the class > > remains un-instantiable)? > > In principle yes, but since an interface has only pure virtual methods, > accidentally removing all of them results in an empty interface, so that > particular mistake is unlikely to occur. Meaning that the advantages of making > the constructor protected don't make up for the cost. It makes perfect sense. Thanks for the explanation. and it is what PS4 follows.
lgtm
lgtm
The CQ bit was checked by minyue@webrtc.org
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: linux_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by minyue@webrtc.org
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: linux_baremetal on master.tryserver.webrtc (JOB_FAILED, no build URL)
The CQ bit was checked by minyue@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 ========== Adding AudioNetworkAdaptor interfaces. AudioNetworkAdaptor is supposed to facilitate AudioEncoder to adapt to varying network conditions. This is the first of a sequence of CLs that are to add one implementation of AudioNetworkAdaptor. This CL illustrates the interfaces of the AudioNetworkAdaptor. BUG=webrtc:6303 ========== to ========== Adding AudioNetworkAdaptor interfaces. AudioNetworkAdaptor is supposed to facilitate AudioEncoder to adapt to varying network conditions. This is the first of a sequence of CLs that are to add one implementation of AudioNetworkAdaptor. This CL illustrates the interfaces of the AudioNetworkAdaptor. BUG=webrtc:6303 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:110001)
Message was sent while issue was closed.
Description was changed from ========== Adding AudioNetworkAdaptor interfaces. AudioNetworkAdaptor is supposed to facilitate AudioEncoder to adapt to varying network conditions. This is the first of a sequence of CLs that are to add one implementation of AudioNetworkAdaptor. This CL illustrates the interfaces of the AudioNetworkAdaptor. BUG=webrtc:6303 ========== to ========== Adding AudioNetworkAdaptor interfaces. AudioNetworkAdaptor is supposed to facilitate AudioEncoder to adapt to varying network conditions. This is the first of a sequence of CLs that are to add one implementation of AudioNetworkAdaptor. This CL illustrates the interfaces of the AudioNetworkAdaptor. BUG=webrtc:6303 Committed: https://crrev.com/7610f85a2bfe2878450914d78e6a85639515275f Cr-Commit-Position: refs/heads/master@{#14115} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7610f85a2bfe2878450914d78e6a85639515275f Cr-Commit-Position: refs/heads/master@{#14115} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
