Minor cleanup of comments
Test: dumpsys media.log
Change-Id: I144908ce81ec10f572c79b75fda569683b7682f5
diff --git a/media/libnbaio/NBLog.cpp b/media/libnbaio/NBLog.cpp
index c73632c..ad38390 100644
--- a/media/libnbaio/NBLog.cpp
+++ b/media/libnbaio/NBLog.cpp
@@ -12,78 +12,10 @@
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
+ *
+ *
*/
-/*
-* Documentation: Workflow summary for histogram data processing:
-* For more details on FIFO, please see system/media/audio_utils; doxygen
-* TODO: add this documentation to doxygen once it is further developed
-* 1) Writing buffer period timestamp to the circular buffer
-* onWork()
-* Called every period length (e.g., 4ms)
-* Calls LOG_HIST_TS
-* LOG_HIST_TS
-* Hashes file name and line number, and writes single timestamp to buffer
-* calls NBLOG::Writer::logEventHistTS once
-* NBLOG::Writer::logEventHistTS
-* calls NBLOG::Writer::log on hash and current timestamp
-* time is in CLOCK_MONOTONIC converted to ns
-* NBLOG::Writer::log(Event, const void*, size_t)
-* Initializes Entry, a struct containing one log entry
-* Entry contains the event type (mEvent), data length (mLength),
-* and data pointer (mData)
-* TODO: why mLength (max length of buffer data) must be <= kMaxLength = 255?
-* calls NBLOG::Writer::log(Entry *, bool)
-* NBLog::Writer::log(Entry *, bool)
-* Calls copyEntryDataAt to format data as follows in temp array:
-* [type][length][data ... ][length]
-* calls audio_utils_fifo_writer.write on temp
-* audio_utils_fifo_writer.write
-* calls obtain(), memcpy (reference in doxygen)
-* returns number of frames written
-* ssize_t audio_utils_fifo_reader::obtain
-* Determines readable buffer section via pointer arithmetic on reader
-* and writer pointers
-* Similarly, LOG_AUDIO_STATE() is called by onStateChange whenever audio is
-* turned on or off, and writes this notification to the FIFO.
-*
-* 2) reading the data from shared memory
-* Thread::threadloop()
-* NBLog::MergeThread::threadLoop()
-* Waits on a mutex, called periodically
-* Calls NBLog::Merger::merge and MergeReader.getAndProcessSnapshot.
-* NBLog::Merger::merge
-* Merges snapshots sorted by timestamp
-* Calls Reader::getSnapshot on each individual thread buffer to in shared
-* memory and writes all their data to the single FIFO stored in mMerger.
-* NBLog::Reader::getSnapshot
-* copies snapshot of reader's fifo buffer into its own buffer
-* calls mFifoReader->obtain to find readable data
-* sets snapshot.begin() and .end() iterators to boundaries of valid entries
-* moves the fifo reader index to after the last entry read
-* in this case, the buffer is in shared memory. in (4), the buffer is private
-* NBLog::MergeThread::getAndProcessSnapshot
-* Iterates through the entries in the local FIFO. Processes the data in
-* specific ways depending on the entry type. If the data is a histogram
-* timestamp or an audio on/off signal, writes to a map of PerformanceAnalysis
-* class instances, where the wakeup() intervals are stored as histograms
-* and analyzed.
-*
-* 3) Dumpsys media.log call to report the data
-* MediaLogService::dump in MediaLogService.cpp
-* calls NBLog::Reader::dump, which calls ReportPerformance::dump
-* ReportPerformance::dump
-* calls PerformanceAnalysis::ReportPerformance
-* and ReportPerformance::WriteToFile
-* PerformanceAnalysis::ReportPerformance
-* for each thread/source file location instance of PerformanceAnalysis data,
-* combines all histograms into a single one and prints it to the console
-* along with outlier data
-* ReportPerformance::WriteToFile
-* writes histogram, outlier, and peak information to file separately for each
-* instance of PerformanceAnalysis data.
-*/
-
#define LOG_TAG "NBLog"
#include <algorithm>
@@ -875,14 +807,6 @@
void NBLog::MergeReader::getAndProcessSnapshot(NBLog::Reader::Snapshot &snapshot)
{
String8 timestamp, body;
- // TODO: check: is this thread safe?
- // TODO: add lost data information and notification to ReportPerformance
- size_t lost = snapshot.lost() + (snapshot.begin() - EntryIterator(snapshot.data()));
- if (lost > 0) {
- // TODO: ultimately, this will be += and reset to 0. TODO: check that this is
- // obsolete now that Merger::merge is called periodically. No data should be lost
- mLost = lost;
- }
for (auto entry = snapshot.begin(); entry != snapshot.end();) {
switch (entry->type) {
diff --git a/media/libnbaio/PerformanceAnalysis.cpp b/media/libnbaio/PerformanceAnalysis.cpp
index 4e446a4..9e0f84d 100644
--- a/media/libnbaio/PerformanceAnalysis.cpp
+++ b/media/libnbaio/PerformanceAnalysis.cpp
@@ -46,9 +46,8 @@
namespace ReportPerformance {
-// Given a the most recent timestamp of a series of audio processing
-// wakeup timestamps,
-// buckets the time interval into a histogram, searches for
+// Given an audio processing wakeup timestamp, buckets the time interval
+// since the previous timestamp into a histogram, searches for
// outliers, analyzes the outlier series for unexpectedly
// small or large values and stores these as peaks
void PerformanceAnalysis::logTsEntry(timestamp ts) {
@@ -75,7 +74,6 @@
// NormalMixer times vary much more than FastMixer times.
// TODO: mOutlierFactor values are set empirically based on what appears to be
// an outlier. Learn these values from the data.
- // TODO: the current settings are too high, change after next data collection is over
mBufferPeriod.mOutlierFactor = mBufferPeriod.mMean < kFastMixerMax ? 1.8 : 2.0;
// set outlier threshold
mBufferPeriod.mOutlier = mBufferPeriod.mMean * mBufferPeriod.mOutlierFactor;
@@ -265,7 +263,6 @@
timestamp startingTs = mHists[0].first;
// histogram which stores .1 precision ms counts instead of Jiffy multiple counts
- // TODO: when there is more data, print many histograms, possibly separated at peaks
std::map<double, int> buckets;
for (const auto &shortHist: mHists) {
for (const auto &countPair : shortHist.second) {
diff --git a/media/libnbaio/include/media/nbaio/NBLog.h b/media/libnbaio/include/media/nbaio/NBLog.h
index 5549728..2c00386 100644
--- a/media/libnbaio/include/media/nbaio/NBLog.h
+++ b/media/libnbaio/include/media/nbaio/NBLog.h
@@ -475,12 +475,6 @@
audio_utils_fifo_reader * const mFifoReader; // used to read from FIFO,
// non-NULL unless constructor fails
- // TODO: it might be clearer, instead of a direct map from source location to vector of
- // timestamps, if we instead first mapped from source location to an object that
- // represented that location. And one_of its fields would be a vector of timestamps.
- // That would allow us to record other information about the source location beyond
- // timestamps.
-
// Searches for the last entry of type <type> in the range [front, back)
// back has to be entry-aligned. Returns nullptr if none enconuntered.
static const uint8_t *findLastEntryOfTypes(const uint8_t *front, const uint8_t *back,
diff --git a/media/libnbaio/include/media/nbaio/PerformanceAnalysis.h b/media/libnbaio/include/media/nbaio/PerformanceAnalysis.h
index fe73551..50367be 100644
--- a/media/libnbaio/include/media/nbaio/PerformanceAnalysis.h
+++ b/media/libnbaio/include/media/nbaio/PerformanceAnalysis.h
@@ -35,9 +35,9 @@
class PerformanceAnalysis {
// This class stores and analyzes audio processing wakeup timestamps from NBLog
- // FIXME: currently, all performance data is stored in deques. Need to add a mutex.
- // FIXME: continue this way until analysis is done in a separate thread. Then, use
- // the fifo writer utilities.
+ // FIXME: currently, all performance data is stored in deques. Turn these into circular
+ // buffers.
+ // TODO: add a mutex.
public:
PerformanceAnalysis() {};
@@ -45,20 +45,11 @@
friend void dump(int fd, int indent,
PerformanceAnalysisMap &threadPerformanceAnalysis);
- // Given a series of audio processing wakeup timestamps,
- // compresses and and analyzes the data, and flushes
- // the timestamp series from memory.
- void processAndFlushTimeStampSeries();
-
- // Called when an audio on/off event is read from the buffer,
- // e.g. EVENT_AUDIO_STATE.
- // calls flushTimeStampSeries on the data up to the event,
- // effectively discarding the idle audio time interval
+ // Called in the case of an audio on/off event, e.g., EVENT_AUDIO_STATE.
+ // Used to discard idle time intervals
void handleStateChange();
// Writes wakeup timestamp entry to log and runs analysis
- // TODO: make this thread safe. Each thread should have its own instance
- // of PerformanceAnalysis.
void logTsEntry(timestamp ts);
// FIXME: make peakdetector and storeOutlierData a single function
@@ -67,13 +58,11 @@
// writes timestamps of significant changes to mPeakTimestamps
bool detectAndStorePeak(msInterval delta, timestamp ts);
- // runs analysis on timestamp series before it is converted to a histogram
- // finds outliers
+ // stores timestamps of intervals above a threshold: these are assumed outliers.
// writes to mOutlierData <time elapsed since previous outlier, outlier timestamp>
bool detectAndStoreOutlier(const msInterval diffMs);
// Generates a string of analysis of the buffer periods and prints to console
- // TODO: WIP write more detailed analysis
// FIXME: move this data visualization to a separate class. Model/view/controller
void reportPerformance(String8 *body, int author, log_hash_t hash,
int maxHeight = 10);
diff --git a/media/libnbaio/include/media/nbaio/ReportPerformance.h b/media/libnbaio/include/media/nbaio/ReportPerformance.h
index af12812..ec0842f 100644
--- a/media/libnbaio/include/media/nbaio/ReportPerformance.h
+++ b/media/libnbaio/include/media/nbaio/ReportPerformance.h
@@ -23,8 +23,7 @@
namespace android {
-// This class is used by reportPerformance function
-// TODO move PerformanceAnalysis::reportPerformance function to ReportPerformance.cpp
+// The String8 class is used by reportPerformance function
class String8;
namespace ReportPerformance {
@@ -35,7 +34,6 @@
constexpr int kJiffyPerMs = 10; // time unit for histogram as a multiple of milliseconds
// stores a histogram: key: observed buffer period (multiple of jiffy). value: count
-// TODO: unsigned, unsigned
using Histogram = std::map<int, int>;
using msInterval = double;
@@ -45,7 +43,6 @@
using log_hash_t = uint64_t;
-// TODO: should this return an int64_t?
static inline int deltaMs(int64_t ns1, int64_t ns2) {
return (ns2 - ns1) / (1000 * 1000);
}
@@ -59,8 +56,7 @@
return 31 - __builtin_clz(x);
}
-// Writes outlier intervals, timestamps, and histograms spanning long time
-// intervals to a file.
+// Writes outlier intervals, timestamps, peaks timestamps, and histograms to a file.
void writeToFile(const std::deque<std::pair<timestamp, Histogram>> &hists,
const std::deque<std::pair<msInterval, timestamp>> &outlierData,
const std::deque<timestamp> &peakTimestamps,