Cast: Provide more meaningful stats.

chrome/renderer/media/cast_session_delegate.cc

 // Copyright 2013 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 "chrome/renderer/media/cast_session_delegate.h"
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "chrome/renderer/media/cast_threads.h"
 #include "chrome/renderer/media/cast_transport_sender_ipc.h"
 #include "content/public/renderer/render_thread.h"
 #include "media/cast/cast_config.h"
 #include "media/cast/cast_environment.h"
 #include "media/cast/cast_sender.h"
 #include "media/cast/logging/encoding_event_subscriber.h"
 #include "media/cast/logging/log_serializer.h"
 #include "media/cast/logging/logging_defines.h"
+#include "media/cast/logging/receiver_time_offset_estimator_impl.h"
 #include "media/cast/logging/stats_event_subscriber.h"
-#include "media/cast/logging/stats_util.h"
 #include "media/cast/transport/cast_transport_config.h"
 #include "media/cast/transport/cast_transport_sender.h"
 
 using media::cast::AudioSenderConfig;
 using media::cast::CastEnvironment;
 using media::cast::CastSender;
 using media::cast::VideoSenderConfig;
 
 static base::LazyInstance<CastThreads> g_cast_threads =
     LAZY_INSTANCE_INITIALIZER;
@@ skipping 15 matching lines @@
     : io_message_loop_proxy_(
           content::RenderThread::Get()->GetIOMessageLoopProxy()),
       weak_factory_(this) {
   DCHECK(io_message_loop_proxy_);
 }
 
 CastSessionDelegate::~CastSessionDelegate() {
   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
 
   if (cast_environment_.get()) {
+    if (receiver_offset_estimator_.get()) {
+      cast_environment_->Logging()->RemoveRawEventSubscriber(
+          receiver_offset_estimator_.get());
+    }
     if (audio_event_subscriber_.get()) {
       cast_environment_->Logging()->RemoveRawEventSubscriber(
           audio_event_subscriber_.get());
     }
     if (video_event_subscriber_.get()) {
       cast_environment_->Logging()->RemoveRawEventSubscriber(
           video_event_subscriber_.get());
     }
   }
 }
@@ skipping 73 matching lines @@
 
   cast_sender_ = CastSender::Create(cast_environment_, cast_transport_.get());
   cast_transport_->SetPacketReceiver(cast_sender_->packet_receiver());
 }
 
 void CastSessionDelegate::ToggleLogging(bool is_audio, bool enable) {
   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
   if (!cast_environment_.get())
     return;
   if (enable) {
+    if (!receiver_offset_estimator_.get()) {

[miu, 2014/04/15 21:29:43]

There's a huge amount of copy-and-paste going throughout CastSessionDelegate,
and especially in this method.  Each EventSubscriber is basically going through
the same steps.

Suggestion: Rather than five separate scoped_ptr<> members, can you just keep a
list<linked_ptr<Subscriber>>?  Then your code becomes:

if (enable) {
  DCHECK(event_subscribers_.empty());
  event_subscribers_.push_back(new FooSubscriber(...));
  event_subscribers_.push_back(new BarSubscriber(...));
  event_subscribers_.push_back(new BazSubscriber(...));
  event_subscribers_.push_back(new GlarfSubscriber(...));
  for (it = event_subscribers_.begin(); it != end(); ++it)
    cast_environment_->Logging()->AddRawEventSubscriber(it->get());
} else {
  for (it = event_subscribers_.begin(); it != end(); ++it)
    cast_environment_->Logging()-RemovedRawEventSubscriber(it->get());
  event_subscribers_.clear();
}

[imcheng, 2014/04/17 19:19:06]

Per offline discussion I am defining a new struct to bundle the subscribers
together. But since we support switching on/off logging by stream, we have 2
sub-bundles. I am going to use a map from bool to sub-bundles to reduce some
code duplication (as opposed to "if(is_audio) ... else ..."). If we do support
more than 2 streams later, it will not be hard to change the key from bool to
int (stream id).


On 2014/04/15 21:29:43, miu wrote:

+      receiver_offset_estimator_.reset(
+          new media::cast::ReceiverTimeOffsetEstimatorImpl);
+      cast_environment_->Logging()->AddRawEventSubscriber(
+          receiver_offset_estimator_.get());
+    }
     if (is_audio) {
       if (!audio_event_subscriber_.get()) {
         audio_event_subscriber_.reset(new media::cast::EncodingEventSubscriber(
             media::cast::AUDIO_EVENT, kMaxAudioEventEntries));
         cast_environment_->Logging()->AddRawEventSubscriber(
             audio_event_subscriber_.get());
       }
       if (!audio_stats_subscriber_.get()) {
-        audio_stats_subscriber_.reset(
-            new media::cast::StatsEventSubscriber(media::cast::AUDIO_EVENT));
+        audio_stats_subscriber_.reset(new media::cast::StatsEventSubscriber(
+            media::cast::AUDIO_EVENT,
+            cast_environment_->Clock(),
+            receiver_offset_estimator_.get()));
         cast_environment_->Logging()->AddRawEventSubscriber(
             audio_stats_subscriber_.get());
       }
     } else {
       if (!video_event_subscriber_.get()) {
         video_event_subscriber_.reset(new media::cast::EncodingEventSubscriber(
             media::cast::VIDEO_EVENT, kMaxVideoEventEntries));
         cast_environment_->Logging()->AddRawEventSubscriber(
             video_event_subscriber_.get());
       }
       if (!video_stats_subscriber_.get()) {
-        video_stats_subscriber_.reset(
-            new media::cast::StatsEventSubscriber(media::cast::VIDEO_EVENT));
+        video_stats_subscriber_.reset(new media::cast::StatsEventSubscriber(
+            media::cast::VIDEO_EVENT,
+            cast_environment_->Clock(),
+            receiver_offset_estimator_.get()));
         cast_environment_->Logging()->AddRawEventSubscriber(
             video_stats_subscriber_.get());
       }
     }
   } else {
+    if (receiver_offset_estimator_.get()) {
+      cast_environment_->Logging()->RemoveRawEventSubscriber(
+          receiver_offset_estimator_.get());
+      receiver_offset_estimator_.reset();
+    }
+
     if (is_audio) {
       if (audio_event_subscriber_.get()) {
         cast_environment_->Logging()->RemoveRawEventSubscriber(
             audio_event_subscriber_.get());
         audio_event_subscriber_.reset();
       }
       if (audio_stats_subscriber_.get()) {
         cast_environment_->Logging()->RemoveRawEventSubscriber(
             audio_stats_subscriber_.get());
         audio_stats_subscriber_.reset();
@@ skipping 56 matching lines @@
 
 void CastSessionDelegate::GetStatsAndReset(bool is_audio,
                                            const StatsCallback& callback) {
   media::cast::StatsEventSubscriber* subscriber =
       is_audio ? audio_stats_subscriber_.get() : video_stats_subscriber_.get();
   if (!subscriber) {
     callback.Run(make_scoped_ptr(new base::DictionaryValue).Pass());
     return;
   }
 
-  media::cast::FrameStatsMap frame_stats;
-  subscriber->GetFrameStats(&frame_stats);
-  media::cast::PacketStatsMap packet_stats;
-  subscriber->GetPacketStats(&packet_stats);
+  scoped_ptr<base::DictionaryValue> stats = subscriber->GetStats();
   subscriber->Reset();
 
-  scoped_ptr<base::DictionaryValue> stats = media::cast::ConvertStats(
-      frame_stats, packet_stats);
-
   callback.Run(stats.Pass());
 }
 
 void CastSessionDelegate::StatusNotificationCB(
     media::cast::transport::CastTransportStatus unused_status) {
   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
   // TODO(hubbe): Call javascript UDPTransport error function.
 }
 
 void CastSessionDelegate::InitializationResultCB(
@@ skipping 20 matching lines @@
        ++it) {
     cast_environment_->Logging()->InsertPacketEvent(it->timestamp,
                                                     it->type,
                                                     it->rtp_timestamp,
                                                     it->frame_id,
                                                     it->packet_id,
                                                     it->max_packet_id,
                                                     it->size);
   }
 }