Add extra documentation and minor fixes into video quality analyzer

Bug: webrtc:10138
Change-Id: Ia4ce55ca5ff92237c8d58811df8fd96cd650a5b0
Reviewed-on: https://webrtc-review.googlesource.com/c/116685
Reviewed-by: Peter Slatala <psla@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Artem Titov <titovartem@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26241}

test/pc/e2e/api/BUILD.gn

@@ -45,6 +45,7 @@ rtc_source_set("peer_connection_quality_test_fixture_api") {
     "../../../../api:libjingle_peerconnection_api",
     "../../../../api:simulated_network_api",
     "../../../../api/transport:network_control",
+    "../../../../api/units:time_delta",
     "../../../../api/video_codecs:video_codecs_api",
     "../../../../logging:rtc_event_log_api",
     "../../../../rtc_base:rtc_base",

test/pc/e2e/api/audio_quality_analyzer_interface.h

@@ -13,7 +13,10 @@
 
 namespace webrtc {
 
-class AudioQualityAnalyzerInterface {};
+class AudioQualityAnalyzerInterface {
+ public:
+  virtual ~AudioQualityAnalyzerInterface() = default;
+};
 
 }  // namespace webrtc
 

test/pc/e2e/api/peerconnection_quality_test_fixture.h

@@ -21,6 +21,7 @@
 #include "api/peer_connection_interface.h"
 #include "api/test/simulated_network.h"
 #include "api/transport/network_control.h"
+#include "api/units/time_delta.h"
 #include "api/video_codecs/video_decoder_factory.h"
 #include "api/video_codecs/video_encoder.h"
 #include "api/video_codecs/video_encoder_factory.h"
@@ -37,6 +38,15 @@ namespace webrtc {
 // TODO(titovartem) move to API when it will be stabilized.
 class PeerConnectionE2EQualityTestFixture {
  public:
+  // Contains most part from PeerConnectionFactoryDependencies. Also all fields
+  // are optional and defaults will be provided by fixture implementation if
+  // any will be omitted.
+  //
+  // Separate class was introduced to clarify which components can be
+  // overridden. For example worker and signaling threads will be provided by
+  // fixture implementation. The same is applicable to the media engine. So user
+  // can override only some parts of media engine like video encoder/decoder
+  // factories.
   struct PeerConnectionFactoryComponents {
     std::unique_ptr<CallFactoryInterface> call_factory;
     std::unique_ptr<RtcEventLogFactoryInterface> event_log_factory;
@@ -51,6 +61,15 @@ class PeerConnectionE2EQualityTestFixture {
     std::unique_ptr<VideoDecoderFactory> video_decoder_factory;
   };
 
+  // Contains most parts from PeerConnectionDependencies. Also all fields are
+  // optional and defaults will be provided by fixture implementation if any
+  // will be omitted.
+  //
+  // Separate class was introduced to clarify which components can be
+  // overridden. For example observer, which is required to
+  // PeerConnectionDependencies, will be provided by fixture implementation,
+  // so client can't inject its own. Also only network manager can be overridden
+  // inside port allocator.
   struct PeerConnectionComponents {
     std::unique_ptr<rtc::NetworkManager> network_manager;
     std::unique_ptr<webrtc::AsyncResolverFactory> async_resolver_factory;
@@ -58,26 +77,34 @@ class PeerConnectionE2EQualityTestFixture {
     std::unique_ptr<rtc::SSLCertificateVerifier> tls_cert_verifier;
   };
 
+  // Contains all components, that can be overridden in peer connection. Also
+  // has a network thread, that will be used to communicate with another peers.
   struct InjectableComponents {
     explicit InjectableComponents(rtc::Thread* network_thread)
-        : network_thread(network_thread) {}
+        : network_thread(network_thread) {
+      RTC_CHECK(network_thread);
+    }
 
-    rtc::Thread* network_thread;
+    rtc::Thread* const network_thread;
 
     std::unique_ptr<PeerConnectionFactoryComponents> pcf_dependencies;
     std::unique_ptr<PeerConnectionComponents> pc_dependencies;
   };
 
+  // Contains screen share video stream properties.
   struct ScreenShareConfig {
     // If true, slides will be generated programmatically.
     bool generate_slides;
-    int32_t slide_change_interval;
+    // Shows how long one slide should be presented on the screen during
+    // slide generation.
+    TimeDelta slide_change_interval;
     // If equal to 0, no scrolling will be applied.
-    int32_t scroll_duration;
+    TimeDelta scroll_duration;
     // If empty, default set of slides will be used.
     std::vector<std::string> slides_yuv_file_names;
   };
 
+  // Contains properties of single video stream.
   struct VideoConfig {
     size_t width;
     size_t height;
@@ -99,6 +126,7 @@ class PeerConnectionE2EQualityTestFixture {
     absl::optional<std::string> output_file_name;
   };
 
+  // Contains properties for audio in the call.
   struct AudioConfig {
     enum Mode {
       kGenerated,
@@ -115,6 +143,9 @@ class PeerConnectionE2EQualityTestFixture {
     cricket::AudioOptions audio_options;
   };
 
+  // Contains information about call media streams (up to 1 audio stream and
+  // unlimited amount of video streams) and rtc configuration, that will be used
+  // to set up peer connection.
   struct Params {
     // If |video_configs| is empty - no video should be added to the test call.
     std::vector<VideoConfig> video_configs;
@@ -124,6 +155,8 @@ class PeerConnectionE2EQualityTestFixture {
     PeerConnectionInterface::RTCConfiguration rtc_configuration;
   };
 
+  // Contains analyzers for audio and video stream. Both of them are optional
+  // and default implementations will be provided, if any will be omitted.
   struct Analyzers {
     std::unique_ptr<AudioQualityAnalyzerInterface> audio_quality_analyzer;
     std::unique_ptr<VideoQualityAnalyzerInterface> video_quality_analyzer;

test/pc/e2e/api/video_quality_analyzer_interface.h

@@ -21,21 +21,47 @@
 
 namespace webrtc {
 
+// Base interface for video quality analyzer for peer connection level end-2-end
+// tests. Interface has only one abstract method, which have to return frame id.
+// Other methods have empty implementation by default, so user can override only
+// required parts.
+//
+// VideoQualityAnalyzerInterface will be injected into WebRTC pipeline on both
+// sides of the call. Here is video data flow in WebRTC pipeline
+//
+// Alice:
+//  ___________       ________       _________
+// |           |     |        |     |         |
+// |   Frame   |-(A)→| WebRTC |-(B)→| Video   |-(C)┐
+// | Generator |     | Stack  |     | Decoder |    |
+//  ¯¯¯¯¯¯¯¯¯¯¯       ¯¯¯¯¯¯¯¯       ¯¯¯¯¯¯¯¯¯     |
+//                                               __↓________
+//                                              | Transport |
+//                                              |     &     |
+//                                              |  Network  |
+//                                               ¯¯|¯¯¯¯¯¯¯¯
+// Bob:                                            |
+//  _______       ________       _________         |
+// |       |     |        |     |         |        |
+// | Video |←(F)-| WebRTC |←(E)-| Video   |←(D)----┘
+// | Sink  |     | Stack  |     | Decoder |
+//  ¯¯¯¯¯¯¯       ¯¯¯¯¯¯¯¯       ¯¯¯¯¯¯¯¯¯
+// The analyzer will be injected in all points from A to F.
 class VideoQualityAnalyzerInterface {
  public:
   virtual ~VideoQualityAnalyzerInterface() = default;
 
   // Will be called by framework before test. |threads_count| is number of
-  // threads, that analyzer can use for heavy calculations. Analyzer can perform
+  // threads that analyzer can use for heavy calculations. Analyzer can perform
   // simple calculations on the calling thread in each method, but should
-  // remember, that is the same thread, that is used in video pipeline.
+  // remember, that it is the same thread, that is used in video pipeline.
-  virtual void Start(uint16_t threads_count) {}
+  virtual void Start(int max_threads_count) {}
 
   // Will be called when frame was generated from the input stream.
   // Returns frame id, that will be set by framework to the frame.
-  virtual uint16_t OnFrameCaptured(std::string stream_label,
+  virtual uint16_t OnFrameCaptured(const std::string& stream_label,
                                    const VideoFrame& frame) = 0;
-  // Will be called before calling the real encoder.
+  // Will be called before calling the encoder.
   virtual void OnFramePreEncode(const VideoFrame& frame) {}
   // Will be called for each EncodedImage received from encoder. Single
   // VideoFrame can produce multiple EncodedImages. Each encoded image will
@@ -44,7 +70,7 @@ class VideoQualityAnalyzerInterface {
                               const EncodedImage& encoded_image) {}
   // Will be called for each frame dropped by encoder.
   virtual void OnFrameDropped(EncodedImageCallback::DropReason reason) {}
-  // Will be called before calling the real decoder.
+  // Will be called before calling the decoder.
   virtual void OnFrameReceived(uint16_t frame_id,
                                const EncodedImage& encoded_image) {}
   // Will be called after decoding the frame. |decode_time_ms| is a decode
@@ -55,12 +81,16 @@ class VideoQualityAnalyzerInterface {
                               absl::optional<uint8_t> qp) {}
   // Will be called when frame will be obtained from PeerConnection stack.
   virtual void OnFrameRendered(const VideoFrame& frame) {}
-  // Will be called if real encoder return not WEBRTC_VIDEO_CODEC_OK.
+  // Will be called if encoder return not WEBRTC_VIDEO_CODEC_OK.
+  // All available codes are listed in
+  // modules/video_coding/include/video_error_codes.h
   virtual void OnEncoderError(const VideoFrame& frame, int32_t error_code) {}
-  // Will be called if real decoder return not WEBRTC_VIDEO_CODEC_OK.
+  // Will be called if decoder return not WEBRTC_VIDEO_CODEC_OK.
+  // All available codes are listed in
+  // modules/video_coding/include/video_error_codes.h
   virtual void OnDecoderError(uint16_t frame_id, int32_t error_code) {}
 
-  // Tells analyzer, that analysis complete and it should calculate final
+  // Tells analyzer that analysis complete and it should calculate final
   // statistics.
   virtual void Stop() {}
 };