Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
420603004:
Use the AudioProcessing float interface in MediaStreamAudioProcessor. (Closed)
|
Created:
6 years, 5 months ago by ajm Modified:
6 years, 4 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, Henrik Grunell, no longer working on chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUse the AudioProcessing float interface in MediaStreamAudioProcessor.
Format conversions are now handled internally by AP, allowing us to
to tear out the conversion code from MSAP. Instead, we use a simple
FIFO to buffer the incoming audio into the 10 ms chunks AP requires.
Add an AudioBus wrapper to get an array of channel pointers, since this
is the type AP deals in.
Add a few TODOs around two suspect areas: i) there appears to be an
assumption on the sink type based on the input format, and ii) don't
we want ProcessAndConsumeData() to output float data?
BUG=400933
TESTED=Audio processing behaves as expected on a MacBook, using both
44.1 and 48 kHz as the render and capture rates (set through "Audio
Midi Setup"). Audio sounds fine when processing is disabled through
constraints.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289653
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Add render_format_ and remove FIFO accessors. #
Total comments: 3
Patch Set 3 : Add MediaStreamAudioBus. #
Total comments: 4
Patch Set 4 : Remove mutable. #
Total comments: 1
Patch Set 5 : Cap the output buffers at 10 ms (previous behavior). #Patch Set 6 : OnDataCallback can be called more than once. #
Total comments: 2
Messages
Total messages: 27 (0 generated)
Normally I'd get Henrik G and Shijing to review this, but since both are out, you're the lucky winner Tommi ;) Let me know if you don't have time, as Henrik G at least is back next week.
https://codereview.chromium.org/420603004/diff/50001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/420603004/diff/50001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:134: int channels() { return destination_->channels(); } make these const? actually - are they necessary? I'm wondering since they are public and there's no thread check or indication on what thread they must be used. https://codereview.chromium.org/420603004/diff/50001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:257: return input_format_; can we add a thread check to these methods? (or can we make input_format_, output_format_ const somehow?) https://codereview.chromium.org/420603004/diff/50001/media/base/audio_bus.h File media/base/audio_bus.h (right): https://codereview.chromium.org/420603004/diff/50001/media/base/audio_bus.h#n... media/base/audio_bus.h:96: float** channel_data() { return &channel_data_[0]; } I would like to get Dale to look at this (and perhaps some of the other changes if he can). I'm currently sheriffing so I have time occasionally but not continuously :) I'll take another round probably later today.
Dale, could you have a look at the AudioBus change? (and if you like, the rest of the CL) https://codereview.chromium.org/420603004/diff/50001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/420603004/diff/50001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:134: int channels() { return destination_->channels(); } On 2014/08/06 13:45:56, tommi wrote: > make these const? > > actually - are they necessary? I'm wondering since they are public and there's > no thread check or indication on what thread they must be used. These fulfill the same function as the previous source_parameters() and sink_parameters() methods, now used on line 473. They're not strictly necessary; the alternative is to add a render_format_ member to MSAP. At least that would be consistent with input_format_ and output_format_, so I made the change. https://codereview.chromium.org/420603004/diff/50001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:257: return input_format_; On 2014/08/06 13:45:56, tommi wrote: > can we add a thread check to these methods? (or can we make input_format_, > output_format_ const somehow?) Unfortunately we can't make them const, as they can change on-the-fly. I had a look at how they were used and documented the assumptions in the interface.
https://codereview.chromium.org/420603004/diff/70001/media/base/audio_bus.h File media/base/audio_bus.h (right): https://codereview.chromium.org/420603004/diff/70001/media/base/audio_bus.h#n... media/base/audio_bus.h:96: float** channel_data() { return &channel_data_[0]; } This isn't okay since the pointers could be rewritten using this interface. Only the const* interface is allowed, callers must use SetChannelData() for other cases. I also don't think you should add this to AudioBus for a single user, instead you should probably handle this with a wrapper inside the WebRTC class.
https://codereview.chromium.org/420603004/diff/70001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/420603004/diff/70001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:254: capture_thread_checker_.CalledOnValidThread()); Turns out this check failed, I assume because CalledOnValidThread() has side effects that weren't running due to the ||. Given the caller's guarantee (documented in OnCaptureChanged()), this is safe, but I'm not sure how to verify it with a thread checker. Any ideas? https://codereview.chromium.org/420603004/diff/70001/media/base/audio_bus.h File media/base/audio_bus.h (right): https://codereview.chromium.org/420603004/diff/70001/media/base/audio_bus.h#n... media/base/audio_bus.h:96: float** channel_data() { return &channel_data_[0]; } On 2014/08/07 20:05:06, DaleCurtis wrote: > This isn't okay since the pointers could be rewritten using this interface. > Only the const* interface is allowed, callers must use SetChannelData() for > other cases. Ah true, I should have returned float* const* which is all I needed (though now removed entirely). > > I also don't think you should add this to AudioBus for a single user, instead > you should probably handle this with a wrapper inside the WebRTC class. Good point. If you like, you can have a look at MediaStreamAudioBus to see if that's what you were thinking.
I defer to tommi@ now since this is all a bit fuzzy to me now, but here's some general commentary: As an aside, it's always hard to follow this code since it hops around so many threads :( Even with the thread checker's it be nice if all this could be split into classes which each did their own thing on one thread, but I understand the difficulty of that. https://codereview.chromium.org/420603004/diff/90001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/420603004/diff/90001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:116: // Mutable to allow updating before a const version is returned. I don't understand why this is necessary. Can you explain further? There's almost always a way to avoid mutable. https://codereview.chromium.org/420603004/diff/90001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:123: class MediaStreamAudioFifo { Long term you may consider switching to a block fifo like xians@ put together for platform interleaved input. The general idea is you push directly into an audiobus which you can hand out later. Instead of having to copy out of the fifo. I'm a little unclear on who's using what, but you might be able to consider just using a few DecoderBuffers here that you directly interleave into during Push() and return via Consume(), they can be recycled once HasOneRef() is true. This would replace the copy into and out of the FIFO plus the interleave step with a single interleave into the FIFO.
Tommi, PTAL. https://codereview.chromium.org/420603004/diff/90001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/420603004/diff/90001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:116: // Mutable to allow updating before a const version is returned. On 2014/08/11 20:53:13, DaleCurtis wrote: > I don't understand why this is necessary. Can you explain further? There's > almost always a way to avoid mutable. Yeah, I was definitely hesitant to add it. The trouble is we want to be able to return a const version of channel_ptrs that can be used with a const MediaStreamAudioBus. Conceptually it's really the underlying AudioBus that we want to be const; it doesn't matter that channel_ptrs_ gets modified. Since the scope of usage is pretty small though, it was easy to change ProcessData to take channel_ptrs rather than MediaStreamAudioFifos, obviating the need for these const methods. Now removed. https://codereview.chromium.org/420603004/diff/90001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:123: class MediaStreamAudioFifo { On 2014/08/11 20:53:13, DaleCurtis wrote: > Long term you may consider switching to a block fifo like xians@ put together > for platform interleaved input. The general idea is you push directly into an > audiobus which you can hand out later. Instead of having to copy out of the > fifo. > > I'm a little unclear on who's using what, but you might be able to consider just > using a few DecoderBuffers here that you directly interleave into during Push() > and return via Consume(), they can be recycled once HasOneRef() is true. This > would replace the copy into and out of the FIFO plus the interleave step with a > single interleave into the FIFO. Thanks for the tips. We still want deinterleaved audio at Consume() for processing. It's only at output from MediaStreamAudioProcessor that we interleave. I suppose that rules out the latter suggestion. The former is essentially implementing a special FIFO directly rather than depending on AudioFifo, correct? In a somewhat related matter, I'm not sure we should be outputting int16 interleaved at all from MSAP. (I left a TODO for this below). I think it was done originally because that was the only format webrtc::AudioProcessing could deal in. Some downstream consumers may still want float deinterleaved (WebAudio?) in which case we should delay the conversion until later. https://codereview.chromium.org/420603004/diff/130001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/420603004/diff/130001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:524: int MediaStreamAudioProcessor::ProcessData(const float* const* process_ptrs, Changed from input to process to better differentiate from input_format_ which represents the data at MSAP input, _before_ the FIFO.
Tommi, could you take a look?
The CQ bit was checked by tommi@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajm@chromium.org/420603004/130001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tommi@chromium.org
The CQ bit was unchecked by tommi@chromium.org
Thanks. It's a legitimate failure; I'm investigating.
On 2014/08/13 17:23:23, ajm wrote: > Thanks. It's a legitimate failure; I'm investigating. I had unintentionally removed the previous behavior of capping the output buffer size at 10 ms, even when audio processing is disabled. Now restored.
The CQ bit was checked by ajm@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajm@chromium.org/420603004/170001
Trying to commit again because I'm fairly certain I know what the problem is. (See comments). https://codereview.chromium.org/420603004/diff/210001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer_unittest.cc (right): https://codereview.chromium.org/420603004/diff/210001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer_unittest.cc:80: // Android works with a buffer size bigger than 20ms. Android using 20ms frames. https://codereview.chromium.org/420603004/diff/210001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer_unittest.cc:134: .Times(AtLeast(1)); The error is that this was getting called more than once on Android. I think that's as it should be actually and the previous behavior is suboptimal. This test on Android happens to be using 20ms frames (see above). Since MediaStreamAudioProcessor is designed to divide the input into 10ms chunks, we should actually be getting two 10ms callbacks on Android. I think this wasn't happening before due to buffering in the resampler. On the first pass, it won't provide as much output as it gets input, because it requires a bit of delay for the filter kernel. MSAP is now just using a simple FIFO and doesn't require the delay.
The CQ bit was checked by ajm@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajm@chromium.org/420603004/210001
The CQ bit was unchecked by ajm@chromium.org
The CQ bit was checked by ajm@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajm@chromium.org/420603004/210001
Message was sent while issue was closed.
Committed patchset #6 (210001) as 289653 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
