Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10820059:
Not streaming packets of silence. (Closed)
|
Created:
8 years, 4 months ago by kxing Modified:
8 years, 4 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNot streaming packets of silence.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149309
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments #
Total comments: 6
Patch Set 3 : Addressed comments #
Total comments: 2
Patch Set 4 : Fixed includes #
Total comments: 1
Messages
Total messages: 13 (0 generated)
Could someone PTAL?
http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.cc File remoting/host/audio_capturer.cc (right): http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.c... remoting/host/audio_capturer.cc:7: #include <math.h> abs() is in stdlib.h http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.c... remoting/host/audio_capturer.cc:36: if (std::abs(data[i]) > kSilenceThreshold) I think using memchr() to verify that the data is all zeroes would be good enough here. If there is really a need to filter low-volume noise then we probably need a fancier algorithm here. E.g. the buffer can contain the same value different from 0 for all samples - that's still silence. http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.c... remoting/host/audio_capturer.cc:36: if (std::abs(data[i]) > kSilenceThreshold) abs() without std:: http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.h File remoting/host/audio_capturer.h (right): http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.h... remoting/host/audio_capturer.h:36: static bool IsPacketOfSilence(const AudioPacket* packet); This shouldn't to be part of AudioCapturer interface. At very least this function should be marked as protected, as it is intended to be used by interface implementations. But it would be better to put it to a separate file audio_capturer_util.[h|cc] . IsValidSampleRate() should be moved there too.
Could someone PTAL again? http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.cc File remoting/host/audio_capturer.cc (right): http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.c... remoting/host/audio_capturer.cc:7: #include <math.h> On 2012/07/27 22:46:51, sergeyu wrote: > abs() is in stdlib.h Done. http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.c... remoting/host/audio_capturer.cc:36: if (std::abs(data[i]) > kSilenceThreshold) On 2012/07/27 22:46:51, sergeyu wrote: > abs() without std:: Done. http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.c... remoting/host/audio_capturer.cc:36: if (std::abs(data[i]) > kSilenceThreshold) > I think using memchr() to verify that the data is all zeroes would be good > enough here. Windows can give samples such as [0, 0, 0, 1, -1, 0, 0, 0, ..., 0, 1, 0, -1, 0] even when nothing is playing. I'm not sure if memchr would be the right solution for that. > If there is really a need to filter low-volume noise then we probably need a > fancier algorithm here. E.g. the buffer can contain the same value different > from 0 for all samples - that's still silence. For this CL, I'll stick to low volume noise centered around 0. Perhaps in the future, I'll turn it into an algorithm detecting low-volume noise centered around an arbitrary value. http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.h File remoting/host/audio_capturer.h (right): http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.h... remoting/host/audio_capturer.h:36: static bool IsPacketOfSilence(const AudioPacket* packet); On 2012/07/27 22:46:51, sergeyu wrote: > This shouldn't to be part of AudioCapturer interface. At very least this > function should be marked as protected, as it is intended to be used by > interface implementations. But it would be better to put it to a separate file > audio_capturer_util.[h|cc] . > > IsValidSampleRate() should be moved there too. Done.
http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.cc File remoting/host/audio_capturer.cc (right): http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.c... remoting/host/audio_capturer.cc:36: if (std::abs(data[i]) > kSilenceThreshold) On 2012/07/30 16:55:18, kxing wrote: > > I think using memchr() to verify that the data is all zeroes would be good > > enough here. > > Windows can give samples such as [0, 0, 0, 1, -1, 0, 0, 0, ..., 0, 1, 0, -1, 0] > even when nothing is playing. I'm not sure if memchr would be the right solution > for that. Hm. that is strange. Then it looks like this particular implementation should be windows-specific. I doubt that we will see the same behavior on other platforms. Move this function to audio_capturer_win.cc? and then you can revert changes for audio_capturer_util.cc. > > > If there is really a need to filter low-volume noise then we probably need a > > fancier algorithm here. E.g. the buffer can contain the same value different > > from 0 for all samples - that's still silence. > For this CL, I'll stick to low volume noise centered around 0. Perhaps in the > future, I'll turn it into an algorithm detecting low-volume noise centered > around an arbitrary value. http://codereview.chromium.org/10820059/diff/6001/remoting/host/audio_capture... File remoting/host/audio_capturer_util.h (right): http://codereview.chromium.org/10820059/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_util.h:14: bool IsValidSampleRate(int sample_rate); add some comments for both functions.
couple more comments http://codereview.chromium.org/10820059/diff/6001/remoting/host/audio_capture... File remoting/host/audio_capturer_util.cc (right): http://codereview.chromium.org/10820059/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_util.cc:14: const int kSilenceThreshold = 2; Please add some comments to explain how that value was chosen (once you move it to _win.cc) http://codereview.chromium.org/10820059/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_util.cc:30: const int16* data = reinterpret_cast<const int16*>(packet->data().data()); You assume here that the packet is always two bytes per sample. Add DCHECK(sizeof(int16), packet->bytes_per_sample); That way you also don't need kBytesPerSample const - you can just use sizeof(int16) instead.
Could someone PTAL again? http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.cc File remoting/host/audio_capturer.cc (right): http://codereview.chromium.org/10820059/diff/1/remoting/host/audio_capturer.c... remoting/host/audio_capturer.cc:36: if (std::abs(data[i]) > kSilenceThreshold) On 2012/07/30 17:53:48, sergeyu wrote: > On 2012/07/30 16:55:18, kxing wrote: > > > I think using memchr() to verify that the data is all zeroes would be good > > > enough here. > > > > Windows can give samples such as [0, 0, 0, 1, -1, 0, 0, 0, ..., 0, 1, 0, -1, > 0] > > even when nothing is playing. I'm not sure if memchr would be the right > solution > > for that. > > Hm. that is strange. Then it looks like this particular implementation should be > windows-specific. I doubt that we will see the same behavior on other platforms. > Move this function to audio_capturer_win.cc? and then you can revert changes for > audio_capturer_util.cc. > > > > > > If there is really a need to filter low-volume noise then we probably need a > > > fancier algorithm here. E.g. the buffer can contain the same value different > > > from 0 for all samples - that's still silence. > > For this CL, I'll stick to low volume noise centered around 0. Perhaps in the > > future, I'll turn it into an algorithm detecting low-volume noise centered > > around an arbitrary value. > Done. http://codereview.chromium.org/10820059/diff/6001/remoting/host/audio_capture... File remoting/host/audio_capturer_util.cc (right): http://codereview.chromium.org/10820059/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_util.cc:14: const int kSilenceThreshold = 2; On 2012/07/30 18:02:56, sergeyu wrote: > Please add some comments to explain how that value was chosen (once you move it > to _win.cc) Done. http://codereview.chromium.org/10820059/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_util.cc:30: const int16* data = reinterpret_cast<const int16*>(packet->data().data()); On 2012/07/30 18:02:56, sergeyu wrote: > You assume here that the packet is always two bytes per sample. Add > DCHECK(sizeof(int16), packet->bytes_per_sample); > That way you also don't need kBytesPerSample const - you can just use > sizeof(int16) instead. Done. http://codereview.chromium.org/10820059/diff/6001/remoting/host/audio_capture... File remoting/host/audio_capturer_util.h (right): http://codereview.chromium.org/10820059/diff/6001/remoting/host/audio_capture... remoting/host/audio_capturer_util.h:14: bool IsValidSampleRate(int sample_rate); On 2012/07/30 17:53:48, sergeyu wrote: > add some comments for both functions. Done.
lgtm. http://codereview.chromium.org/10820059/diff/4002/remoting/host/audio_capture... File remoting/host/audio_capturer.cc (right): http://codereview.chromium.org/10820059/diff/4002/remoting/host/audio_capture... remoting/host/audio_capturer.cc:7: #include <stdlib.h> nit: do we still need this header?
I'll commit this soon, unless someone objects. http://codereview.chromium.org/10820059/diff/4002/remoting/host/audio_capture... File remoting/host/audio_capturer.cc (right): http://codereview.chromium.org/10820059/diff/4002/remoting/host/audio_capture... remoting/host/audio_capturer.cc:7: #include <stdlib.h> On 2012/07/31 20:16:32, sergeyu wrote: > nit: do we still need this header? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kxing@chromium.org/10820059/9003
Change committed as 149309
Is there a reason that this is implemented directly in the Windows capturer, rather than as a filtering AudioCapturer, so that it can be re-used for other platforms? https://chromiumcodereview.appspot.com/10820059/diff/9003/remoting/host/audio... File remoting/host/audio_capturer_win.cc (right): https://chromiumcodereview.appspot.com/10820059/diff/9003/remoting/host/audio... remoting/host/audio_capturer_win.cc:35: // -1, even when no audio is playing. This doesn't sound right; a packet of frames all of the same value will be silent, regardless of the frames' absolute value.
For some reason windows returns -1/1 and zeroes when there is no sound being played. That behavior seems to be windows specific that's why this silence detection code is in platform-specific file. Yes we could have more complicated silence detection algorithm that would detect constant value being played, but its not needed right now. On other platforms we should be able to just compare to 0 to detect when there is nothing being played. Also note that if some app is playing silence that is different from 0 audio codec should be able to detect and compress it, so this case will be even less important after we implement compression. On 2012/08/07 18:01:37, Wez wrote: > Is there a reason that this is implemented directly in the Windows capturer, > rather than as a filtering AudioCapturer, so that it can be re-used for other > platforms? > > https://chromiumcodereview.appspot.com/10820059/diff/9003/remoting/host/audio... > File remoting/host/audio_capturer_win.cc (right): > > https://chromiumcodereview.appspot.com/10820059/diff/9003/remoting/host/audio... > remoting/host/audio_capturer_win.cc:35: // -1, even when no audio is playing. > This doesn't sound right; a packet of frames all of the same value will be > silent, regardless of the frames' absolute value.
On 2012/08/07 20:23:46, sergeyu wrote: > For some reason windows returns -1/1 and zeroes when there is no sound being > played. That behavior seems to be windows specific that's why this silence > detection code is in platform-specific file. Curious that silence isn't reliable zero on Windows; it's as though capturing from the output pin actually resamples the analogue output... > Yes we could have more complicated silence detection algorithm that would detect > constant value being played, but its not needed right now. On other platforms we > should be able to just compare to 0 to detect when there is nothing being > played. Perhaps, but that would be detected as silence by the code we've just added in the Windows capturer, so adding a test-for-zero special-case for other platforms makes little sense. Checking for a consistent non-zero value with minimal variance wouldn't be particularly complicated, given the constraints of the samplers we currently have. > Also note that if some app is playing silence that is different from 0 audio > codec should be able to detect and compress it, so this case will be even less > important after we implement compression. In which case we don't need silence suppression at all, so having in in-built to platform-specific capturers is still a bad idea. ;) We may reduce CPU overhead by pre-suppressing silence, though, depending on what level codecs implement that at. > On 2012/08/07 18:01:37, Wez wrote: > > Is there a reason that this is implemented directly in the Windows capturer, > > rather than as a filtering AudioCapturer, so that it can be re-used for other > > platforms? > > > > > https://chromiumcodereview.appspot.com/10820059/diff/9003/remoting/host/audio... > > File remoting/host/audio_capturer_win.cc (right): > > > > > https://chromiumcodereview.appspot.com/10820059/diff/9003/remoting/host/audio... > > remoting/host/audio_capturer_win.cc:35: // -1, even when no audio is playing. > > This doesn't sound right; a packet of frames all of the same value will be > > silent, regardless of the frames' absolute value. |
