media.log: re-implement NBLog using audio_utils_fifo
NBLog previously has its own shared memory circular buffer code.
Now NBLog operates on top of the circular buffer code in audio_utils.
Test: media.log still works
Change-Id: Ib3026d2a96e6c0b433603e8baf19164ad97a1e1f
diff --git a/include/media/nbaio/NBLog.h b/include/media/nbaio/NBLog.h
index 1297b51..acf2d31 100644
--- a/include/media/nbaio/NBLog.h
+++ b/include/media/nbaio/NBLog.h
@@ -21,7 +21,7 @@
#include <binder/IMemory.h>
#include <utils/Mutex.h>
-#include <audio_utils/roundup.h>
+#include <audio_utils/fifo.h>
namespace android {
@@ -55,8 +55,11 @@
private:
friend class Writer;
Event mEvent; // event type
- size_t mLength; // length of additional data, 0 <= mLength <= 255
+ uint8_t mLength; // length of additional data, 0 <= mLength <= kMaxLength
const void *mData; // event type-specific data
+ static const size_t kMaxLength = 255;
+public:
+ static const size_t kOverhead = 3; // mEvent, mLength, mData[...], duplicate mLength
};
// representation of a single log entry in shared memory
@@ -70,13 +73,17 @@
// byte[2+mLength] duplicate copy of mLength to permit reverse scan
// byte[3+mLength] start of next log entry
-// located in shared memory
+public:
+
+// Located in shared memory, must be POD.
+// Exactly one process must explicitly call the constructor or use placement new.
+// Since this is a POD, the destructor is empty and unnecessary to call it explicitly.
struct Shared {
- Shared() : mRear(0) { }
+ Shared() /* mRear initialized via default constructor */ { }
/*virtual*/ ~Shared() { }
- volatile int32_t mRear; // index one byte past the end of most recent Entry
- char mBuffer[0]; // circular buffer for entries
+ audio_utils_fifo_index mRear; // index one byte past the end of most recent Entry
+ char mBuffer[0]; // circular buffer for entries
};
public:
@@ -117,10 +124,10 @@
// Input parameter 'size' is the desired size of the timeline in byte units.
// The size of the shared memory must be at least Timeline::sharedSize(size).
- Writer(size_t size, void *shared);
- Writer(size_t size, const sp<IMemory>& iMemory);
+ Writer(void *shared, size_t size);
+ Writer(const sp<IMemory>& iMemory, size_t size);
- virtual ~Writer() { }
+ virtual ~Writer();
virtual void log(const char *string);
virtual void logf(const char *fmt, ...) __attribute__ ((format (printf, 2, 3)));
@@ -138,13 +145,16 @@
sp<IMemory> getIMemory() const { return mIMemory; }
private:
+ // 0 <= length <= kMaxLength
void log(Event event, const void *data, size_t length);
void log(const Entry *entry, bool trusted = false);
- const size_t mSize; // circular buffer size in bytes, must be a power of 2
Shared* const mShared; // raw pointer to shared memory
- const sp<IMemory> mIMemory; // ref-counted version
- int32_t mRear; // my private copy of mShared->mRear
+ sp<IMemory> mIMemory; // ref-counted version, initialized in constructor and then const
+ audio_utils_fifo * const mFifo; // FIFO itself,
+ // non-NULL unless constructor fails
+ audio_utils_fifo_writer * const mFifoWriter; // used to write to FIFO,
+ // non-NULL unless dummy constructor used
bool mEnabled; // whether to actually log
};
@@ -154,7 +164,7 @@
class LockedWriter : public Writer {
public:
LockedWriter();
- LockedWriter(size_t size, void *shared);
+ LockedWriter(void *shared, size_t size);
virtual void log(const char *string);
virtual void logf(const char *fmt, ...) __attribute__ ((format (printf, 2, 3)));
@@ -176,21 +186,24 @@
// Input parameter 'size' is the desired size of the timeline in byte units.
// The size of the shared memory must be at least Timeline::sharedSize(size).
- Reader(size_t size, const void *shared);
- Reader(size_t size, const sp<IMemory>& iMemory);
+ Reader(const void *shared, size_t size);
+ Reader(const sp<IMemory>& iMemory, size_t size);
- virtual ~Reader() { }
+ virtual ~Reader();
void dump(int fd, size_t indent = 0);
bool isIMemory(const sp<IMemory>& iMemory) const;
private:
- const size_t mSize; // circular buffer size in bytes, must be a power of 2
- const Shared* const mShared; // raw pointer to shared memory
- const sp<IMemory> mIMemory; // ref-counted version
- int32_t mFront; // index of oldest acknowledged Entry
+ /*const*/ Shared* const mShared; // raw pointer to shared memory, actually const but not
+ // declared as const because audio_utils_fifo() constructor
+ sp<IMemory> mIMemory; // ref-counted version, assigned only in constructor
int mFd; // file descriptor
int mIndent; // indentation level
+ audio_utils_fifo * const mFifo; // FIFO itself,
+ // non-NULL unless constructor fails
+ audio_utils_fifo_reader * const mFifoReader; // used to read from FIFO,
+ // non-NULL unless constructor fails
void dumpLine(const String8& timestamp, String8& body);
diff --git a/media/libnbaio/NBLog.cpp b/media/libnbaio/NBLog.cpp
index c728e3e..f019df5 100644
--- a/media/libnbaio/NBLog.cpp
+++ b/media/libnbaio/NBLog.cpp
@@ -23,7 +23,7 @@
#include <string.h>
#include <time.h>
#include <new>
-#include <cutils/atomic.h>
+#include <audio_utils/roundup.h>
#include <media/nbaio/NBLog.h>
#include <utils/Log.h>
#include <utils/String8.h>
@@ -74,19 +74,30 @@
// ---------------------------------------------------------------------------
NBLog::Writer::Writer()
- : mSize(0), mShared(NULL), mRear(0), mEnabled(false)
+ : mShared(NULL), mFifo(NULL), mFifoWriter(NULL), mEnabled(false)
{
}
-NBLog::Writer::Writer(size_t size, void *shared)
- : mSize(roundup(size)), mShared((Shared *) shared), mRear(0), mEnabled(mShared != NULL)
+NBLog::Writer::Writer(void *shared, size_t size)
+ : mShared((Shared *) shared),
+ mFifo(mShared != NULL ?
+ new audio_utils_fifo(size, sizeof(uint8_t),
+ mShared->mBuffer, mShared->mRear, NULL /*throttlesFront*/) : NULL),
+ mFifoWriter(mFifo != NULL ? new audio_utils_fifo_writer(*mFifo) : NULL),
+ mEnabled(mFifoWriter != NULL)
{
}
-NBLog::Writer::Writer(size_t size, const sp<IMemory>& iMemory)
- : mSize(roundup(size)), mShared(iMemory != 0 ? (Shared *) iMemory->pointer() : NULL),
- mIMemory(iMemory), mRear(0), mEnabled(mShared != NULL)
+NBLog::Writer::Writer(const sp<IMemory>& iMemory, size_t size)
+ : Writer(iMemory != 0 ? (Shared *) iMemory->pointer() : NULL, size)
{
+ mIMemory = iMemory;
+}
+
+NBLog::Writer::~Writer()
+{
+ delete mFifoWriter;
+ delete mFifo;
}
void NBLog::Writer::log(const char *string)
@@ -95,8 +106,8 @@
return;
}
size_t length = strlen(string);
- if (length > 255) {
- length = 255;
+ if (length > Entry::kMaxLength) {
+ length = Entry::kMaxLength;
}
log(EVENT_STRING, string, length);
}
@@ -117,7 +128,7 @@
if (!mEnabled) {
return;
}
- char buffer[256];
+ char buffer[Entry::kMaxLength + 1 /*NUL*/];
int length = vsnprintf(buffer, sizeof(buffer), fmt, ap);
if (length >= (int) sizeof(buffer)) {
length = sizeof(buffer) - 1;
@@ -153,7 +164,10 @@
if (!mEnabled) {
return;
}
- if (data == NULL || length > 255) {
+ if (data == NULL || length > Entry::kMaxLength) {
+ // TODO Perhaps it makes sense to display truncated data or at least a
+ // message that the data is too long? The current behavior can create
+ // a confusion for a programmer debugging their code.
return;
}
switch (event) {
@@ -177,26 +191,16 @@
log(entry->mEvent, entry->mData, entry->mLength);
return;
}
- size_t rear = mRear & (mSize - 1);
- size_t written = mSize - rear; // written = number of bytes that have been written so far
- size_t need = entry->mLength + 3; // mEvent, mLength, data[length], mLength
- // need = number of bytes remaining to write
- if (written > need) {
- written = need;
- }
- size_t i;
+ size_t need = entry->mLength + Entry::kOverhead; // mEvent, mLength, data[length], mLength
+ // need = number of bytes remaining to write
+
// FIXME optimize this using memcpy for the data part of the Entry.
// The Entry could have a method copyTo(ptr, offset, size) to optimize the copy.
- for (i = 0; i < written; ++i) {
- mShared->mBuffer[rear + i] = entry->readAt(i);
+ uint8_t temp[Entry::kMaxLength + Entry::kOverhead];
+ for (size_t i = 0; i < need; i++) {
+ temp[i] = entry->readAt(i);
}
- if (rear + written == mSize && (need -= written) > 0) {
- for (i = 0; i < need; ++i) {
- mShared->mBuffer[i] = entry->readAt(written + i);
- }
- written += need;
- }
- android_atomic_release_store(mRear += written, &mShared->mRear);
+ mFifoWriter->write(temp, need);
}
bool NBLog::Writer::isEnabled() const
@@ -218,8 +222,8 @@
{
}
-NBLog::LockedWriter::LockedWriter(size_t size, void *shared)
- : Writer(size, shared)
+NBLog::LockedWriter::LockedWriter(void *shared, size_t size)
+ : Writer(shared, size)
{
}
@@ -273,60 +277,59 @@
// ---------------------------------------------------------------------------
-NBLog::Reader::Reader(size_t size, const void *shared)
- : mSize(roundup(size)), mShared((const Shared *) shared), mFront(0)
+NBLog::Reader::Reader(const void *shared, size_t size)
+ : mShared((/*const*/ Shared *) shared), /*mIMemory*/
+ mFd(-1), mIndent(0),
+ mFifo(mShared != NULL ?
+ new audio_utils_fifo(size, sizeof(uint8_t),
+ mShared->mBuffer, mShared->mRear, NULL /*throttlesFront*/) : NULL),
+ mFifoReader(mFifo != NULL ? new audio_utils_fifo_reader(*mFifo) : NULL)
{
}
-NBLog::Reader::Reader(size_t size, const sp<IMemory>& iMemory)
- : mSize(roundup(size)), mShared(iMemory != 0 ? (const Shared *) iMemory->pointer() : NULL),
- mIMemory(iMemory), mFront(0)
+NBLog::Reader::Reader(const sp<IMemory>& iMemory, size_t size)
+ : Reader(iMemory != 0 ? (Shared *) iMemory->pointer() : NULL, size)
{
+ mIMemory = iMemory;
+}
+
+NBLog::Reader::~Reader()
+{
+ delete mFifoReader;
+ delete mFifo;
}
void NBLog::Reader::dump(int fd, size_t indent)
{
- int32_t rear = android_atomic_acquire_load(&mShared->mRear);
- size_t avail = rear - mFront;
- if (avail == 0) {
+ if (mFifoReader == NULL) {
return;
}
- size_t lost = 0;
- if (avail > mSize) {
- lost = avail - mSize;
- mFront += lost;
- avail = mSize;
- }
- size_t remaining = avail; // remaining = number of bytes left to read
- size_t front = mFront & (mSize - 1);
- size_t read = mSize - front; // read = number of bytes that have been read so far
- if (read > remaining) {
- read = remaining;
- }
// make a copy to avoid race condition with writer
- uint8_t *copy = new uint8_t[avail];
- // copy first part of circular buffer up until the wraparound point
- memcpy(copy, &mShared->mBuffer[front], read);
- if (front + read == mSize) {
- if ((remaining -= read) > 0) {
- // copy second part of circular buffer starting at beginning
- memcpy(©[read], mShared->mBuffer, remaining);
- read += remaining;
- // remaining = 0 but not necessary
- }
- }
- mFront += read;
+ size_t capacity = mFifo->capacity();
+
+ // TODO Stack-based allocation of large objects may fail.
+ // Currently the log buffers are a page or two, which should be safe.
+ // But if the log buffers ever get a lot larger,
+ // then change this to allocate from heap when necessary.
+ static size_t kReasonableStackObjectSize = 32768;
+ ALOGW_IF(capacity > kReasonableStackObjectSize, "Stack-based allocation of object may fail");
+ uint8_t copy[capacity];
+
+ size_t lost;
+ ssize_t actual = mFifoReader->read(copy, capacity, NULL /*timeout*/, &lost);
+ ALOG_ASSERT(actual <= capacity);
+ size_t avail = actual > 0 ? (size_t) actual : 0;
size_t i = avail;
Event event;
size_t length;
struct timespec ts;
time_t maxSec = -1;
- while (i >= 3) {
+ while (i >= Entry::kOverhead) {
length = copy[i - 1];
- if (length + 3 > i || copy[i - length - 2] != length) {
+ if (length + Entry::kOverhead > i || copy[i - length - 2] != length) {
break;
}
- event = (Event) copy[i - length - 3];
+ event = (Event) copy[i - length - Entry::kOverhead];
if (event == EVENT_TIMESTAMP) {
if (length != sizeof(struct timespec)) {
// corrupt
@@ -337,7 +340,7 @@
maxSec = ts.tv_sec;
}
}
- i -= length + 3;
+ i -= length + Entry::kOverhead;
}
mFd = fd;
mIndent = indent;
@@ -362,7 +365,7 @@
event = (Event) copy[i];
length = copy[i + 1];
const void *data = ©[i + 2];
- size_t advance = length + 3;
+ size_t advance = length + Entry::kOverhead;
switch (event) {
case EVENT_STRING:
body.appendFormat("%.*s", (int) length, (const char *) data);
@@ -376,7 +379,7 @@
long deltaTotal = 0;
size_t j = i;
for (;;) {
- j += sizeof(struct timespec) + 3;
+ j += sizeof(struct timespec) + 3 /*Entry::kOverhead?*/;
if (j >= avail || (Event) copy[j] != EVENT_TIMESTAMP) {
break;
}
@@ -398,7 +401,7 @@
deltaTotal += delta;
prevNsec = tsNext.tv_nsec;
}
- size_t n = (j - i) / (sizeof(struct timespec) + 3);
+ size_t n = (j - i) / (sizeof(struct timespec) + 3 /*Entry::kOverhead?*/);
if (deferredTimestamp) {
dumpLine(timestamp, body);
deferredTimestamp = false;
@@ -432,8 +435,6 @@
if (deferredTimestamp) {
dumpLine(timestamp, body);
}
- // FIXME it would be more efficient to put a char mCopy[256] as a member variable of the dumper
- delete[] copy;
}
void NBLog::Reader::dumpLine(const String8& timestamp, String8& body)
diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp
index d08309b..e4b73c6 100644
--- a/services/audioflinger/AudioFlinger.cpp
+++ b/services/audioflinger/AudioFlinger.cpp
@@ -512,8 +512,11 @@
return new NBLog::Writer();
}
success:
+ NBLog::Shared *sharedRawPtr = (NBLog::Shared *) shared->pointer();
+ new((void *) sharedRawPtr) NBLog::Shared(); // placement new here, but the corresponding
+ // explicit destructor not needed since it is POD
mediaLogService->registerWriter(shared, size, name);
- return new NBLog::Writer(size, shared);
+ return new NBLog::Writer(shared, size);
}
void AudioFlinger::unregisterWriter(const sp<NBLog::Writer>& writer)
diff --git a/services/audioflinger/FastThreadDumpState.cpp b/services/audioflinger/FastThreadDumpState.cpp
index 9df5c4c..964a725 100644
--- a/services/audioflinger/FastThreadDumpState.cpp
+++ b/services/audioflinger/FastThreadDumpState.cpp
@@ -14,6 +14,7 @@
* limitations under the License.
*/
+#include <audio_utils/roundup.h>
#include "FastThreadDumpState.h"
namespace android {
diff --git a/services/medialog/Android.mk b/services/medialog/Android.mk
index a1da63d..423b186 100644
--- a/services/medialog/Android.mk
+++ b/services/medialog/Android.mk
@@ -4,7 +4,7 @@
LOCAL_SRC_FILES := MediaLogService.cpp IMediaLogService.cpp
-LOCAL_SHARED_LIBRARIES := libbinder libutils liblog libnbaio
+LOCAL_SHARED_LIBRARIES := libbinder libutils liblog libnbaio libaudioutils
LOCAL_MULTILIB := $(AUDIOSERVER_MULTILIB)
diff --git a/services/medialog/MediaLogService.cpp b/services/medialog/MediaLogService.cpp
index f85aa13..ab2f925 100644
--- a/services/medialog/MediaLogService.cpp
+++ b/services/medialog/MediaLogService.cpp
@@ -35,7 +35,7 @@
shared->size() < NBLog::Timeline::sharedSize(size)) {
return;
}
- sp<NBLog::Reader> reader(new NBLog::Reader(size, shared));
+ sp<NBLog::Reader> reader(new NBLog::Reader(shared, size));
NamedReader namedReader(reader, name);
Mutex::Autolock _l(mLock);
mNamedReaders.add(namedReader);