Adds logging of configuration information for VideoReceiveStream

webrtc/modules/audio_coding/main/acm2/acm_dump.h

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #ifndef WEBRTC_MODULES_AUDIO_CODING_MAIN_ACM2_ACM_DUMP_H_
 #define WEBRTC_MODULES_AUDIO_CODING_MAIN_ACM2_ACM_DUMP_H_
 
 #include <string>
 
 #include "webrtc/base/scoped_ptr.h"
+#include "webrtc/video_receive_stream.h"
+#include "webrtc/video_send_stream.h"
+#include "webrtc/call.h"  // For MediaType definition

[ivoc, 2015/07/14 12:13:14]

I think it would be a good idea to move these includes to the .cc file, and use
forward declarations for the types that are used on the interface. That should
lead to faster compilation.

[terelius, 2015/07/16 12:47:03]

Good idea, unfortunately it seems difficult to do. We actually use the
definition of MediaType from call.h. From the other headers we only need the
declarations of VideoReceiveStream::Config and VideoSendStream::Config, but
afaik it is not possible to forward declare nested classes outside of the class
where they are defined.

[ivoc, 2015/07/17 12:14:28]

I think forward declaring MediaType should work at least. Can you try it?

 
 namespace webrtc {
 
 // Forward declaration of storage class that is automatically generated from
 // the protobuf file.
 class ACMDumpEventStream;
 
 class AcmDumpImpl;
 
 class AcmDump {
  public:
   // The types of debug events that are currently supported for logging.
   enum class DebugEvent { kLogStart, kLogEnd, kAudioPlayout };
 
   virtual ~AcmDump() {}
 
   static rtc::scoped_ptr<AcmDump> Create();
 
   // Starts logging for the specified duration to the specified file.
   // The logging will stop automatically after the specified duration.
   // If the file already exists it will be overwritten.
   // The function will return false on failure.
+  // TODO(terelius): Can't return false because declared as void. Fix
+  // documentation?

[ivoc, 2015/07/14 12:13:14]

Sounds like a good idea, I probably forgot to change it when changing the return
value at some point :-)

[terelius, 2015/07/16 12:47:03]

Done.

   virtual void StartLogging(const std::string& file_name, int duration_ms) = 0;
 
-  // Logs an incoming or outgoing RTP packet.
-  virtual void LogRtpPacket(bool incoming,
-                            const uint8_t* packet,
-                            size_t length) = 0;
+  // Logs configuration information for webrtc::VideoReceiveStream
+  virtual void LogVideoReceiveStreamConfig(
+      const webrtc::VideoReceiveStream::Config& config) = 0;
+
+  // Logs configuration information for webrtc::VideoSendStream
+  virtual void LogVideoSendStreamConfig(
+      const webrtc::VideoSendStream::Config& config) = 0;
+
+  // Logs the header of an incoming or outgoing RTP packet.
+  virtual void LogRtpHeader(bool incoming,
+                            MediaType media_type,

[ivoc, 2015/07/14 12:13:14]

media_type should be a const reference.

[stefan-webrtc, 2015/07/14 13:28:56]

Why would we do that? Seems like MediaType is a very simple type that wouldn't
be harmful to pass by value.

[ivoc, 2015/07/14 13:58:03]

Oh, I didn't actually look at the definition of MediaType. If it's small and
simple then it makes sense to pass by value.

[terelius, 2015/07/16 12:47:03]

I'll keep passing by value unless a change of types would motivate using
references.

+                            const uint8_t* header,
+                            size_t header_length,
+                            size_t total_length) = 0;
+
+  // Logs an incoming or outgoing RTCP packet.
+  virtual void LogRtcpPacket(bool incoming,
+                             MediaType media_type,

[ivoc, 2015/07/14 12:13:14]

Same here.

[terelius, 2015/07/16 12:47:02]

I'll keep it as-is for the time being.

+                             const uint8_t* packet,
+                             size_t length) = 0;
 
   // Logs a debug event, with optional message.
   virtual void LogDebugEvent(DebugEvent event_type,
                              const std::string& event_message) = 0;
   virtual void LogDebugEvent(DebugEvent event_type) = 0;
 
   // Reads an AcmDump file and returns true when reading was successful.
   // The result is stored in the given ACMDumpEventStream object.
   static bool ParseAcmDump(const std::string& file_name,
                            ACMDumpEventStream* result);
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_AUDIO_CODING_MAIN_ACM2_ACM_DUMP_H_