Add input pool to ConvertingAudioFifo [chromium/src : main]
Johannes Kron (Gerrit)
Johannes Kron voted and added 1 comment![]( "Open in Gerrit")
Votes added by Johannes Kron
| Commit-Queue | +1 |
1 comment
Hey Thomas!
Can you review this change to ConvertingAudioFifo that I need to use this component in another [CL](https://chromium-review.git.corp.google.com/c/chromium/src/+/7673640)?
Thanks!
Related details
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 7 comments![]( "Open in Gerrit")
void PopInput();Tiniest of nits: do you mind renaming this to `PopInputs()`
// Max capacity needed to safely hold frames until min_input_frames_needed_NIT: wrap with backticks ```
(min_input_frames_needed_ + input_params_.frames_per_buffer() - 1) /It's not obvious to me where the `-1` comes from. Is this just to force a rounding down of the number of buffers?
This would be easier to understand if the calculation was broken down into an intermediary named variable (which the compiler will optimize away anyway).
inputs_.emplace_back(EnsureExpectedChannelCount(std::move(input_bus)));This behavior complicates things quite a bit when it comes to pool usage.
If we push a bus with a different channel count than `input_params_`, we will still be allocating busses constantly, which negates the point of the pool.
Also calling pushing a bus with a certain channel count and saving the mixed bus to the pool forces callers to check for a channel count match when calling `GetInputAudioBus()`, which is surprising.
I would suggest only accepting busses that match `input_params_.channels()` when using the pool. Alternatively, we could use 2 pools (the one you're adding and one more for the mixer), but I feel like this would complicate things... It depends how often you expect channel counts to change in your dependent CL.
return base::Seconds(static_cast<double>(total_frames_) /Use `AudioTimestampHelper::FramesToTime()`
EXPECT_NEAR(fifo()->GetBufferedInputDuration().InSecondsF(),
static_cast<double>(total_frames_pushed) / kInputSampleRate,Use `AudioTimestampHelper::FramesToTime` here as well.
EXPECT_EQ(pooled_bus->channels(), kDefaultParams.channels());
EXPECT_EQ(pooled_bus->frames(), kDefaultParams.frames_per_buffer());As mentioned in the .cc file, I find this behavior surprising, and I think it would be easiest to reason about if this wasn't allowed.
Related details
- Johannes Kron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Kron (Gerrit)
Johannes Kron voted and added 7 comments![]( "Open in Gerrit")
Votes added by Johannes Kron
| Commit-Queue | +1 |
7 comments
Tiniest of nits: do you mind renaming this to `PopInputs()`
Not at all. Done.
// Max capacity needed to safely hold frames until min_input_frames_needed_Johannes KronNIT: wrap with backticks ```
Done
(min_input_frames_needed_ + input_params_.frames_per_buffer() - 1) /It's not obvious to me where the `-1` comes from. Is this just to force a rounding down of the number of buffers?
This would be easier to understand if the calculation was broken down into an intermediary named variable (which the compiler will optimize away anyway).
I'll blame this one on Gemini—it insisted I optimize away the ceil method. Re-reading this a week later, it’s not obvious to me what’s happening at a glance, so I’m reverting to the more readable version.
inputs_.emplace_back(EnsureExpectedChannelCount(std::move(input_bus)));This behavior complicates things quite a bit when it comes to pool usage.
If we push a bus with a different channel count than `input_params_`, we will still be allocating busses constantly, which negates the point of the pool.
Also calling pushing a bus with a certain channel count and saving the mixed bus to the pool forces callers to check for a channel count match when calling `GetInputAudioBus()`, which is surprising.
I would suggest only accepting busses that match `input_params_.channels()` when using the pool. Alternatively, we could use 2 pools (the one you're adding and one more for the mixer), but I feel like this would complicate things... It depends how often you expect channel counts to change in your dependent CL.
I agree. Since there are no channel count changes in my dependent CL, I’ll restrict `EnsureExpectedChannelCount` to only accept buses matching `input_params_.channels()` if the pool is used. I'll add a `CHECK` to enforce this; since pooling is an explicit opt-in, a mismatch is a logic error that we should catch immediately. This keeps the pool efficient and `GetInputAudioBus()` predictable.
return base::Seconds(static_cast<double>(total_frames_) /Johannes KronUse `AudioTimestampHelper::FramesToTime()`
Done
EXPECT_NEAR(fifo()->GetBufferedInputDuration().InSecondsF(),
static_cast<double>(total_frames_pushed) / kInputSampleRate,Use `AudioTimestampHelper::FramesToTime` here as well.
Done
EXPECT_EQ(pooled_bus->channels(), kDefaultParams.channels());
EXPECT_EQ(pooled_bus->frames(), kDefaultParams.frames_per_buffer());As mentioned in the .cc file, I find this behavior surprising, and I think it would be easiest to reason about if this wasn't allowed.
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Guilbert
| Code-Review | +1 |
1 comment
Related details
- Johannes Kron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Kron (Gerrit)
Johannes Kron voted and added 1 comment![]( "Open in Gerrit")
Votes added by Johannes Kron
| Commit-Queue | +2 |
1 comment
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Add input pool to ConvertingAudioFifo
Introduces an optional input `AudioBusPoolImpl` to `ConvertingAudioFifo`
to reduce memory allocations on the audio processing hot path. Input
buses are safely recycled in `PopInput()` only if their frame counts
strictly match `input_params_.frames_per_buffer()`, avoiding DCHECKs
when handling dynamically sized buffers.
Additionally, adds `GetBufferedInputDuration()` to provide the total
duration of audio currently held in the FIFO.
- M media/base/converting_audio_fifo.cc
- M media/base/converting_audio_fifo.h
- M media/base/converting_audio_fifo_unittest.cc
Code-Review: +1 by Thomas Guilbert
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |