Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1844583003:
Only split into bands when the reverse stream is analyzed in the APM (Closed)
|
Created:
4 years, 8 months ago by aluebs-webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionOnly split into bands when the reverse stream is analyzed in the APM
BUG=596079
R=henrik.lundin@webrtc.org, peah@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/63a2c13d6dddab0331fb1fff42620c3c694f8017
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move to same function #Patch Set 3 : Rebasing #
Messages
Total messages: 18 (9 generated)
Description was changed from ========== Only split into bands when the reverse stream is analyzed in the APM ========== to ========== Only split into bands when the reverse stream is analyzed in the APM BUG=596079 ==========
aluebs@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, ivoc@webrtc.org
henrik.lundin@webrtc.org changed reviewers: + peah@webrtc.org
henrik.lundin@webrtc.org changed required reviewers: + peah@webrtc.org
lgtm, but I'd like peah to take a look too.
https://codereview.webrtc.org/1844583003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1844583003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:1150: return is_rev_processed() || As I see it, the purpose of this method is to avoid having to do reverse stream synthesis when it is not necessary, right? That is a great change indeed. I think this is an excellent time to simplify this scheme. Currently we have: -fwd_analysis_needed() -fwd_synthesis_needed() -rev_synthesis_needed() -rev_analysis_needed() -is_data_processed() -output_copy_needed() -is_rev_processed() -is_rev_analyzed() Which basically all have the purpose of determining whether -an output copy is needed for the stream, -whether a synthesis needs to be done. -whether an analysis needs to be done. It should be possible to simplify this quite a lot. For instance: -It would make sense to have matching name to their reverse stream counterparts for the methods: --is_data_processed() --output_copy_needed() -The methods rev_analysis_needed() and is_rev_analyzed() do different things, but have names which indicate that they are exactly the same thing (there should be no case where the reverse stream has not been analyzed even though it has been flagged that it needs to be analyzed). -is_rev_processed indicates that there is only reverse side processing being done if that is true. That is, however, not really the case as the band-splitting filter merge needed for the other components also modifies the stream. Therefore it would make sense to bundle these methods into one method.
https://codereview.webrtc.org/1844583003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1844583003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:1150: return is_rev_processed() || On 2016/03/30 13:01:11, peah-webrtc wrote: > As I see it, the purpose of this method is to avoid having to do reverse stream > synthesis when it is not necessary, right? That is a great change indeed. > > I think this is an excellent time to simplify this scheme. > > Currently we have: > -fwd_analysis_needed() > -fwd_synthesis_needed() > -rev_synthesis_needed() > -rev_analysis_needed() > -is_data_processed() > -output_copy_needed() > -is_rev_processed() > -is_rev_analyzed() > Which basically all have the purpose of determining whether > -an output copy is needed for the stream, > -whether a synthesis needs to be done. > -whether an analysis needs to be done. > > It should be possible to simplify this quite a lot. > For instance: > -It would make sense to have matching name to their reverse stream counterparts > for the methods: > --is_data_processed() > --output_copy_needed() > -The methods rev_analysis_needed() and is_rev_analyzed() do different things, > but have names which indicate that they are exactly the same thing (there should > be no case where the reverse stream has not been analyzed even though it has > been flagged that it needs to be analyzed). > -is_rev_processed indicates that there is only reverse side processing being > done if that is true. That is, however, not really the case as the > band-splitting filter merge needed for the other components also modifies the > stream. Therefore it would make sense to bundle these methods into one method. > No, this change is to avoid doing reverse stream analysis when it is not necessary. I think that rev_analysis_needed and is_rev_analyzed were easier to read in different functions, but the naming was probably chosen poorly by me. Anyway, I unified them, since the new function was only used in one place. I definitively agree that we can simplify these functions, but I am hesitant to mix that kind of refactoring into a CL that is supposed to change the behavior, like this one. Although renaming I think is ok, there is no reverse counterpart to output_copy_needed, because it is not as simple as you describe it. Unfortunately, because the AnalyzeReverseStream API is not deprecated yet, it we need to keep track manually of the output_copy and conversions needed. is_rev_processed indicates there is reverse processing done, but not necessarily only processing or not only in the reverse stream. And the merging would not happen if no processing was done, so it still holds up.
On 2016/03/31 01:00:19, aluebs-webrtc wrote: > https://codereview.webrtc.org/1844583003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/1844583003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/audio_processing_impl.cc:1150: return > is_rev_processed() || > On 2016/03/30 13:01:11, peah-webrtc wrote: > > As I see it, the purpose of this method is to avoid having to do reverse > stream > > synthesis when it is not necessary, right? That is a great change indeed. > > > > I think this is an excellent time to simplify this scheme. > > > > Currently we have: > > -fwd_analysis_needed() > > -fwd_synthesis_needed() > > -rev_synthesis_needed() > > -rev_analysis_needed() > > -is_data_processed() > > -output_copy_needed() > > -is_rev_processed() > > -is_rev_analyzed() > > Which basically all have the purpose of determining whether > > -an output copy is needed for the stream, > > -whether a synthesis needs to be done. > > -whether an analysis needs to be done. > > > > It should be possible to simplify this quite a lot. > > For instance: > > -It would make sense to have matching name to their reverse stream > counterparts > > for the methods: > > --is_data_processed() > > --output_copy_needed() > > -The methods rev_analysis_needed() and is_rev_analyzed() do different things, > > but have names which indicate that they are exactly the same thing (there > should > > be no case where the reverse stream has not been analyzed even though it has > > been flagged that it needs to be analyzed). > > -is_rev_processed indicates that there is only reverse side processing being > > done if that is true. That is, however, not really the case as the > > band-splitting filter merge needed for the other components also modifies the > > stream. Therefore it would make sense to bundle these methods into one method. > > > > No, this change is to avoid doing reverse stream analysis when it is not > necessary. > I think that rev_analysis_needed and is_rev_analyzed were easier to read in > different functions, but the naming was probably chosen poorly by me. Anyway, I > unified them, since the new function was only used in one place. > I definitively agree that we can simplify these functions, but I am hesitant to > mix that kind of refactoring into a CL that is supposed to change the behavior, > like this one. Although renaming I think is ok, there is no reverse counterpart > to output_copy_needed, because it is not as simple as you describe it. > Unfortunately, because the AnalyzeReverseStream API is not deprecated yet, it we > need to keep track manually of the output_copy and conversions needed. > is_rev_processed indicates there is reverse processing done, but not necessarily > only processing or not only in the reverse stream. And the merging would not > happen if no processing was done, so it still holds up. lgtm You are right, it definitely makes sense to separate that refactoring into another CL.
The CQ bit was checked by aluebs@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1844583003/#ps20001 (title: "Move to same function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1844583003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1844583003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Message was sent while issue was closed.
Description was changed from ========== Only split into bands when the reverse stream is analyzed in the APM BUG=596079 ========== to ========== Only split into bands when the reverse stream is analyzed in the APM BUG=596079 R=henrik.lundin@webrtc.org, peah@webrtc.org Committed: https://crrev.com/63a2c13d6dddab0331fb1fff42620c3c694f8017 Cr-Commit-Position: refs/heads/master@{#12187} ==========
Message was sent while issue was closed.
Description was changed from ========== Only split into bands when the reverse stream is analyzed in the APM BUG=596079 ========== to ========== Only split into bands when the reverse stream is analyzed in the APM BUG=596079 R=henrik.lundin@webrtc.org, peah@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/63a2c13d6dddab0331fb1fff4... ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/63a2c13d6dddab0331fb1fff42620c3c694f8017 Cr-Commit-Position: refs/heads/master@{#12187}
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 63a2c13d6dddab0331fb1fff42620c3c694f8017 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
