Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1236793003:
Add LoudestFilter in ConferenceTransport (Closed)
|
Created:
5 years, 5 months ago by minyue-webrtc Modified:
5 years, 3 months ago CC:
henrika_webrtc, tterriberry_mozilla.com, webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd LoudestFilter in ConferenceTransport
BUG=
Committed: https://crrev.com/03bb7c7bfac45e662e2159c111fb7f9bdb006d18
Cr-Commit-Position: refs/heads/master@{#9712}
Patch Set 1 : #
Total comments: 29
Patch Set 2 : after comments #
Total comments: 1
Patch Set 3 : removing silence.pcm #
Total comments: 37
Patch Set 4 : after rebasing #Patch Set 5 : after comments #
Total comments: 6
Patch Set 6 : renaming a funtion #
Total comments: 2
Patch Set 7 : fixing an indentation #
Total comments: 5
Patch Set 8 : final cleaning up #Patch Set 9 : avoiding C++11 map.erase signature #
Messages
Total messages: 46 (11 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
minyue@webrtc.org changed reviewers: + phoglund@webrtc.org, tina.legrand@webrtc.org
Hi Tina, This CL adds a loudness filter to the conference transport, to simulate a select-loudest-to-forward operation, which is is used in many conferencing applications. You may take a look first and I will ask phoglund@ to further approve it when he comes back.
Drive-by: is it necessary to add a file representing silence? Generating test vectors in code is more clear to the reader, easier to modify and usually faster than reading a file anyway. (Silence seems particularly easy to generate :)
On 2015/08/04 21:06:51, ajm wrote: > Drive-by: is it necessary to add a file representing silence? Generating test > vectors in code is more clear to the reader, easier to modify and usually faster > than reading a file anyway. > > (Silence seems particularly easy to generate :) I thought about this, too. I could not find in VoE APIs something like playAsMic with non-file.
Great work! I have some comments inline. https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:42: return kInvalidAudioLevel; If we end up here, the RTP header is too short to contain an audio level value, but we report invalid value? https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:45: const uint32_t first_word = rtc::GetBE32(data); Can we add a more descriptive variable name? What does the first 32 bits(?) contain? https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:47: printf("No extension %x.\n", first_word); Is the printf needed? https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:51: int cc = (first_word >> 24) & 0x0f; // CSRC count, to calculate header size cc -> csrc_count, perhaps? https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:52: size_t ext_pos = 12 + cc * 4; The variable names are a bit confusing, since they seem to be abreviations. Consider changing to make them more descriptive, or add comments that explains what the code does. Example: ext_pos, ext_type, ext_length. https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:72: uint8_t b = rtc::Get8(data, ext_pos + 4 + offset); Change "b" to a descriptive name. https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:86: // two-byte format Maybe you can save some code lines by only using the if-statement to decide where to read the data. I think it is possible to combine the code on line 74-83 and 94-101 https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.h (right): https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:30: #include "webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h" CL description talks about Loudness filter, but in the code you call it "loudest". Which one is correct? Should it be named loudness_filter.h instead? https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:61: * file_name : name of the file to be added as microphone input, Nit , -> . https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:160: LoudestFilter loudest_filter_; Should this be "LoudnessFilter loudness_filter" instead of "LoudestFilter ..."? https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:147: // Cannot be accurate since stream 0 started the earliest. Can you explain this comment? I'm not following.
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Quick drive-by comment, before you start changing too much. Also, the CL title says "LoudnessFilter" which doesn't match the class name. (I like the class name better.) https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:38: // Parses audio level in RTP header extension according to I suggest you use RtpHeaderParser from webrtc/modules/rtp_rtcp/interface/rtp_header_parser.h.
https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:38: // Parses audio level in RTP header extension according to On 2015/08/05 14:04:24, hlundin-webrtc wrote: > I suggest you use RtpHeaderParser from > webrtc/modules/rtp_rtcp/interface/rtp_header_parser.h. I actually tried it, but it did not fit here very well. I forgot what the problem was. I'll reconsider it.
https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:38: // Parses audio level in RTP header extension according to On 2015/08/05 14:15:16, minyue-webrtc wrote: > On 2015/08/05 14:04:24, hlundin-webrtc wrote: > > I suggest you use RtpHeaderParser from > > webrtc/modules/rtp_rtcp/interface/rtp_header_parser.h. > > I actually tried it, but it did not fit here very well. I forgot what the > problem was. I'll reconsider it. OK. I used it in webrtc::test::RtpFileSource. Look there and see if our use cases match.
On 2015/08/05 15:41:19, hlundin-webrtc wrote: > https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... > File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): > > https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:38: // Parses > audio level in RTP header extension according to > On 2015/08/05 14:15:16, minyue-webrtc wrote: > > On 2015/08/05 14:04:24, hlundin-webrtc wrote: > > > I suggest you use RtpHeaderParser from > > > webrtc/modules/rtp_rtcp/interface/rtp_header_parser.h. > > > > I actually tried it, but it did not fit here very well. I forgot what the > > problem was. I'll reconsider it. > > OK. I used it in webrtc::test::RtpFileSource. Look there and see if our use > cases match. Now I get it. The audio level returned by RtpHeaderParser must be & 0x7f to be correct. I did not realize this and thought that RtpHeaderParser parses full header and may be less efficient (since we need only audio level here) Now I think it can be good to use RtpHeaderParser to avoid code replication. WDYT?
On 2015/08/05 10:17:30, minyue-webrtc wrote: > On 2015/08/04 21:06:51, ajm wrote: > > Drive-by: is it necessary to add a file representing silence? Generating test > > vectors in code is more clear to the reader, easier to modify and usually > faster > > than reading a file anyway. > > > > (Silence seems particularly easy to generate :) > > I thought about this, too. > > I could not find in VoE APIs something like playAsMic with non-file. Ah, I see. You might consider refactoring things to use voice engine's external media processing interface. Then you can dump in whatever you want. That could be a big change, but you should at least consider writing your own temp file in the test, rather than checking one in: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
andrew@webrtc.org changed reviewers: + andrew@webrtc.org
https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:38: // Parses audio level in RTP header extension according to On 2015/08/05 15:41:19, hlundin-webrtc wrote: > On 2015/08/05 14:15:16, minyue-webrtc wrote: > > On 2015/08/05 14:04:24, hlundin-webrtc wrote: > > > I suggest you use RtpHeaderParser from > > > webrtc/modules/rtp_rtcp/interface/rtp_header_parser.h. > > > > I actually tried it, but it did not fit here very well. I forgot what the > > problem was. I'll reconsider it. > > OK. I used it in webrtc::test::RtpFileSource. Look there and see if our use > cases match. In any case, you really don't want to be doing this header parsing in a test file. The existing one should be refactored to meet your needs, but if that really doesn't fit, you should lift this out and give it its own unit test. https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:31: const std::string kInputFileName = nit: not a compile-time const.
On 2015/08/05 16:30:17, minyue-webrtc wrote: > On 2015/08/05 15:41:19, hlundin-webrtc wrote: > > > https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... > > File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): > > > > > https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... > > webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:38: // Parses > > audio level in RTP header extension according to > > On 2015/08/05 14:15:16, minyue-webrtc wrote: > > > On 2015/08/05 14:04:24, hlundin-webrtc wrote: > > > > I suggest you use RtpHeaderParser from > > > > webrtc/modules/rtp_rtcp/interface/rtp_header_parser.h. > > > > > > I actually tried it, but it did not fit here very well. I forgot what the > > > problem was. I'll reconsider it. > > > > OK. I used it in webrtc::test::RtpFileSource. Look there and see if our use > > cases match. > > Now I get it. The audio level returned by RtpHeaderParser must be & 0x7f to be > correct. I did not realize this and thought that RtpHeaderParser parses full > header and may be less efficient (since we need only audio level here) > > Now I think it can be good to use RtpHeaderParser to avoid code replication. > WDYT? Right. Looking at the code in modules/rtp_rtcp/source/rtp_utility.cc, line 415 and on, it is clear that the parser doesn't separate the V-bit from the 7-bit level. This is unfortunate imo, and should probably be fixed. I don't know if the parser is used anywhere in production.
On 2015/08/05 16:36:41, andrew wrote: > On 2015/08/05 10:17:30, minyue-webrtc wrote: > > On 2015/08/04 21:06:51, ajm wrote: > > > Drive-by: is it necessary to add a file representing silence? Generating > test > > > vectors in code is more clear to the reader, easier to modify and usually > > faster > > > than reading a file anyway. > > > > > > (Silence seems particularly easy to generate :) > > > > I thought about this, too. > > > > I could not find in VoE APIs something like playAsMic with non-file. > > Ah, I see. You might consider refactoring things to use voice engine's external > media processing interface. Then you can dump in whatever you want. > > That could be a big change, but you should at least consider writing your own > temp file in the test, rather than checking one in: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... Hi, I am not sure if external media processing is proper. It seems that it is about mixing external audio for playing back, but we here want a fake mic pickup (which gets encoded and sent). But the idea of temp file is great. I will do.
Thank you for your comments! Here is an new patch set. The RTP header parser is not perfect, I am waiting for https://codereview.chromium.org/1272343003/ landed to make a rebase, a naive one. https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:29: static unsigned int ParseSsrc(const void* data, size_t len, bool rtcp) { It would be good to get rid of this parser too. I'd like to remove the RTP part in this function and leave the RTCP parsing to a separate CL. https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:38: // Parses audio level in RTP header extension according to On 2015/08/05 16:37:07, andrew wrote: > On 2015/08/05 15:41:19, hlundin-webrtc wrote: > > On 2015/08/05 14:15:16, minyue-webrtc wrote: > > > On 2015/08/05 14:04:24, hlundin-webrtc wrote: > > > > I suggest you use RtpHeaderParser from > > > > webrtc/modules/rtp_rtcp/interface/rtp_header_parser.h. > > > > > > I actually tried it, but it did not fit here very well. I forgot what the > > > problem was. I'll reconsider it. > > > > OK. I used it in webrtc::test::RtpFileSource. Look there and see if our use > > cases match. > > In any case, you really don't want to be doing this header parsing in a test > file. The existing one should be refactored to meet your needs, but if that > really doesn't fit, you should lift this out and give it its own unit test. Thanks. I agree that it is better to use existing tool. and I found the reason that the existing tool did not fit, and made it a separate CL. https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:42: return kInvalidAudioLevel; On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > If we end up here, the RTP header is too short to contain an audio level value, > but we report invalid value? Sorry Tina, other reviewers want these function to be got rid of. I agree with them. So I will skip your comments in this function. Sorry for the inconvenience. https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.h (right): https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:30: #include "webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h" On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > CL description talks about Loudness filter, but in the code you call it > "loudest". Which one is correct? Should it be named loudness_filter.h instead? The CL title was made wrong, and I have changed it. https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:61: * file_name : name of the file to be added as microphone input, On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > Nit , -> . Ok. I thought "," was more correct, since it is an inner item in a list. But I think you know the style better. I have changed it. https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:160: LoudestFilter loudest_filter_; On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > Should this be "LoudnessFilter loudness_filter" instead of "LoudestFilter ..."? Henrik and I like Loudest better, so I changed the CL description. https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:31: const std::string kInputFileName = On 2015/08/05 16:37:07, andrew wrote: > nit: not a compile-time const. Done. https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:147: // Cannot be accurate since stream 0 started the earliest. On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > Can you explain this comment? I'm not following. I have updated the comments.
On 2015/08/06 13:31:22, minyue-webrtc wrote: > Thank you for your comments! Here is an new patch set. > > The RTP header parser is not perfect, I am waiting for > https://codereview.chromium.org/1272343003/ landed to make a rebase, a naive > one. > > https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... > File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): > > https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:29: static > unsigned int ParseSsrc(const void* data, size_t len, bool rtcp) { > It would be good to get rid of this parser too. I'd like to remove the RTP part > in this function and leave the RTCP parsing to a separate CL. > > https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:38: // Parses > audio level in RTP header extension according to > On 2015/08/05 16:37:07, andrew wrote: > > On 2015/08/05 15:41:19, hlundin-webrtc wrote: > > > On 2015/08/05 14:15:16, minyue-webrtc wrote: > > > > On 2015/08/05 14:04:24, hlundin-webrtc wrote: > > > > > I suggest you use RtpHeaderParser from > > > > > webrtc/modules/rtp_rtcp/interface/rtp_header_parser.h. > > > > > > > > I actually tried it, but it did not fit here very well. I forgot what the > > > > problem was. I'll reconsider it. > > > > > > OK. I used it in webrtc::test::RtpFileSource. Look there and see if our use > > > cases match. > > > > In any case, you really don't want to be doing this header parsing in a test > > file. The existing one should be refactored to meet your needs, but if that > > really doesn't fit, you should lift this out and give it its own unit test. > > Thanks. I agree that it is better to use existing tool. > > and I found the reason that the existing tool did not fit, and made it a > separate CL. > > https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:42: return > kInvalidAudioLevel; > On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > > If we end up here, the RTP header is too short to contain an audio level > value, > > but we report invalid value? > > Sorry Tina, other reviewers want these function to be got rid of. I agree with > them. So I will skip your comments in this function. Sorry for the > inconvenience. > Not a problem at all! :) > https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... > File webrtc/voice_engine/test/auto_test/fakes/conference_transport.h (right): > > https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:30: #include > "webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h" > On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > > CL description talks about Loudness filter, but in the code you call it > > "loudest". Which one is correct? Should it be named loudness_filter.h instead? > > The CL title was made wrong, and I have changed it. > > https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:61: * > file_name : name of the file to be added as microphone input, > On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > > Nit , -> . > > Ok. I thought "," was more correct, since it is an inner item in a list. > But I think you know the style better. I have changed it. > > https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:160: > LoudestFilter loudest_filter_; > On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > > Should this be "LoudnessFilter loudness_filter" instead of "LoudestFilter > ..."? > > Henrik and I like Loudest better, so I changed the CL description. > > https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... > File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): > > https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/voe_conference_test.cc:31: const std::string > kInputFileName = > On 2015/08/05 16:37:07, andrew wrote: > > nit: not a compile-time const. > > Done. > > https://codereview.chromium.org/1236793003/diff/40001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/voe_conference_test.cc:147: // Cannot be > accurate since stream 0 started the earliest. > On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > > Can you explain this comment? I'm not following. > > I have updated the comments.
LGTM, but please wait for input from Andrew and Henrik before committing. I think you can remove silence.pcm.sha1 from the CL too. https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.h (right): https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:61: * file_name : name of the file to be added as microphone input, On 2015/08/06 13:31:22, minyue-webrtc wrote: > On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > > Nit , -> . > > Ok. I thought "," was more correct, since it is an inner item in a list. > But I think you know the style better. I have changed it. Aha. Not sure about the style guide, but I read it as separate sentences. Maybe take a look at other places in WebRTC and pick the most common? https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:160: LoudestFilter loudest_filter_; On 2015/08/06 13:31:22, minyue-webrtc wrote: > On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > > Should this be "LoudnessFilter loudness_filter" instead of "LoudestFilter > ..."? > > Henrik and I like Loudest better, so I changed the CL description. Acknowledged. https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:147: // Cannot be accurate since stream 0 started the earliest. On 2015/08/06 13:31:22, minyue-webrtc wrote: > On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > > Can you explain this comment? I'm not following. > > I have updated the comments. Acknowledged.
one question to Andrew https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:31: const std::string kInputFileName = On 2015/08/05 16:37:07, andrew wrote: > nit: not a compile-time const. Hi Andrew, Per offline talk with Tina, I think I have not addressed your comment here correctly. Do you suggest the that the name should not have a k-?
On 2015/08/06 14:51:55, tlegrand-webrtc wrote: > LGTM, but please wait for input from Andrew and Henrik before committing. I > think you can remove silence.pcm.sha1 from the CL too. Yes, I forgot to remove the file, and have put a note on myself. > > https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... > File webrtc/voice_engine/test/auto_test/fakes/conference_transport.h (right): > > https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:61: * > file_name : name of the file to be added as microphone input, > On 2015/08/06 13:31:22, minyue-webrtc wrote: > > On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > > > Nit , -> . > > > > Ok. I thought "," was more correct, since it is an inner item in a list. > > But I think you know the style better. I have changed it. > > Aha. Not sure about the style guide, but I read it as separate sentences. Maybe > take a look at other places in WebRTC and pick the most common? > > https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:160: > LoudestFilter loudest_filter_; > On 2015/08/06 13:31:22, minyue-webrtc wrote: > > On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > > > Should this be "LoudnessFilter loudness_filter" instead of "LoudestFilter > > ..."? > > > > Henrik and I like Loudest better, so I changed the CL description. > > Acknowledged. > > https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... > File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): > > https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... > webrtc/voice_engine/test/auto_test/voe_conference_test.cc:147: // Cannot be > accurate since stream 0 started the earliest. > On 2015/08/06 13:31:22, minyue-webrtc wrote: > > On 2015/08/05 13:32:44, tlegrand-webrtc wrote: > > > Can you explain this comment? I'm not following. > > > > I have updated the comments. > > Acknowledged.
https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:157: rtp_header.extension.audioLevel & 0x7f : kInvalidAudioLevel; Nit: indent https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h (right): https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:21: /* DecideForward() I think this method name is hard to understand. I suppose you mean "decide whether to forward", but a filter doesn't forward things, but rather filters stuff out. Maybe ShouldFilter() is a better name? https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:119: const std::string silence_file = Extract this to a helper method CreateTempSilenceFile.
https://codereview.webrtc.org/1236793003/diff/60001/resources/audio_coding/si... File resources/audio_coding/silence.pcm.sha1 (right): https://codereview.webrtc.org/1236793003/diff/60001/resources/audio_coding/si... resources/audio_coding/silence.pcm.sha1:1: 6be3f18df0553904a151b26f5d197a0be11f466e removed https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:157: rtp_header.extension.audioLevel & 0x7f : kInvalidAudioLevel; On 2015/08/10 08:43:27, phoglund wrote: > Nit: indent Yes. and I expect here a rebase after https://codereview.chromium.org/1272343003/ https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h (right): https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:21: /* DecideForward() On 2015/08/10 08:43:27, phoglund wrote: > I think this method name is hard to understand. I suppose you mean "decide > whether to forward", but a filter doesn't forward things, but rather filters > stuff out. Maybe ShouldFilter() is a better name? Good point. May I suggest ShouldPass()? ShouldFilter() might be a bit ambiguous for being filtered out and being passed through. https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:119: const std::string silence_file = On 2015/08/10 08:43:28, phoglund wrote: > Extract this to a helper method CreateTempSilenceFile. ok, will do.
LG, but I have a few comments. https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:26: static const int kEnergyHeaderId = 1; Why call it energy header? I suggest kAudioLevelId, or something else including "audio level". https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:158: if (!loudest_filter_.DecideForward(rtp_header.ssrc, audio_level)) { I think the code will be more readable if you reverse the logic of this if-statement. That is: if (loudest_filter_.DecideForward(...)) { destination = GetReceiver... ... } https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:158: if (!loudest_filter_.DecideForward(rtp_header.ssrc, audio_level)) { Why parse out two components of the webrtc::RTPHeader struct and then pass them to the same method? I think DecideForward should take a const webrtc::RTPHeader reference as input. https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc (right): https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:19: while (it != stream_levels_.end()) { I suggest you make this a for loop instead for (auto stream : stream_levels_) { ... But maybe you cannot call erase on the stream variable. If that doesn't work, you should still be able to use for (auto it = stream_levels_.begin(); it != c.end(); ) { ... There's an example at http://en.cppreference.com/w/cpp/container/map/erase. Note also how in that example they do not ++ the iterator when erase is called, but rather update it with the output from the erase method. https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:20: if (rtc::TimeDiff(time_ms, it->second.last_time_ms_) > Oh, is that why you kept using uint32_t for the time variables? You may disregard from my earlier comments about this. https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:22: stream_levels_.erase(it++); I'm not sure it is valid after you called erase. "References and iterators to the erased elements are invalidated." https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:34: stream.second.audio_level_ > quietest_level) { Comment about the fact that a quiet stream has a high audio level. https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:67: if (audio_level < stream_levels_[quietest_ssrc].audio_level_) { Again, explain that "up is down, and down is up". https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h (right): https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:21: /* DecideForward() On 2015/08/10 09:30:22, minyue-webrtc wrote: > On 2015/08/10 08:43:27, phoglund wrote: > > I think this method name is hard to understand. I suppose you mean "decide > > whether to forward", but a filter doesn't forward things, but rather filters > > stuff out. Maybe ShouldFilter() is a better name? > > Good point. May I suggest ShouldPass()? > ShouldFilter() might be a bit ambiguous for being filtered out and being passed > through. I suggest ForwardThisPacket(). https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:22: * Decide whether to filter out a stream. "Decide whether to forward a packet." After all, this method is to be called for each packet, right? https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:33: audio_level_ = audio_level; Data members of structs don't have a trailing underscore. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:37: uint32 last_time_ms_; int https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:40: void RemoveTimeoutStreams(uint32 time_ms); int https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:46: const int32 kStreamTimeOutMs = 5000; const int
https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:158: if (!loudest_filter_.DecideForward(rtp_header.ssrc, audio_level)) { On 2015/08/10 10:50:17, hlundin-webrtc wrote: > Why parse out two components of the webrtc::RTPHeader struct and then pass them > to the same method? I think DecideForward should take a const webrtc::RTPHeader > reference as input. +1 https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h (right): https://codereview.webrtc.org/1236793003/diff/80001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:21: /* DecideForward() On 2015/08/10 10:50:18, hlundin-webrtc wrote: > On 2015/08/10 09:30:22, minyue-webrtc wrote: > > On 2015/08/10 08:43:27, phoglund wrote: > > > I think this method name is hard to understand. I suppose you mean "decide > > > whether to forward", but a filter doesn't forward things, but rather filters > > > stuff out. Maybe ShouldFilter() is a better name? > > > > Good point. May I suggest ShouldPass()? > > ShouldFilter() might be a bit ambiguous for being filtered out and being > passed > > through. > > I suggest ForwardThisPacket(). +1
Patchset #4 (id:100001) has been deleted
I think I covered all comments. Please take another look. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:26: static const int kEnergyHeaderId = 1; On 2015/08/10 10:50:17, hlundin-webrtc wrote: > Why call it energy header? I suggest kAudioLevelId, or something else including > "audio level". Now called kAudioLevelHeaderId https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:158: if (!loudest_filter_.DecideForward(rtp_header.ssrc, audio_level)) { On 2015/08/10 10:50:17, hlundin-webrtc wrote: > I think the code will be more readable if you reverse the logic of this > if-statement. That is: > if (loudest_filter_.DecideForward(...)) { > destination = GetReceiver... > ... > } Done. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:158: if (!loudest_filter_.DecideForward(rtp_header.ssrc, audio_level)) { On 2015/08/10 10:50:17, hlundin-webrtc wrote: > Why parse out two components of the webrtc::RTPHeader struct and then pass them > to the same method? I think DecideForward should take a const webrtc::RTPHeader > reference as input. Done. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:158: if (!loudest_filter_.DecideForward(rtp_header.ssrc, audio_level)) { On 2015/08/10 11:34:04, phoglund wrote: > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > Why parse out two components of the webrtc::RTPHeader struct and then pass > them > > to the same method? I think DecideForward should take a const > webrtc::RTPHeader > > reference as input. > > +1 Done. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... File webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc (right): https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:19: while (it != stream_levels_.end()) { On 2015/08/10 10:50:17, hlundin-webrtc wrote: > I suggest you make this a for loop instead > for (auto stream : stream_levels_) { > ... > But maybe you cannot call erase on the stream variable. > > If that doesn't work, you should still be able to use > for (auto it = stream_levels_.begin(); it != c.end(); ) { > ... > > There's an example at http://en.cppreference.com/w/cpp/container/map/erase. > Note also how in that example they do not ++ the iterator when erase is called, > but rather update it with the output from the erase method. I do not think for (auto stream : stream_levels_) can work. The "while" vs. "for" in the example you provided is a tie. I agree that it is better to use the C++11 std::map feature "it = erase(it)" https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:20: if (rtc::TimeDiff(time_ms, it->second.last_time_ms_) > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > Oh, is that why you kept using uint32_t for the time variables? You may > disregard from my earlier comments about this. Acknowledged. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:22: stream_levels_.erase(it++); On 2015/08/10 10:50:17, hlundin-webrtc wrote: > I'm not sure it is valid after you called erase. "References and iterators to > the erased elements are invalidated." I saw many examples of this on the net. It should be valid. but better to use C++11 feature of it = erase(it). I thought that I tried it and the compiler did not work. But now it does, which is nice. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:34: stream.second.audio_level_ > quietest_level) { On 2015/08/10 10:50:17, hlundin-webrtc wrote: > Comment about the fact that a quiet stream has a high audio level. Done. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:67: if (audio_level < stream_levels_[quietest_ssrc].audio_level_) { On 2015/08/10 10:50:17, hlundin-webrtc wrote: > Again, explain that "up is down, and down is up". Done. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... File webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h (right): https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:21: /* DecideForward() On 2015/08/10 11:34:04, phoglund wrote: > On 2015/08/10 10:50:18, hlundin-webrtc wrote: > > On 2015/08/10 09:30:22, minyue-webrtc wrote: > > > On 2015/08/10 08:43:27, phoglund wrote: > > > > I think this method name is hard to understand. I suppose you mean "decide > > > > whether to forward", but a filter doesn't forward things, but rather > filters > > > > stuff out. Maybe ShouldFilter() is a better name? > > > > > > Good point. May I suggest ShouldPass()? > > > ShouldFilter() might be a bit ambiguous for being filtered out and being > > passed > > > through. > > > > I suggest ForwardThisPacket(). > > +1 I am a bit confused. Did we say "forward" isn't an operation of filter? Combining the ideas, I tried to make a new name. Throw eggs if you don't like it. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:22: * Decide whether to filter out a stream. On 2015/08/10 10:50:18, hlundin-webrtc wrote: > "Decide whether to forward a packet." > After all, this method is to be called for each packet, right? Acknowledged. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:33: audio_level_ = audio_level; On 2015/08/10 10:50:17, hlundin-webrtc wrote: > Data members of structs don't have a trailing underscore. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names Acknowledged. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:37: uint32 last_time_ms_; On 2015/08/10 10:50:18, hlundin-webrtc wrote: > int Done. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:40: void RemoveTimeoutStreams(uint32 time_ms); On 2015/08/10 10:50:17, hlundin-webrtc wrote: > int Done. https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:46: const int32 kStreamTimeOutMs = 5000; On 2015/08/10 10:50:17, hlundin-webrtc wrote: > const int Done.
https://codereview.webrtc.org/1236793003/diff/140001/webrtc/voice_engine/test... File webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h (right): https://codereview.webrtc.org/1236793003/diff/140001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:28: bool IfPassRtpPacket(const webrtc::RTPHeader& rtp_header); Hmm. This sounds like the if statement is somehow embedded into the method (i.e. that it should call a callback when the predicate is true or something? I liked hlundin's ForwardThisPacket. Yeah, arguably forwarding as a concept isn't very related to filters, but I can't think of anything better. Also, update the comment.
On nit, and also some egg-throwing about a method name. Otherwise, lgtm. https://codereview.webrtc.org/1236793003/diff/140001/webrtc/voice_engine/test... File webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h (right): https://codereview.webrtc.org/1236793003/diff/140001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:28: bool IfPassRtpPacket(const webrtc::RTPHeader& rtp_header); On 2015/08/12 15:01:23, phoglund wrote: > Hmm. This sounds like the if statement is somehow embedded into the method (i.e. > that it should call a callback when the predicate is true or something? > > I liked hlundin's ForwardThisPacket. Yeah, arguably forwarding as a concept > isn't very related to filters, but I can't think of anything better. > > Also, update the comment. Virtual egg thrown. I agree with phoglund, to agree with my own suggestion. :) Yes, "forwarding" is maybe not a typical operation of a filter, but I'd turn the argument around and ask if what you are implementing really is a filter, or is it a selective packet router? Note: not arguing that you should change he name of the class. https://codereview.webrtc.org/1236793003/diff/140001/webrtc/voice_engine/test... File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): https://codereview.webrtc.org/1236793003/diff/140001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:29: int temp = 0; I don't think it matters in practice, but I think this should be an int16_t, since it represents one sample.
thanks, the name of the function is now settled. https://codereview.chromium.org/1236793003/diff/140001/webrtc/voice_engine/te... File webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h (right): https://codereview.chromium.org/1236793003/diff/140001/webrtc/voice_engine/te... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:28: bool IfPassRtpPacket(const webrtc::RTPHeader& rtp_header); On 2015/08/13 07:11:12, hlundin-webrtc wrote: > On 2015/08/12 15:01:23, phoglund wrote: > > Hmm. This sounds like the if statement is somehow embedded into the method > (i.e. > > that it should call a callback when the predicate is true or something? > > > > I liked hlundin's ForwardThisPacket. Yeah, arguably forwarding as a concept > > isn't very related to filters, but I can't think of anything better. > > > > Also, update the comment. > > Virtual egg thrown. I agree with phoglund, to agree with my own suggestion. :) > > Yes, "forwarding" is maybe not a typical operation of a filter, but I'd turn the > argument around and ask if what you are implementing really is a filter, or is > it a selective packet router? Note: not arguing that you should change he name > of the class. Done. https://codereview.chromium.org/1236793003/diff/140001/webrtc/voice_engine/te... webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:28: bool IfPassRtpPacket(const webrtc::RTPHeader& rtp_header); On 2015/08/12 15:01:23, phoglund wrote: > Hmm. This sounds like the if statement is somehow embedded into the method (i.e. > that it should call a callback when the predicate is true or something? > > I liked hlundin's ForwardThisPacket. Yeah, arguably forwarding as a concept > isn't very related to filters, but I can't think of anything better. > > Also, update the comment. Done. https://codereview.chromium.org/1236793003/diff/140001/webrtc/voice_engine/te... File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): https://codereview.chromium.org/1236793003/diff/140001/webrtc/voice_engine/te... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:29: int temp = 0; On 2015/08/13 07:11:12, hlundin-webrtc wrote: > I don't think it matters in practice, but I think this should be an int16_t, > since it represents one sample. Done.
lgtm
lgtm https://codereview.webrtc.org/1236793003/diff/160001/webrtc/voice_engine/test... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.webrtc.org/1236793003/diff/160001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:72: SetSendAudioLevelIndicationStatus(local_sender_, true, Nit: indent (should be 4 right?)
Ok. fixed. But here is a unsolved comment from Andrew. https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... I'll wait a bit before landing. https://codereview.webrtc.org/1236793003/diff/160001/webrtc/voice_engine/test... File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): https://codereview.webrtc.org/1236793003/diff/160001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:72: SetSendAudioLevelIndicationStatus(local_sender_, true, On 2015/08/13 11:54:56, phoglund wrote: > Nit: indent (should be 4 right?) true! thanks.
Didn't look closely, but lgtm with a bunch of minor changes. https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): https://codereview.webrtc.org/1236793003/diff/40001/webrtc/voice_engine/test/... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:31: const std::string kInputFileName = On 2015/08/06 14:57:14, minyue-webrtc wrote: > On 2015/08/05 16:37:07, andrew wrote: > > nit: not a compile-time const. > > Hi Andrew, > > Per offline talk with Tina, I think I have not addressed your comment here > correctly. Do you suggest the that the name should not have a k-? Right, it's not a compile-time const (i.e. can only be evaluated at run-time), so should use input_file_name. https://codereview.webrtc.org/1236793003/diff/180001/webrtc/voice_engine/test... File webrtc/voice_engine/test/auto_test/voe_conference_test.cc (right): https://codereview.webrtc.org/1236793003/diff/180001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:21: static const int kRttMs = 25; This block should not be indented. Also, static on all of these is redundant because they're in an unnamed namespace. https://codereview.webrtc.org/1236793003/diff/180001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:29: int16_t temp = 0; nit: const int16_t zero = 0; https://codereview.webrtc.org/1236793003/diff/180001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:30: for (int i = 0; i < 32000; i++) { nit: ++i Is 32000 fixed? Should either be a file-scope const or passed to the function. https://codereview.webrtc.org/1236793003/diff/180001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:32: fwrite(&temp, 2, 1, fid); sizeof(int16_t) https://codereview.webrtc.org/1236793003/diff/180001/webrtc/voice_engine/test... webrtc/voice_engine/test/auto_test/voe_conference_test.cc:37: static const std::string kInputFileName = Variables with static storage duration must be POD: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_G... So you'll need to declare this in each test. You can have the file path here to reduce duplication: const char* input_file_path = "audio_coding/testfile32kHz";
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tina.legrand@webrtc.org, henrik.lundin@webrtc.org, phoglund@webrtc.org, andrew@webrtc.org Link to the patchset: https://codereview.chromium.org/1236793003/#ps200001 (title: "final cleaning up")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236793003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1236793003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_rel/builds/8676)
On 2015/08/12 14:53:24, minyue-webrtc wrote: > I think I covered all comments. Please take another look. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > File webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc (right): > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:26: static > const int kEnergyHeaderId = 1; > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > Why call it energy header? I suggest kAudioLevelId, or something else > including > > "audio level". > > Now called kAudioLevelHeaderId > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:158: if > (!loudest_filter_.DecideForward(rtp_header.ssrc, audio_level)) { > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > I think the code will be more readable if you reverse the logic of this > > if-statement. That is: > > if (loudest_filter_.DecideForward(...)) { > > destination = GetReceiver... > > ... > > } > > Done. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:158: if > (!loudest_filter_.DecideForward(rtp_header.ssrc, audio_level)) { > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > Why parse out two components of the webrtc::RTPHeader struct and then pass > them > > to the same method? I think DecideForward should take a const > webrtc::RTPHeader > > reference as input. > > Done. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/conference_transport.cc:158: if > (!loudest_filter_.DecideForward(rtp_header.ssrc, audio_level)) { > On 2015/08/10 11:34:04, phoglund wrote: > > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > > Why parse out two components of the webrtc::RTPHeader struct and then pass > > them > > > to the same method? I think DecideForward should take a const > > webrtc::RTPHeader > > > reference as input. > > > > +1 > > Done. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > File webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc (right): > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:19: while (it != > stream_levels_.end()) { > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > I suggest you make this a for loop instead > > for (auto stream : stream_levels_) { > > ... > > But maybe you cannot call erase on the stream variable. > > > > If that doesn't work, you should still be able to use > > for (auto it = stream_levels_.begin(); it != c.end(); ) { > > ... > > > > There's an example at http://en.cppreference.com/w/cpp/container/map/erase. > > Note also how in that example they do not ++ the iterator when erase is > called, > > but rather update it with the output from the erase method. > > I do not think for (auto stream : stream_levels_) can work. > > The "while" vs. "for" in the example you provided is a tie. I agree that it is > better to use the C++11 std::map feature "it = erase(it)" > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:20: if > (rtc::TimeDiff(time_ms, it->second.last_time_ms_) > > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > Oh, is that why you kept using uint32_t for the time variables? You may > > disregard from my earlier comments about this. > > Acknowledged. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:22: > stream_levels_.erase(it++); > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > I'm not sure it is valid after you called erase. "References and iterators to > > the erased elements are invalidated." > > I saw many examples of this on the net. It should be valid. but better to use > C++11 feature of it = erase(it). I thought that I tried it and the compiler did > not work. But now it does, which is nice. I see, it failed on Mac. And in general, Chromium is no in favor of C++11 libraries. So I need to go back to my first implementation. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:34: > stream.second.audio_level_ > quietest_level) { > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > Comment about the fact that a quiet stream has a high audio level. > > Done. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/loudest_filter.cc:67: if (audio_level < > stream_levels_[quietest_ssrc].audio_level_) { > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > Again, explain that "up is down, and down is up". > > Done. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > File webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h (right): > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:21: /* DecideForward() > On 2015/08/10 11:34:04, phoglund wrote: > > On 2015/08/10 10:50:18, hlundin-webrtc wrote: > > > On 2015/08/10 09:30:22, minyue-webrtc wrote: > > > > On 2015/08/10 08:43:27, phoglund wrote: > > > > > I think this method name is hard to understand. I suppose you mean > "decide > > > > > whether to forward", but a filter doesn't forward things, but rather > > filters > > > > > stuff out. Maybe ShouldFilter() is a better name? > > > > > > > > Good point. May I suggest ShouldPass()? > > > > ShouldFilter() might be a bit ambiguous for being filtered out and being > > > passed > > > > through. > > > > > > I suggest ForwardThisPacket(). > > > > +1 > > I am a bit confused. Did we say "forward" isn't an operation of filter? > Combining the ideas, I tried to make a new name. Throw eggs if you don't like > it. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:22: * Decide whether > to filter out a stream. > On 2015/08/10 10:50:18, hlundin-webrtc wrote: > > "Decide whether to forward a packet." > > After all, this method is to be called for each packet, right? > > Acknowledged. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:33: audio_level_ = > audio_level; > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > Data members of structs don't have a trailing underscore. > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names > > Acknowledged. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:37: uint32 > last_time_ms_; > On 2015/08/10 10:50:18, hlundin-webrtc wrote: > > int > > Done. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:40: void > RemoveTimeoutStreams(uint32 time_ms); > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > int > > Done. > > https://codereview.chromium.org/1236793003/diff/80001/webrtc/voice_engine/tes... > webrtc/voice_engine/test/auto_test/fakes/loudest_filter.h:46: const int32 > kStreamTimeOutMs = 5000; > On 2015/08/10 10:50:17, hlundin-webrtc wrote: > > const int > > Done.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tina.legrand@webrtc.org, henrik.lundin@webrtc.org, phoglund@webrtc.org, andrew@webrtc.org Link to the patchset: https://codereview.webrtc.org/1236793003/#ps220001 (title: "avoiding C++11 map.erase signature")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236793003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1236793003/220001
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/03bb7c7bfac45e662e2159c111fb7f9bdb006d18 Cr-Commit-Position: refs/heads/master@{#9712} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
