Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1528473003:
Feed the WebRTC APM with empty far-end frames for the number of frames skipped by OS. (Closed)
|
Created:
5 years ago by Henrik Grunell Modified:
4 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFeed the WebRTC APM with empty far-end frames for the number of frames skipped by OS.
*** This CL is replaced by https://codereview.chromium.org/1596523005/ ***
BUG=560371
Patch Set 1 #
Total comments: 5
Patch Set 2 : Code review. Rebase. #
Depends on Patchset: Messages
Total messages: 22 (3 generated)
Description was changed from ========== Feed the WebRTC APM with empty far-end frames for the number of frames skipped by OS. BUG=560371 ========== to ========== Feed the WebRTC APM with empty far-end frames for the number of frames skipped by OS. BUG=560371 ==========
grunell@chromium.org changed reviewers: + peah@chromium.org, tommi@chromium.org
Before I update the unit tests, I'd like a first round of review, specifically usage of APM. Tommi: all Per: review usage of APM (content/renderer/media/media_stream_audio_processor.cc)
On 2015/12/14 18:44:07, Henrik Grunell wrote: > Before I update the unit tests, I'd like a first round of review, specifically > usage of APM. > > Tommi: all > Per: review usage of APM > (content/renderer/media/media_stream_audio_processor.cc) One thing I was thinking about when writing this CL is whether the same thing happens on the input side. Perhaps frames are always skipped in sync on input and output. Then this CL will make things worse. I need to verify how it behaves. Tommi, did you examine the input side when looking at the output?
On 2015/12/14 18:47:13, Henrik Grunell wrote: > On 2015/12/14 18:44:07, Henrik Grunell wrote: > > Before I update the unit tests, I'd like a first round of review, specifically > > usage of APM. > > > > Tommi: all > > Per: review usage of APM > > (content/renderer/media/media_stream_audio_processor.cc) > > One thing I was thinking about when writing this CL is whether the same thing > happens on the input side. Perhaps frames are always skipped in sync on input > and output. Then this CL will make things worse. I need to verify how it > behaves. Tommi, did you examine the input side when looking at the output? I haven't but the same timestamp information is available there: https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/mac/au... Sounds like a good thing to investigate.
https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor.cc:454: MediaStreamAudioBus empty_bus(audio_bus->channels(), skipped_frames); Does this call also set all samples in the empty_bus to zero? https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor.cc:457: empty_bus.bus()->frames(), Is there any restriction on the values of skipped_frames? AnalyzeReverseStream only allows multiples of 10 ms of samples to be received.
https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor.cc:454: MediaStreamAudioBus empty_bus(audio_bus->channels(), skipped_frames); On 2015/12/16 10:26:57, peah wrote: > Does this call also set all samples in the empty_bus to zero? I doesn't. Fixed. https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor.cc:457: empty_bus.bus()->frames(), On 2015/12/16 10:26:57, peah wrote: > Is there any restriction on the values of skipped_frames? AnalyzeReverseStream > only allows multiples of 10 ms of samples to be received. No, it can be any value. OK, I added storing the number of skipped frames and feeding empty 10 ms chunks when reaching that.
On 2015/12/18 10:17:33, Henrik Grunell wrote: > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > File content/renderer/media/media_stream_audio_processor.cc (right): > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > content/renderer/media/media_stream_audio_processor.cc:454: MediaStreamAudioBus > empty_bus(audio_bus->channels(), skipped_frames); > On 2015/12/16 10:26:57, peah wrote: > > Does this call also set all samples in the empty_bus to zero? > > I doesn't. Fixed. > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > content/renderer/media/media_stream_audio_processor.cc:457: > empty_bus.bus()->frames(), > On 2015/12/16 10:26:57, peah wrote: > > Is there any restriction on the values of skipped_frames? AnalyzeReverseStream > > only allows multiples of 10 ms of samples to be received. > > No, it can be any value. OK, I added storing the number of skipped frames and > feeding empty 10 ms chunks when reaching that. Hmm, wait a minute. A multiple of 10 ms you say? Then I don't have to loop and put 10 ms each call?
On 2015/12/18 10:18:56, Henrik Grunell wrote: > On 2015/12/18 10:17:33, Henrik Grunell wrote: > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > File content/renderer/media/media_stream_audio_processor.cc (right): > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > content/renderer/media/media_stream_audio_processor.cc:454: > MediaStreamAudioBus > > empty_bus(audio_bus->channels(), skipped_frames); > > On 2015/12/16 10:26:57, peah wrote: > > > Does this call also set all samples in the empty_bus to zero? > > > > I doesn't. Fixed. > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > content/renderer/media/media_stream_audio_processor.cc:457: > > empty_bus.bus()->frames(), > > On 2015/12/16 10:26:57, peah wrote: > > > Is there any restriction on the values of skipped_frames? > AnalyzeReverseStream > > > only allows multiples of 10 ms of samples to be received. > > > > No, it can be any value. OK, I added storing the number of skipped frames and > > feeding empty 10 ms chunks when reaching that. > > Hmm, wait a minute. A multiple of 10 ms you say? Then I don't have to loop and > put 10 ms each call? How much work would it be to update the AEC to support being notified of skipped frames? When I've seen this happen, it is usually in a period of high CPU load. So it makes sense to do as little as possible and it feels like we're doing the opposite by allocating extra empty buffers and inserting them into a queue of buffers. Can't we just skip?
On 2015/12/18 10:22:38, tommi wrote: > On 2015/12/18 10:18:56, Henrik Grunell wrote: > > On 2015/12/18 10:17:33, Henrik Grunell wrote: > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > File content/renderer/media/media_stream_audio_processor.cc (right): > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > content/renderer/media/media_stream_audio_processor.cc:454: > > MediaStreamAudioBus > > > empty_bus(audio_bus->channels(), skipped_frames); > > > On 2015/12/16 10:26:57, peah wrote: > > > > Does this call also set all samples in the empty_bus to zero? > > > > > > I doesn't. Fixed. > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > content/renderer/media/media_stream_audio_processor.cc:457: > > > empty_bus.bus()->frames(), > > > On 2015/12/16 10:26:57, peah wrote: > > > > Is there any restriction on the values of skipped_frames? > > AnalyzeReverseStream > > > > only allows multiples of 10 ms of samples to be received. > > > > > > No, it can be any value. OK, I added storing the number of skipped frames > and > > > feeding empty 10 ms chunks when reaching that. > > > > Hmm, wait a minute. A multiple of 10 ms you say? Then I don't have to loop and > > put 10 ms each call? > > How much work would it be to update the AEC to support being notified of skipped > frames? > When I've seen this happen, it is usually in a period of high CPU load. So it > makes sense to do as little as possible and it feels like we're doing the > opposite by allocating extra empty buffers and inserting them into a queue of > buffers. Can't we just skip? Yeah, Per and I talked about this before. Agree that it's unfortunate to add this extra allocation. Per, did you file a bug for supporting this in APM?
On 2015/12/18 10:18:56, Henrik Grunell wrote: > On 2015/12/18 10:17:33, Henrik Grunell wrote: > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > File content/renderer/media/media_stream_audio_processor.cc (right): > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > content/renderer/media/media_stream_audio_processor.cc:454: > MediaStreamAudioBus > > empty_bus(audio_bus->channels(), skipped_frames); > > On 2015/12/16 10:26:57, peah wrote: > > > Does this call also set all samples in the empty_bus to zero? > > > > I doesn't. Fixed. > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > content/renderer/media/media_stream_audio_processor.cc:457: > > empty_bus.bus()->frames(), > > On 2015/12/16 10:26:57, peah wrote: > > > Is there any restriction on the values of skipped_frames? > AnalyzeReverseStream > > > only allows multiples of 10 ms of samples to be received. > > > > No, it can be any value. OK, I added storing the number of skipped frames and > > feeding empty 10 ms chunks when reaching that. > > Hmm, wait a minute. A multiple of 10 ms you say? Then I don't have to loop and > put 10 ms each call? No, for sure not, it is better to put everything at once. Note, however, that if we are not sure that there are exact multiples of 10 ms, this approach may not be the best to use (pls see other comments).
https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor.cc:457: empty_bus.bus()->frames(), On 2015/12/18 10:17:33, Henrik Grunell wrote: > On 2015/12/16 10:26:57, peah wrote: > > Is there any restriction on the values of skipped_frames? AnalyzeReverseStream > > only allows multiples of 10 ms of samples to be received. > > No, it can be any value. OK, I added storing the number of skipped frames and > feeding empty 10 ms chunks when reaching that. What does this mean? Does it mean that typically any number of samples can be skipped? In that case, feeding the AEC zero frames will not work, and may actually even make things worse. If we are to compensate for the delay change, that compensation needs to be done before the AEC starts seeing the delay change in the captured microphone signal as when that happens, the AEC starts to readjust to the delay change. Therefore, if the skipped samples are not sent to the AEC as a zero frame until at least a full frame has been skipped, the extra zero frame passed to the AEC will likely arrive after the AEC has started to readjusting and will therefore cause even further readjustment to occur. Compared to that it may be better not to do anything.
On 2015/12/18 13:34:12, peah wrote: > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > File content/renderer/media/media_stream_audio_processor.cc (right): > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > content/renderer/media/media_stream_audio_processor.cc:457: > empty_bus.bus()->frames(), > On 2015/12/18 10:17:33, Henrik Grunell wrote: > > On 2015/12/16 10:26:57, peah wrote: > > > Is there any restriction on the values of skipped_frames? > AnalyzeReverseStream > > > only allows multiples of 10 ms of samples to be received. > > > > No, it can be any value. OK, I added storing the number of skipped frames and > > feeding empty 10 ms chunks when reaching that. > > What does this mean? Does it mean that typically any number of samples can be > skipped? > > In that case, feeding the AEC zero frames will not work, and may actually even > make things worse. > > If we are to compensate for the delay change, that compensation needs to be done > before the AEC starts seeing the delay change in the captured microphone signal > as when that happens, the AEC starts to readjust to the delay change. > > Therefore, if the skipped samples are not sent to the AEC as a zero frame until > at least a full frame has been skipped, the extra zero frame passed to the AEC > will likely arrive after the AEC has started to readjusting and will therefore > cause even further readjustment to occur. Compared to that it may be better not > to do anything. The underlying capture might be capturing at e.g. 256 frames per callback (regardless of frequency), so that data might be accumulating in a fifo before being delivered into webrtc in chunks of 10ms.
On 2015/12/18 10:22:38, tommi wrote: > On 2015/12/18 10:18:56, Henrik Grunell wrote: > > On 2015/12/18 10:17:33, Henrik Grunell wrote: > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > File content/renderer/media/media_stream_audio_processor.cc (right): > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > content/renderer/media/media_stream_audio_processor.cc:454: > > MediaStreamAudioBus > > > empty_bus(audio_bus->channels(), skipped_frames); > > > On 2015/12/16 10:26:57, peah wrote: > > > > Does this call also set all samples in the empty_bus to zero? > > > > > > I doesn't. Fixed. > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > content/renderer/media/media_stream_audio_processor.cc:457: > > > empty_bus.bus()->frames(), > > > On 2015/12/16 10:26:57, peah wrote: > > > > Is there any restriction on the values of skipped_frames? > > AnalyzeReverseStream > > > > only allows multiples of 10 ms of samples to be received. > > > > > > No, it can be any value. OK, I added storing the number of skipped frames > and > > > feeding empty 10 ms chunks when reaching that. > > > > Hmm, wait a minute. A multiple of 10 ms you say? Then I don't have to loop and > > put 10 ms each call? > > How much work would it be to update the AEC to support being notified of skipped > frames? > When I've seen this happen, it is usually in a period of high CPU load. So it > makes sense to do as little as possible and it feels like we're doing the > opposite by allocating extra empty buffers and inserting them into a queue of > buffers. Can't we just skip? As far as I have understood it the following holds (please correct me if I'm wrong). 1) There is no way of avoiding that skipped frames may occur, right? 2) The duration of the skipped frames may be any, and cannot be assumed to be fixed, right? 3) After a number of frames have been skipped, we always know exactly how many frames have been skipped, right? If 2) holds, the we cannot solve this by passing zero frames to the AEC. If 3) holds, the best solution would instead be to open up an APM API that reports the number of skipped frames to the APM. I think we should not increase the APM API if we do not need to, but if 1-3) holds the only feasible way of solving this for the AEC is to do that, so that we definitely should in that case. Does this make sense?
On 2015/12/18 13:39:07, tommi wrote: > On 2015/12/18 13:34:12, peah wrote: > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > File content/renderer/media/media_stream_audio_processor.cc (right): > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > content/renderer/media/media_stream_audio_processor.cc:457: > > empty_bus.bus()->frames(), > > On 2015/12/18 10:17:33, Henrik Grunell wrote: > > > On 2015/12/16 10:26:57, peah wrote: > > > > Is there any restriction on the values of skipped_frames? > > AnalyzeReverseStream > > > > only allows multiples of 10 ms of samples to be received. > > > > > > No, it can be any value. OK, I added storing the number of skipped frames > and > > > feeding empty 10 ms chunks when reaching that. > > > > What does this mean? Does it mean that typically any number of samples can be > > skipped? > > > > In that case, feeding the AEC zero frames will not work, and may actually even > > make things worse. > > > > If we are to compensate for the delay change, that compensation needs to be > done > > before the AEC starts seeing the delay change in the captured microphone > signal > > as when that happens, the AEC starts to readjust to the delay change. > > > > Therefore, if the skipped samples are not sent to the AEC as a zero frame > until > > at least a full frame has been skipped, the extra zero frame passed to the AEC > > will likely arrive after the AEC has started to readjusting and will therefore > > cause even further readjustment to occur. Compared to that it may be better > not > > to do anything. > > The underlying capture might be capturing at e.g. 256 frames per callback > (regardless of frequency), so that data might be accumulating in a fifo before > being delivered into webrtc in chunks of 10ms. But this is on the playout side, right? Or does that mean that frames are sent at a rate of 256 at a time for playout as well, meaning that they will only be skipped in chunks of lengths 256?
On 2015/12/18 13:39:42, peah wrote: > On 2015/12/18 10:22:38, tommi wrote: > > On 2015/12/18 10:18:56, Henrik Grunell wrote: > > > On 2015/12/18 10:17:33, Henrik Grunell wrote: > > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > > File content/renderer/media/media_stream_audio_processor.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > > content/renderer/media/media_stream_audio_processor.cc:454: > > > MediaStreamAudioBus > > > > empty_bus(audio_bus->channels(), skipped_frames); > > > > On 2015/12/16 10:26:57, peah wrote: > > > > > Does this call also set all samples in the empty_bus to zero? > > > > > > > > I doesn't. Fixed. > > > > > > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > > content/renderer/media/media_stream_audio_processor.cc:457: > > > > empty_bus.bus()->frames(), > > > > On 2015/12/16 10:26:57, peah wrote: > > > > > Is there any restriction on the values of skipped_frames? > > > AnalyzeReverseStream > > > > > only allows multiples of 10 ms of samples to be received. > > > > > > > > No, it can be any value. OK, I added storing the number of skipped frames > > and > > > > feeding empty 10 ms chunks when reaching that. > > > > > > Hmm, wait a minute. A multiple of 10 ms you say? Then I don't have to loop > and > > > put 10 ms each call? > > > > How much work would it be to update the AEC to support being notified of > skipped > > frames? > > When I've seen this happen, it is usually in a period of high CPU load. So it > > makes sense to do as little as possible and it feels like we're doing the > > opposite by allocating extra empty buffers and inserting them into a queue of > > buffers. Can't we just skip? > > As far as I have understood it the following holds (please correct me if I'm > wrong). > 1) There is no way of avoiding that skipped frames may occur, right? > 2) The duration of the skipped frames may be any, and cannot be assumed to be > fixed, right? > 3) After a number of frames have been skipped, we always know exactly how many > frames have been skipped, right? > > If 2) holds, the we cannot solve this by passing zero frames to the AEC. > If 3) holds, the best solution would instead be to open up an APM API that > reports the number of skipped frames to the APM. > > I think we should not increase the APM API if we do not need to, but if 1-3) > holds the only feasible way of solving this for the AEC is to do that, so that > we definitely should in that case. > > Does this make sense? Makes sense to me and I think this *might* warrant a new API. Ideally one that allows us to feed in information for both capture and rendering. An alternative to a new API, could be to pass in a "timestamp" for each buffer? I.e. monotonically increasing number representing the serial number of the first sample. I guess a problem with that approach would arise when frames are skipped "inside" a buffer. Maybe there's a solution to that.
On 2015/12/18 13:39:42, peah wrote: > On 2015/12/18 10:22:38, tommi wrote: > > On 2015/12/18 10:18:56, Henrik Grunell wrote: > > > On 2015/12/18 10:17:33, Henrik Grunell wrote: > > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > > File content/renderer/media/media_stream_audio_processor.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > > content/renderer/media/media_stream_audio_processor.cc:454: > > > MediaStreamAudioBus > > > > empty_bus(audio_bus->channels(), skipped_frames); > > > > On 2015/12/16 10:26:57, peah wrote: > > > > > Does this call also set all samples in the empty_bus to zero? > > > > > > > > I doesn't. Fixed. > > > > > > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > > content/renderer/media/media_stream_audio_processor.cc:457: > > > > empty_bus.bus()->frames(), > > > > On 2015/12/16 10:26:57, peah wrote: > > > > > Is there any restriction on the values of skipped_frames? > > > AnalyzeReverseStream > > > > > only allows multiples of 10 ms of samples to be received. > > > > > > > > No, it can be any value. OK, I added storing the number of skipped frames > > and > > > > feeding empty 10 ms chunks when reaching that. > > > > > > Hmm, wait a minute. A multiple of 10 ms you say? Then I don't have to loop > and > > > put 10 ms each call? > > > > How much work would it be to update the AEC to support being notified of > skipped > > frames? > > When I've seen this happen, it is usually in a period of high CPU load. So it > > makes sense to do as little as possible and it feels like we're doing the > > opposite by allocating extra empty buffers and inserting them into a queue of > > buffers. Can't we just skip? > > As far as I have understood it the following holds (please correct me if I'm > wrong). > 1) There is no way of avoiding that skipped frames may occur, right? Correct. > 2) The duration of the skipped frames may be any, and cannot be assumed to be > fixed, right? Well, it's fixed to a multiple of the currently used buffer size, but the buffer size is likely not 10 ms. So, correct. > 3) After a number of frames have been skipped, we always know exactly how many > frames have been skipped, right? Correct. > > If 2) holds, the we cannot solve this by passing zero frames to the AEC. > If 3) holds, the best solution would instead be to open up an APM API that > reports the number of skipped frames to the APM. > > I think we should not increase the APM API if we do not need to, but if 1-3) > holds the only feasible way of solving this for the AEC is to do that, so that > we definitely should in that case. > > Does this make sense? Yes, that would be best.
On 2015/12/18 13:45:01, tommi wrote: > On 2015/12/18 13:39:42, peah wrote: > > On 2015/12/18 10:22:38, tommi wrote: > > > On 2015/12/18 10:18:56, Henrik Grunell wrote: > > > > On 2015/12/18 10:17:33, Henrik Grunell wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > > > File content/renderer/media/media_stream_audio_processor.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > > > content/renderer/media/media_stream_audio_processor.cc:454: > > > > MediaStreamAudioBus > > > > > empty_bus(audio_bus->channels(), skipped_frames); > > > > > On 2015/12/16 10:26:57, peah wrote: > > > > > > Does this call also set all samples in the empty_bus to zero? > > > > > > > > > > I doesn't. Fixed. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > > > content/renderer/media/media_stream_audio_processor.cc:457: > > > > > empty_bus.bus()->frames(), > > > > > On 2015/12/16 10:26:57, peah wrote: > > > > > > Is there any restriction on the values of skipped_frames? > > > > AnalyzeReverseStream > > > > > > only allows multiples of 10 ms of samples to be received. > > > > > > > > > > No, it can be any value. OK, I added storing the number of skipped > frames > > > and > > > > > feeding empty 10 ms chunks when reaching that. > > > > > > > > Hmm, wait a minute. A multiple of 10 ms you say? Then I don't have to loop > > and > > > > put 10 ms each call? > > > > > > How much work would it be to update the AEC to support being notified of > > skipped > > > frames? > > > When I've seen this happen, it is usually in a period of high CPU load. So > it > > > makes sense to do as little as possible and it feels like we're doing the > > > opposite by allocating extra empty buffers and inserting them into a queue > of > > > buffers. Can't we just skip? > > > > As far as I have understood it the following holds (please correct me if I'm > > wrong). > > 1) There is no way of avoiding that skipped frames may occur, right? > > 2) The duration of the skipped frames may be any, and cannot be assumed to be > > fixed, right? > > 3) After a number of frames have been skipped, we always know exactly how many > > frames have been skipped, right? > > > > If 2) holds, the we cannot solve this by passing zero frames to the AEC. > > If 3) holds, the best solution would instead be to open up an APM API that > > reports the number of skipped frames to the APM. > > > > I think we should not increase the APM API if we do not need to, but if 1-3) > > holds the only feasible way of solving this for the AEC is to do that, so that > > we definitely should in that case. > > > > Does this make sense? > > Makes sense to me and I think this *might* warrant a new API. Ideally one that > allows us to feed in information for both capture and rendering. > > An alternative to a new API, could be to pass in a "timestamp" for each buffer? > I.e. monotonically increasing number representing the serial number of the first > sample. I guess a problem with that approach would arise when frames are > skipped "inside" a buffer. Maybe there's a solution to that. This is problematic due to the fifo on Mac. Maybe this was what you meant.
On 2015/12/18 13:45:01, tommi wrote: > On 2015/12/18 13:39:42, peah wrote: > > On 2015/12/18 10:22:38, tommi wrote: > > > On 2015/12/18 10:18:56, Henrik Grunell wrote: > > > > On 2015/12/18 10:17:33, Henrik Grunell wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > > > File content/renderer/media/media_stream_audio_processor.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > > > content/renderer/media/media_stream_audio_processor.cc:454: > > > > MediaStreamAudioBus > > > > > empty_bus(audio_bus->channels(), skipped_frames); > > > > > On 2015/12/16 10:26:57, peah wrote: > > > > > > Does this call also set all samples in the empty_bus to zero? > > > > > > > > > > I doesn't. Fixed. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > > > > content/renderer/media/media_stream_audio_processor.cc:457: > > > > > empty_bus.bus()->frames(), > > > > > On 2015/12/16 10:26:57, peah wrote: > > > > > > Is there any restriction on the values of skipped_frames? > > > > AnalyzeReverseStream > > > > > > only allows multiples of 10 ms of samples to be received. > > > > > > > > > > No, it can be any value. OK, I added storing the number of skipped > frames > > > and > > > > > feeding empty 10 ms chunks when reaching that. > > > > > > > > Hmm, wait a minute. A multiple of 10 ms you say? Then I don't have to loop > > and > > > > put 10 ms each call? > > > > > > How much work would it be to update the AEC to support being notified of > > skipped > > > frames? > > > When I've seen this happen, it is usually in a period of high CPU load. So > it > > > makes sense to do as little as possible and it feels like we're doing the > > > opposite by allocating extra empty buffers and inserting them into a queue > of > > > buffers. Can't we just skip? > > > > As far as I have understood it the following holds (please correct me if I'm > > wrong). > > 1) There is no way of avoiding that skipped frames may occur, right? > > 2) The duration of the skipped frames may be any, and cannot be assumed to be > > fixed, right? > > 3) After a number of frames have been skipped, we always know exactly how many > > frames have been skipped, right? > > > > If 2) holds, the we cannot solve this by passing zero frames to the AEC. > > If 3) holds, the best solution would instead be to open up an APM API that > > reports the number of skipped frames to the APM. > > > > I think we should not increase the APM API if we do not need to, but if 1-3) > > holds the only feasible way of solving this for the AEC is to do that, so that > > we definitely should in that case. > > > > Does this make sense? > > Makes sense to me and I think this *might* warrant a new API. Ideally one that > allows us to feed in information for both capture and rendering. > > An alternative to a new API, could be to pass in a "timestamp" for each buffer? > I.e. monotonically increasing number representing the serial number of the first > sample. I guess a problem with that approach would arise when frames are > skipped "inside" a buffer. Maybe there's a solution to that. Yes, I'll set up an issue for creating a new API method for this. That is definitely an option, but it would affect the current API as well, as we would then need to change the ProcessStream and ProcessReverseStream APIs too. Furthermore, we'd only be able to leverage that if we know the exact timestamp of what frames it was that was changed. And I guess we'd have to be able to deal with issues such as faulty timestamps, etc, right? I think the fastest way forward is to open up an API that receives the exact number of samples that have been skipped. And I think the AEC will be able to perform sufficiently well for that case if this does not happen very often.
On 2015/12/18 13:34:12, peah wrote: > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > File content/renderer/media/media_stream_audio_processor.cc (right): > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > content/renderer/media/media_stream_audio_processor.cc:457: > empty_bus.bus()->frames(), > On 2015/12/18 10:17:33, Henrik Grunell wrote: > > On 2015/12/16 10:26:57, peah wrote: > > > Is there any restriction on the values of skipped_frames? > AnalyzeReverseStream > > > only allows multiples of 10 ms of samples to be received. > > > > No, it can be any value. OK, I added storing the number of skipped frames and > > feeding empty 10 ms chunks when reaching that. > > What does this mean? Does it mean that typically any number of samples can be > skipped? > > In that case, feeding the AEC zero frames will not work, and may actually even > make things worse. > > If we are to compensate for the delay change, that compensation needs to be done > before the AEC starts seeing the delay change in the captured microphone signal > as when that happens, the AEC starts to readjust to the delay change. > > Therefore, if the skipped samples are not sent to the AEC as a zero frame until > at least a full frame has been skipped, the extra zero frame passed to the AEC > will likely arrive after the AEC has started to readjusting and will therefore > cause even further readjustment to occur. Compared to that it may be better not > to do anything. Yeah, we talked about this offline before. I was under the impression informing about this later than would be fine but that was apparently under the condition it was before the near-end data arrived.
On 2015/12/18 13:53:51, Henrik Grunell wrote: > On 2015/12/18 13:34:12, peah wrote: > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > File content/renderer/media/media_stream_audio_processor.cc (right): > > > > > https://codereview.chromium.org/1528473003/diff/1/content/renderer/media/medi... > > content/renderer/media/media_stream_audio_processor.cc:457: > > empty_bus.bus()->frames(), > > On 2015/12/18 10:17:33, Henrik Grunell wrote: > > > On 2015/12/16 10:26:57, peah wrote: > > > > Is there any restriction on the values of skipped_frames? > > AnalyzeReverseStream > > > > only allows multiples of 10 ms of samples to be received. > > > > > > No, it can be any value. OK, I added storing the number of skipped frames > and > > > feeding empty 10 ms chunks when reaching that. > > > > What does this mean? Does it mean that typically any number of samples can be > > skipped? > > > > In that case, feeding the AEC zero frames will not work, and may actually even > > make things worse. > > > > If we are to compensate for the delay change, that compensation needs to be > done > > before the AEC starts seeing the delay change in the captured microphone > signal > > as when that happens, the AEC starts to readjust to the delay change. > > > > Therefore, if the skipped samples are not sent to the AEC as a zero frame > until > > at least a full frame has been skipped, the extra zero frame passed to the AEC > > will likely arrive after the AEC has started to readjusting and will therefore > > cause even further readjustment to occur. Compared to that it may be better > not > > to do anything. > > Yeah, we talked about this offline before. I was under the impression informing > about this later than would be fine but that was apparently under the condition > it was before the near-end data arrived. True, and also that the skipped frames happen in chunks of 10 ms length. Lets go for a new API method instead. That will simplify things for all the code.
Description was changed from ========== Feed the WebRTC APM with empty far-end frames for the number of frames skipped by OS. BUG=560371 ========== to ========== Feed the WebRTC APM with empty far-end frames for the number of frames skipped by OS. *** This CL is replaced by https://codereview.chromium.org/1596523005/ *** BUG=560371 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
