Add media log messages to the main log

media/base/media_log.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/base/media_log.h"
 
 #include <string>
 
 #include "base/atomic_sequence_num.h"
 #include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/values.h"
 
 namespace media {
 
 // A count of all MediaLogs created in the current process. Used to generate
 // unique IDs.
 static base::StaticAtomicSequenceNumber g_media_log_count;
 
 std::string MediaLog::EventTypeToString(MediaLogEvent::Type type) {
   switch (type) {
@@ skipping 70 matching lines @@
       return "demuxer: could not parse";
     case DEMUXER_ERROR_NO_SUPPORTED_STREAMS:
       return "demuxer: no supported streams";
     case DECODER_ERROR_NOT_SUPPORTED:
       return "decoder: not supported";
   }
   NOTREACHED();
   return NULL;
 }
 
+std::string MediaLog::MediaEventToLogString(const MediaLogEvent& event) {
+  std::string param;
+  switch (event.type) {
+    case MediaLogEvent::WEBMEDIAPLAYER_CREATED:
+    case MediaLogEvent::WEBMEDIAPLAYER_DESTROYED:
+    case MediaLogEvent::PIPELINE_CREATED:
+    case MediaLogEvent::PIPELINE_DESTROYED:
+    case MediaLogEvent::PLAY:
+    case MediaLogEvent::PAUSE:
+    case MediaLogEvent::ENDED:
+    case MediaLogEvent::TEXT_ENDED:
+    case MediaLogEvent::BUFFERED_EXTENTS_CHANGED:
+    case MediaLogEvent::TOTAL_BYTES_SET:
+    case MediaLogEvent::NETWORK_ACTIVITY_SET:
+    case MediaLogEvent::PROPERTY_CHANGE:
+      return EventTypeToString(event.type);
+
+    case MediaLogEvent::LOAD:

[wolenetz, 2015/02/13 20:36:36]

I still think this method is too fragile to changes elsewhere in which params
are populated differently for each event type. Mitigations might include:
1) Tests
2) Implementing a custom ValueSerializer and iterate across all event params (if
the existing JSON impl is too wordy for logs).
3) Refactoring MediaLogEvent creation to also setup all the requisite parameters
per each event type *here*, not in the scattered callers of
MediaLog::CreateEvent. A further refactoring (IMO not required, but could be
nice) might make MediaLogEvent abstract with concrete subtypes for each specific
log event type.

At a bare minimum, DCHECK the retval of the various event.params.Get*() used
here.

I do not want this CL to introduce technical debt that will bite us later when
the log event params might diverge from current impl.

ISTM like a combination of (3) and the DCHECK might be the best way forward.

Alternatively, a more scoped-to-the-current-problem CL might not add this
method, and instead use the JSON serializer directly for the non-PIPELINE_ERROR
DVLOGs (to show full detail), and a very specific construction of pipeline error
media log message for PIPELINE_ERROR LOG(INFO)s.

[servolk, 2015/03/26 00:04:09]

Ok, I understand your concert. I've looked a bit more into this, and found a
more maintainable way to do this. We could use base::JSONWriter for most event
types, and special case just PIPELINE_ERROR events (since for those we can get a
pretty,human-readable message via media::MediaLog::PipelineStatusToString,
instead of cryptic numeric error code written by JSONWriter by default)

+      event.params.GetString("url", &param);
+      return EventTypeToString(event.type) + " url=" + param;
+
+    case MediaLogEvent::SEEK:
+      {
+        double seek_target = -1.0;
+        event.params.GetDouble("seek_target", &seek_target);
+        return EventTypeToString(event.type) + " seek_target=" +
+            base::DoubleToString(seek_target);
+      }
+
+    case MediaLogEvent::PIPELINE_STATE_CHANGED:
+      event.params.GetString("pipeline_state", &param);
+      return EventTypeToString(event.type) + " pipeline_state=" + param;
+
+    case MediaLogEvent::PIPELINE_ERROR:
+      {
+        int err;
+        event.params.GetInteger("pipeline_error", &err);
+        media::PipelineStatus status = static_cast<media::PipelineStatus>(err);
+        return EventTypeToString(event.type) + " " +
+            media::MediaLog::PipelineStatusToString(status);
+      }
+
+    case MediaLogEvent::VIDEO_SIZE_SET:
+      {
+        int width = 0;
+        int height = 0;
+        event.params.GetInteger("width", &width);
+        event.params.GetInteger("height", &height);
+        return EventTypeToString(event.type) + " " + base::IntToString(width) +
+            "x" + base::IntToString(height);
+      }
+
+    case MediaLogEvent::DURATION_SET:
+      {
+        double duration = -1.0;
+        event.params.GetDouble("duration", &duration);
+        return EventTypeToString(event.type) + " duration=" +
+            base::DoubleToString(duration);
+      }
+
+    case MediaLogEvent::MEDIA_SOURCE_ERROR:
+      event.params.GetString("error", &param);
+      return EventTypeToString(event.type) + " error=" + param;
+  }
+  NOTREACHED();
+  return NULL;
+}
+
 LogHelper::LogHelper(const LogCB& log_cb) : log_cb_(log_cb) {}
 
 LogHelper::~LogHelper() {
   if (log_cb_.is_null())
     return;
   log_cb_.Run(stream_.str());
 }
 
 MediaLog::MediaLog() : id_(g_media_log_count.GetNext()) {}
 
@@ skipping 126 matching lines @@
     const std::string& key, base::TimeDelta value) {
   scoped_ptr<MediaLogEvent> event(CreateEvent(MediaLogEvent::PROPERTY_CHANGE));
   if (value.is_max())
     event->params.SetString(key, "unknown");
   else
     event->params.SetDouble(key, value.InSecondsF());
   AddEvent(event.Pass());
 }
 
 }  //namespace media