0

[CodeHealth] Spanify AudioFifo

This CL remove AudioFifo's use of "float* AudioBus::channel()" and
memcopy, in favor of the safer base::span alternatives.

This CL also adds a missing UT, per an ancient TODO.

Bug: 373960632
Change-Id: I99113b34bbefe1e4bd441b581bf184f7eacbf93d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6154858
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Auto-Submit: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1407289}
This commit is contained in:
Thomas Guilbert
2025-01-16 06:50:07 -08:00
committed by Chromium LUCI CQ
parent 96a598feba
commit 91180a6196
7 changed files with 158 additions and 119 deletions

@ -84,7 +84,7 @@ bool FlacAudioHandler::CopyTo(AudioBus* bus, size_t* frames_written) {
DCHECK(is_initialized());
if (AtEnd()) {
DCHECK_EQ(fifo_->frames(), 0);
DCHECK_EQ(fifo_->frames(), 0u);
bus->Zero();
return true;
}
@ -93,7 +93,8 @@ bool FlacAudioHandler::CopyTo(AudioBus* bus, size_t* frames_written) {
DCHECK_EQ(bus->channels(), num_channels_);
// Records the number of frames copied into `bus`.
int frames_copied = 0;
size_t frames_copied = 0;
size_t bus_size = static_cast<size_t>(bus->frames());
do {
if (fifo_->frames() == 0 && !AtEnd()) {
@ -103,13 +104,12 @@ bool FlacAudioHandler::CopyTo(AudioBus* bus, size_t* frames_written) {
}
}
if (fifo_->frames() > 0) {
const int frames =
std::min(bus->frames() - frames_copied, fifo_->frames());
if (fifo_->frames() > 0u) {
const size_t frames = std::min(bus_size - frames_copied, fifo_->frames());
fifo_->Consume(bus, frames_copied, frames);
frames_copied += frames;
}
} while (!AtEnd() && frames_copied < bus->frames());
} while (!AtEnd() && frames_copied < bus_size);
*frames_written = frames_copied;
return true;

@ -2,17 +2,14 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifdef UNSAFE_BUFFERS_BUILD
// TODO(crbug.com/40285824): Remove this and convert code to safer constructs.
#pragma allow_unsafe_buffers
#endif
#include "media/base/audio_fifo.h"
#include <cstring>
#include "base/check_op.h"
#include "base/numerics/safe_math.h"
#include "base/trace_event/trace_event.h"
#include "base/types/zip.h"
namespace media {
@ -25,32 +22,33 @@ namespace media {
// where we have not yet reached the "edge" of the FIFO. If |pos| + |in_size|
// exceeds the total size of the FIFO, we must wrap around and start reusing
// a part the allocated memory. The size of this part is given by |wrap_size|.
static void GetSizes(
int pos, int max_size, int in_size, int* size, int* wrap_size) {
if (pos + in_size > max_size) {
static void GetSizes(size_t pos,
size_t max_size,
size_t in_size,
size_t* size,
size_t* wrap_size) {
const size_t frames_before_end = base::CheckSub(max_size, pos).ValueOrDie();
if (in_size > frames_before_end) {
// Wrapping is required => derive size of each segment.
*size = max_size - pos;
*wrap_size = in_size - *size;
} else {
// Wrapping is not required.
*size = in_size;
*wrap_size = 0;
*wrap_size = 0u;
}
}
// Updates the read/write position with |step| modulo the maximum number of
// elements in the FIFO to ensure that the position counters wraps around at
// the endpoint.
static int UpdatePos(int pos, int step, int max_size) {
static size_t UpdatePos(size_t pos, size_t step, int max_size) {
return ((pos + step) % max_size);
}
AudioFifo::AudioFifo(int channels, int frames)
: audio_bus_(AudioBus::Create(channels, frames)),
max_frames_(frames),
frames_(0),
read_pos_(0),
write_pos_(0) {}
max_frames_(base::checked_cast<size_t>(frames)) {}
AudioFifo::~AudioFifo() = default;
@ -64,35 +62,40 @@ void AudioFifo::Push(const AudioBus* source, int source_size) {
DCHECK_LE(source_size, source->frames());
// Ensure that there is space for the new data in the FIFO.
CHECK_LE(source_size + frames_, max_frames_);
const size_t source_frames = base::checked_cast<size_t>(source_size);
const size_t remaining_frames =
base::CheckSub(max_frames_, frames_).ValueOrDie();
CHECK_LE(source_frames, remaining_frames);
TRACE_EVENT_BEGIN2(TRACE_DISABLED_BY_DEFAULT("audio"), "AudioFifo::Push",
"this", static_cast<void*>(this), "fifo frames", frames_);
// Figure out if wrapping is needed and if so what segment sizes we need
// when adding the new audio bus content to the FIFO.
int append_size = 0;
int wrap_size = 0;
GetSizes(write_pos_, max_frames_, source_size, &append_size, &wrap_size);
size_t append_size = 0u;
size_t wrap_size = 0u;
GetSizes(write_pos_, max_frames_, source_frames, &append_size, &wrap_size);
// Copy all channels from the source to the FIFO. Wrap around if needed.
for (int ch = 0; ch < source->channels(); ++ch) {
float* dest = audio_bus_->channel(ch);
const float* src = source->channel(ch);
for (auto [data_src, fifo_dest] :
base::zip(source->AllChannels(), audio_bus_->AllChannels())) {
auto [append_data, wrap_data] = data_src.split_at(append_size);
// Append part of (or the complete) source to the FIFO.
memcpy(&dest[write_pos_], &src[0], append_size * sizeof(src[0]));
fifo_dest.subspan(write_pos_, append_size)
.copy_from_nonoverlapping(append_data);
if (wrap_size > 0) {
// Wrapping is needed: copy remaining part from the source to the FIFO.
memcpy(&dest[0], &src[append_size], wrap_size * sizeof(src[0]));
fifo_dest.first(wrap_size).copy_from_nonoverlapping(wrap_data);
}
}
frames_ += source_size;
frames_ += source_frames;
DCHECK_LE(frames_, max_frames_);
write_pos_ = UpdatePos(write_pos_, source_size, max_frames_);
write_pos_ = UpdatePos(write_pos_, source_frames, max_frames_);
TRACE_EVENT_END1(TRACE_DISABLED_BY_DEFAULT("audio"), "AudioFifo::Push",
"frames", source_size);
"frames", source_frames);
}
void AudioFifo::Consume(AudioBus* destination,
@ -101,48 +104,53 @@ void AudioFifo::Consume(AudioBus* destination,
DCHECK(destination);
DCHECK_EQ(destination->channels(), audio_bus_->channels());
const size_t dest_offset = base::checked_cast<size_t>(start_frame);
const size_t frame_count = base::checked_cast<size_t>(frames_to_consume);
// It is not possible to ask for more data than what is available in the FIFO.
CHECK_LE(frames_to_consume, frames_);
CHECK_LE(frame_count, frames_);
// A copy from the FIFO to |destination| will only be performed if the
// allocated memory in |destination| is sufficient.
CHECK_LE(frames_to_consume + start_frame, destination->frames());
const size_t free_frames_in_dest =
base::CheckSub(destination->frames(), dest_offset).ValueOrDie();
CHECK_LE(frame_count, free_frames_in_dest);
TRACE_EVENT_BEGIN2(TRACE_DISABLED_BY_DEFAULT("audio"), "AudioFifo::Consume",
"this", static_cast<void*>(this), "fifo frames", frames_);
// Figure out if wrapping is needed and if so what segment sizes we need
// when removing audio bus content from the FIFO.
int consume_size = 0;
int wrap_size = 0;
GetSizes(read_pos_, max_frames_, frames_to_consume, &consume_size,
&wrap_size);
size_t consume_size = 0u;
size_t wrap_size = 0u;
GetSizes(read_pos_, max_frames_, frame_count, &consume_size, &wrap_size);
// For all channels, remove the requested amount of data from the FIFO
// and copy the content to the destination. Wrap around if needed.
for (int ch = 0; ch < destination->channels(); ++ch) {
float* dest = destination->channel(ch);
const float* src = audio_bus_->channel(ch);
for (auto [data_dest, fifo_src] :
base::zip(destination->AllChannels(), audio_bus_->AllChannels())) {
auto [consume_data, wrap_data] =
data_dest.subspan(dest_offset, frame_count).split_at(consume_size);
// Copy a selected part of the FIFO to the destination.
memcpy(&dest[start_frame], &src[read_pos_], consume_size * sizeof(src[0]));
consume_data.copy_from_nonoverlapping(
fifo_src.subspan(read_pos_, consume_size));
if (wrap_size > 0) {
// Wrapping is needed: copy remaining part to the destination.
memcpy(&dest[consume_size + start_frame], &src[0],
wrap_size * sizeof(src[0]));
wrap_data.copy_from_nonoverlapping(fifo_src.first(wrap_size));
}
}
frames_ -= frames_to_consume;
read_pos_ = UpdatePos(read_pos_, frames_to_consume, max_frames_);
frames_ -= frame_count;
read_pos_ = UpdatePos(read_pos_, frame_count, max_frames_);
TRACE_EVENT_END1(TRACE_DISABLED_BY_DEFAULT("audio"), "AudioFifo::Consume",
"frames", frames_to_consume);
"frames", frame_count);
}
void AudioFifo::Clear() {
frames_ = 0;
read_pos_ = 0;
write_pos_ = 0;
frames_ = 0u;
read_pos_ = 0u;
write_pos_ = 0u;
}
} // namespace media

@ -46,9 +46,9 @@ class MEDIA_EXPORT AudioFifo {
void Clear();
// Number of actual audio frames in the FIFO.
int frames() const { return frames_; }
size_t frames() const { return frames_; }
int max_frames() const { return max_frames_; }
size_t max_frames() const { return max_frames_; }
private:
// The actual FIFO is an audio bus implemented as a ring buffer.
@ -56,16 +56,16 @@ class MEDIA_EXPORT AudioFifo {
// Maximum number of elements the FIFO can contain.
// This value is set by |frames| in the constructor.
const int max_frames_;
const size_t max_frames_;
// Number of actual elements in the FIFO.
int frames_;
size_t frames_ = 0u;
// Current read position.
int read_pos_;
size_t read_pos_ = 0u;
// Current write position.
int write_pos_;
size_t write_pos_ = 0u;
};
} // namespace media

@ -2,16 +2,12 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifdef UNSAFE_BUFFERS_BUILD
// TODO(crbug.com/40285824): Remove this and convert code to safer constructs.
#pragma allow_unsafe_buffers
#endif
// TODO(henrika): add test which included |start_frame| in Consume() call.
#include "media/base/audio_fifo.h"
#include <algorithm>
#include <memory>
#include "media/base/audio_fifo.h"
#include "base/containers/span.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace media {
@ -25,42 +21,42 @@ class AudioFifoTest : public testing::Test {
~AudioFifoTest() override = default;
void VerifyValue(const float data[], int size, float value) {
for (int i = 0; i < size; ++i)
void VerifyValue(base::span<float> data, float value) {
for (size_t i = 0; i < data.size(); ++i) {
ASSERT_FLOAT_EQ(value, data[i]) << "i=" << i;
}
}
protected:
};
// Verify that construction works as intended.
TEST_F(AudioFifoTest, Construct) {
static const int kChannels = 6;
static const int kMaxFrameCount = 128;
static constexpr int kChannels = 6;
static constexpr size_t kMaxFrameCount = 128;
AudioFifo fifo(kChannels, kMaxFrameCount);
EXPECT_EQ(fifo.frames(), 0);
EXPECT_EQ(fifo.frames(), 0u);
}
// Pushes audio bus objects to a FIFO and fill it up to different degrees.
TEST_F(AudioFifoTest, Push) {
static const int kChannels = 2;
static const int kMaxFrameCount = 128;
static constexpr int kChannels = 2;
static constexpr size_t kMaxFrameCount = 128;
AudioFifo fifo(kChannels, kMaxFrameCount);
{
SCOPED_TRACE("Push 50%");
static constexpr size_t kHalfFrameCount = kMaxFrameCount / 2;
std::unique_ptr<AudioBus> bus =
AudioBus::Create(kChannels, kMaxFrameCount / 2);
EXPECT_EQ(fifo.frames(), 0);
AudioBus::Create(kChannels, kHalfFrameCount);
EXPECT_EQ(fifo.frames(), 0u);
fifo.Push(bus.get());
EXPECT_EQ(fifo.frames(), bus->frames());
EXPECT_EQ(fifo.frames(), kHalfFrameCount);
fifo.Clear();
}
{
SCOPED_TRACE("Push 100%");
std::unique_ptr<AudioBus> bus = AudioBus::Create(kChannels, kMaxFrameCount);
EXPECT_EQ(fifo.frames(), 0);
EXPECT_EQ(fifo.frames(), 0u);
fifo.Push(bus.get());
EXPECT_EQ(fifo.frames(), bus->frames());
EXPECT_EQ(fifo.frames(), static_cast<size_t>(bus->frames()));
fifo.Clear();
}
}
@ -68,7 +64,7 @@ TEST_F(AudioFifoTest, Push) {
// Consumes audio bus objects from a FIFO and empty it to different degrees.
TEST_F(AudioFifoTest, Consume) {
static const int kChannels = 2;
static const int kMaxFrameCount = 128;
static const size_t kMaxFrameCount = 128;
AudioFifo fifo(kChannels, kMaxFrameCount);
{
std::unique_ptr<AudioBus> bus = AudioBus::Create(kChannels, kMaxFrameCount);
@ -77,10 +73,11 @@ TEST_F(AudioFifoTest, Consume) {
}
{
SCOPED_TRACE("Consume 50%");
static constexpr size_t kHalfFrameCount = kMaxFrameCount / 2;
std::unique_ptr<AudioBus> bus =
AudioBus::Create(kChannels, kMaxFrameCount / 2);
AudioBus::Create(kChannels, kHalfFrameCount);
fifo.Consume(bus.get(), 0, bus->frames());
EXPECT_TRUE(fifo.frames() == bus->frames());
EXPECT_EQ(fifo.frames(), kHalfFrameCount);
fifo.Push(bus.get());
EXPECT_EQ(fifo.frames(), kMaxFrameCount);
}
@ -88,23 +85,51 @@ TEST_F(AudioFifoTest, Consume) {
SCOPED_TRACE("Consume 100%");
std::unique_ptr<AudioBus> bus = AudioBus::Create(kChannels, kMaxFrameCount);
fifo.Consume(bus.get(), 0, bus->frames());
EXPECT_EQ(fifo.frames(), 0);
EXPECT_EQ(fifo.frames(), 0u);
fifo.Push(bus.get());
EXPECT_EQ(fifo.frames(), kMaxFrameCount);
}
}
TEST_F(AudioFifoTest, ConsumeWithStartFrame) {
static constexpr int kChannels = 2;
static constexpr size_t kMaxFrameCount = 128;
AudioFifo fifo(kChannels, kMaxFrameCount);
std::unique_ptr<AudioBus> bus = AudioBus::Create(kChannels, kMaxFrameCount);
// Fill the fifo with `kTestValue`.
static constexpr float kTestValue = 0.5f;
for (auto channel : bus->AllChannels()) {
std::ranges::fill(channel, kTestValue);
}
fifo.Push(bus.get());
static constexpr size_t kOffset = 10;
static constexpr size_t kCount = 5;
// Fill `output_bus` with an offset.
bus->Zero();
fifo.Consume(bus.get(), kOffset, kCount);
// `kTestValue` should only be present at `kOffset`, for `kCount` values.
for (auto channel : bus->AllChannels()) {
VerifyValue(channel.first(kOffset), 0);
VerifyValue(channel.subspan(kOffset, kCount), kTestValue);
VerifyValue(channel.subspan(kOffset + kCount), 0);
}
}
// Verify that the frames() method of the FIFO works as intended while
// appending and removing audio bus elements to/from the FIFO.
TEST_F(AudioFifoTest, FramesInFifo) {
static const int kChannels = 2;
static const int kMaxFrameCount = 64;
static constexpr int kChannels = 2;
static constexpr size_t kMaxFrameCount = 64;
AudioFifo fifo(kChannels, kMaxFrameCount);
// Fill up the FIFO and verify that the size grows as it should while adding
// one audio frame each time.
std::unique_ptr<AudioBus> bus = AudioBus::Create(kChannels, 1);
int n = 0;
size_t n = 0;
while (fifo.frames() < kMaxFrameCount) {
fifo.Push(bus.get());
EXPECT_EQ(fifo.frames(), ++n);
@ -113,22 +138,22 @@ TEST_F(AudioFifoTest, FramesInFifo) {
// Empty the FIFO and verify that the size decreases as it should.
// Reduce the size of the FIFO by one frame each time.
while (fifo.frames() > 0) {
while (fifo.frames() > 0u) {
fifo.Consume(bus.get(), 0, bus->frames());
EXPECT_EQ(fifo.frames(), --n);
}
EXPECT_EQ(fifo.frames(), 0);
EXPECT_EQ(fifo.frames(), 0u);
// Verify that a steady-state size of #frames in the FIFO is maintained
// during a sequence of Push/Consume calls which involves wrapping. We ensure
// wrapping by selecting a buffer size which does divides the FIFO size
// with a remainder of one.
// during a sequence of Push/Consume calls which involves wrapping. We
// ensure wrapping by selecting a buffer size which does divides the FIFO
// size with a remainder of one.
std::unique_ptr<AudioBus> bus2 =
AudioBus::Create(kChannels, (kMaxFrameCount / 4) - 1);
const int frames_in_fifo = bus2->frames();
const size_t frames_in_fifo = static_cast<size_t>(bus2->frames());
fifo.Push(bus2.get());
EXPECT_EQ(fifo.frames(), frames_in_fifo);
for (n = 0; n < kMaxFrameCount; ++n) {
for (n = 0u; n < kMaxFrameCount; ++n) {
fifo.Push(bus2.get());
fifo.Consume(bus2.get(), 0, frames_in_fifo);
EXPECT_EQ(fifo.frames(), frames_in_fifo);
@ -139,24 +164,23 @@ TEST_F(AudioFifoTest, FramesInFifo) {
// to the FIFO is correctly retrieved, i.e., that the order is correct and the
// values are correct.
TEST_F(AudioFifoTest, VerifyDataValues) {
static const int kChannels = 2;
static const int kFrameCount = 2;
static const int kFifoFrameCount = 5 * kFrameCount;
static constexpr int kChannels = 2;
static constexpr size_t kFrameCount = 2;
static constexpr size_t kFifoFrameCount = 5 * kFrameCount;
AudioFifo fifo(kChannels, kFifoFrameCount);
std::unique_ptr<AudioBus> bus = AudioBus::Create(kChannels, kFrameCount);
EXPECT_EQ(fifo.frames(), 0);
EXPECT_EQ(bus->frames(), kFrameCount);
// Start by filling up the FIFO with audio frames. The first audio frame
// will contain all 1's, the second all 2's etc. All channels contain the
// same value.
int value = 1;
while (fifo.frames() < kFifoFrameCount) {
for (int j = 0; j < bus->channels(); ++j)
std::fill(bus->channel(j), bus->channel(j) + bus->frames(), value);
for (auto channel : bus->AllChannels()) {
std::ranges::fill(channel, value);
}
fifo.Push(bus.get());
EXPECT_EQ(fifo.frames(), bus->frames() * value);
EXPECT_EQ(fifo.frames(), static_cast<size_t>(bus->frames() * value));
++value;
}
@ -169,36 +193,39 @@ TEST_F(AudioFifoTest, VerifyDataValues) {
// It means that we shall read out the same value two times in row.
value = 1;
int n = 1;
const int frames_to_consume = bus->frames() / 2;
const size_t frames_to_consume = bus->frames() / 2;
while (fifo.frames() > 0) {
fifo.Consume(bus.get(), 0, frames_to_consume);
for (int j = 0; j < bus->channels(); ++j)
VerifyValue(bus->channel(j), frames_to_consume, value);
if (n++ % 2 == 0)
for (auto channel : bus->AllChannels()) {
VerifyValue(channel.first(frames_to_consume), value);
}
if (n++ % 2 == 0) {
++value; // counts 1, 1, 2, 2, 3, 3,...
}
}
// FIFO should be empty now.
EXPECT_EQ(fifo.frames(), 0);
EXPECT_EQ(fifo.frames(), 0u);
// Push one audio bus to the FIFO and fill it with 1's.
value = 1;
for (int j = 0; j < bus->channels(); ++j)
std::fill(bus->channel(j), bus->channel(j) + bus->frames(), value);
for (auto channel : bus->AllChannels()) {
std::ranges::fill(channel, value);
}
fifo.Push(bus.get());
EXPECT_EQ(fifo.frames(), bus->frames());
EXPECT_EQ(fifo.frames(), static_cast<size_t>(bus->frames()));
// Keep calling Consume/Push a few rounds and verify that we read out the
// correct values. The number of elements shall be fixed (kFrameCount) during
// this phase.
for (int i = 0; i < 5 * kFifoFrameCount; i++) {
// correct values. The number of elements shall be fixed (kFrameCount)
// during this phase.
for (size_t i = 0; i < 5 * kFifoFrameCount; i++) {
fifo.Consume(bus.get(), 0, bus->frames());
for (int j = 0; j < bus->channels(); ++j) {
VerifyValue(bus->channel(j), bus->channels(), value);
std::fill(bus->channel(j), bus->channel(j) + bus->frames(), value + 1);
for (auto channel : bus->AllChannels()) {
VerifyValue(channel, value);
std::ranges::fill(channel, value + 1);
}
fifo.Push(bus.get());
EXPECT_EQ(fifo.frames(), bus->frames());
EXPECT_EQ(fifo.frames(), static_cast<size_t>(bus->frames()));
++value;
}
}

@ -178,7 +178,8 @@ class AudioProcessorCaptureFifo {
}
if (fifo_) {
CHECK_LT(fifo_->frames(), destination_->bus()->frames());
CHECK_LT(fifo_->frames(),
static_cast<size_t>(destination_->bus()->frames()));
next_audio_delay_ =
audio_delay + fifo_->frames() * base::Seconds(1) / sample_rate_;
fifo_->Push(source_to_push);
@ -195,8 +196,10 @@ class AudioProcessorCaptureFifo {
bool Consume(AudioProcessorCaptureBus** destination,
base::TimeDelta* audio_delay) {
if (fifo_) {
if (fifo_->frames() < destination_->bus()->frames())
if (fifo_->frames() <
static_cast<size_t>(destination_->bus()->frames())) {
return false;
}
fifo_->Consume(destination_->bus(), 0, destination_->bus()->frames());
*audio_delay = next_audio_delay_;

@ -187,8 +187,9 @@ void AudioTrackOpusEncoder::EncodeAudio(
// Wait to have enough |input_bus|s to guarantee a satisfactory conversion,
// accounting for multiple calls to ProvideInput().
while (fifo_->frames() >= converter_->GetMaxInputFramesRequested(
kOpusPreferredFramesPerBuffer)) {
while (fifo_->frames() >=
static_cast<size_t>(converter_->GetMaxInputFramesRequested(
kOpusPreferredFramesPerBuffer))) {
std::unique_ptr<media::AudioBus> audio_bus = media::AudioBus::Create(
converted_params_.channels(), kOpusPreferredFramesPerBuffer);
converter_->Convert(audio_bus.get());

@ -218,7 +218,7 @@ double WebAudioMediaStreamAudioSink::ProvideInput(
media::AudioTimestampHelper::FramesToTime(
frames_delayed + fifo_->frames(), source_params_.sample_rate())
.InMillisecondsF());
if (fifo_->frames() >= audio_bus->frames()) {
if (fifo_->frames() >= static_cast<size_t>(audio_bus->frames())) {
fifo_->Consume(audio_bus, 0, audio_bus->frames());
TRACE_COUNTER_ID1(TRACE_DISABLED_BY_DEFAULT("mediastream"),
"WebAudioMediaStreamAudioSink fifo space", this,