Reland of Add QP sum stats for received streams. (patchset #2 id:300001 of https://codereview.webrtc.org/2680893002/ )

Reason for revert:
Fix the problem.

Original issue's description:
> Revert of Add QP sum stats for received streams. (patchset #10 id:180001 of https://codereview.webrtc.org/2649133005/ )
>
> Reason for revert:
> Breaks downstream build.
>
> Original issue's description:
> > Add QP sum stats for received streams.
> >
> > This is not implemented yet in any of the decoders.
> >
> > BUG=webrtc:6541
> >
> > Review-Url: https://codereview.webrtc.org/2649133005
> > Cr-Commit-Position: refs/heads/master@{#16475}
> > Committed: ff0e72fd16
>
> TBR=hta@webrtc.org,hbos@webrtc.org,sprang@webrtc.org,magjed@webrtc.org,stefan@webrtc.org,sakal@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:6541
>
> Review-Url: https://codereview.webrtc.org/2680893002 .
> Cr-Commit-Position: refs/heads/master@{#16480}
> Committed: 69fb2cca4d

TBR=hta@webrtc.org,hbos@webrtc.org,sprang@webrtc.org,magjed@webrtc.org,stefan@webrtc.org,skvlad@webrtc.org
BUG=webrtc:6541

Review-Url: https://codereview.webrtc.org/2681663005
Cr-Commit-Position: refs/heads/master@{#16511}

webrtc/media/base/mediachannel.h

@@ -781,6 +781,7 @@ struct VideoReceiverInfo : public MediaReceiverInfo {
   uint32_t frames_received;
   uint32_t frames_decoded;
   uint32_t frames_rendered;
+  rtc::Optional<uint64_t> qp_sum;
 
   // All stats below are gathered per-VideoReceiver, but some will be correlated
   // across MediaStreamTracks.  NOTE(hta): when sinking stats into per-SSRC

webrtc/media/engine/videodecodersoftwarefallbackwrapper_unittest.cc

@@ -141,6 +141,11 @@ TEST_F(VideoDecoderSoftwareFallbackWrapperTest,
       RTC_NOTREACHED();
       return -1;
     }
+    void Decoded(webrtc::VideoFrame& decodedImage,
+                 rtc::Optional<int32_t> decode_time_ms,
+                 rtc::Optional<uint8_t> qp) override {
+      RTC_NOTREACHED();
+    }
   } callback, callback2;
 
   VideoCodec codec = {};

webrtc/media/engine/webrtcvideoengine2.cc

@@ -2400,6 +2400,7 @@ WebRtcVideoChannel2::WebRtcVideoReceiveStream::GetVideoReceiverInfo(
                          stats.frame_counts.delta_frames;
   info.frames_decoded = stats.frames_decoded;
   info.frames_rendered = stats.frames_rendered;
+  info.qp_sum = stats.qp_sum;
 
   info.codec_name = GetCodecNameFromPayloadType(stats.current_payload_type);
 

webrtc/media/engine/webrtcvideoengine2_unittest.cc

@@ -3430,6 +3430,7 @@ TEST_F(WebRtcVideoChannel2Test, GetStatsTranslatesDecodeStatsCorrectly) {
   stats.frame_counts.delta_frames = 12;
   stats.frames_rendered = 13;
   stats.frames_decoded = 14;
+  stats.qp_sum = rtc::Optional<uint64_t>(15);
   stream->SetStats(stats);
 
   cricket::VideoMediaInfo info;
@@ -3449,6 +3450,7 @@ TEST_F(WebRtcVideoChannel2Test, GetStatsTranslatesDecodeStatsCorrectly) {
             info.receivers[0].frames_received);
   EXPECT_EQ(stats.frames_rendered, info.receivers[0].frames_rendered);
   EXPECT_EQ(stats.frames_decoded, info.receivers[0].frames_decoded);
+  EXPECT_EQ(stats.qp_sum, info.receivers[0].qp_sum);
 }
 
 TEST_F(WebRtcVideoChannel2Test, GetStatsTranslatesReceivePacketStatsCorrectly) {

webrtc/modules/video_coding/codecs/test/videoprocessor.h

@@ -250,6 +250,11 @@ class VideoProcessorImpl : public VideoProcessor {
       RTC_NOTREACHED();
       return -1;
     }
+    void Decoded(VideoFrame& frame,
+                 rtc::Optional<int32_t> decode_time_ms,
+                 rtc::Optional<uint8_t> qp) override {
+      RTC_NOTREACHED();
+    }
 
    private:
     VideoProcessorImpl* video_processor_;

webrtc/modules/video_coding/codecs/vp8/simulcast_unittest.h

@@ -145,6 +145,11 @@ class Vp8TestDecodedImageCallback : public DecodedImageCallback {
     RTC_NOTREACHED();
     return -1;
   }
+  void Decoded(VideoFrame& decoded_image,
+               rtc::Optional<int32_t> decode_time_ms,
+               rtc::Optional<uint8_t> qp) override {
+    RTC_NOTREACHED();
+  }
   int DecodedFrames() { return decoded_frames_; }
 
  private:

webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc

@@ -97,6 +97,11 @@ class Vp8UnitTestDecodeCompleteCallback : public webrtc::DecodedImageCallback {
     RTC_NOTREACHED();
     return -1;
   }
+  void Decoded(VideoFrame& frame,
+               rtc::Optional<int32_t> decode_time_ms,
+               rtc::Optional<uint8_t> qp) override {
+    RTC_NOTREACHED();
+  }
   bool DecodeComplete();
 
  private:

webrtc/modules/video_coding/generic_decoder.cc

@@ -48,6 +48,16 @@ int32_t VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage) {
 
 int32_t VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage,
                                          int64_t decode_time_ms) {
+  Decoded(decodedImage,
+          decode_time_ms >= 0 ? rtc::Optional<int32_t>(decode_time_ms)
+                              : rtc::Optional<int32_t>(),
+          rtc::Optional<uint8_t>());
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+void VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage,
+                                      rtc::Optional<int32_t> decode_time_ms,
+                                      rtc::Optional<uint8_t> qp) {
   TRACE_EVENT_INSTANT1("webrtc", "VCMDecodedFrameCallback::Decoded",
                        "timestamp", decodedImage.timestamp());
   // TODO(holmer): We should improve this so that we can handle multiple
@@ -63,15 +73,15 @@ int32_t VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage,
   if (frameInfo == NULL) {
     LOG(LS_WARNING) << "Too many frames backed up in the decoder, dropping "
                        "this one.";
-    return WEBRTC_VIDEO_CODEC_OK;
+    return;
   }
 
   const int64_t now_ms = _clock->TimeInMilliseconds();
-  if (decode_time_ms < 0) {
+  if (!decode_time_ms) {
     decode_time_ms =
-        static_cast<int32_t>(now_ms - frameInfo->decodeStartTimeMs);
+        rtc::Optional<int32_t>(now_ms - frameInfo->decodeStartTimeMs);
   }
-  _timing->StopDecodeTimer(decodedImage.timestamp(), decode_time_ms, now_ms,
+  _timing->StopDecodeTimer(decodedImage.timestamp(), *decode_time_ms, now_ms,
                            frameInfo->renderTimeMs);
 
   decodedImage.set_timestamp_us(
@@ -80,11 +90,10 @@ int32_t VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage,
   // TODO(sakal): Investigate why callback is NULL sometimes and replace if
   // statement with a DCHECK.
   if (callback) {
-    callback->FrameToRender(decodedImage);
+    callback->FrameToRender(decodedImage, qp);
   } else {
     LOG(LS_WARNING) << "No callback, dropping frame.";
   }
-  return WEBRTC_VIDEO_CODEC_OK;
 }
 
 int32_t VCMDecodedFrameCallback::ReceivedDecodedReferenceFrame(

webrtc/modules/video_coding/generic_decoder.h

@@ -37,11 +37,13 @@ class VCMDecodedFrameCallback : public DecodedImageCallback {
     void SetUserReceiveCallback(VCMReceiveCallback* receiveCallback);
     VCMReceiveCallback* UserReceiveCallback();
 
-    virtual int32_t Decoded(VideoFrame& decodedImage);  // NOLINT
+    int32_t Decoded(VideoFrame& decodedImage) override;
-    virtual int32_t Decoded(VideoFrame& decodedImage,   // NOLINT
+    int32_t Decoded(VideoFrame& decodedImage, int64_t decode_time_ms) override;
-                            int64_t decode_time_ms);
+    void Decoded(VideoFrame& decodedImage,
-    virtual int32_t ReceivedDecodedReferenceFrame(const uint64_t pictureId);
+                 rtc::Optional<int32_t> decode_time_ms,
-    virtual int32_t ReceivedDecodedFrame(const uint64_t pictureId);
+                 rtc::Optional<uint8_t> qp) override;
+    int32_t ReceivedDecodedReferenceFrame(const uint64_t pictureId) override;
+    int32_t ReceivedDecodedFrame(const uint64_t pictureId) override;
 
     uint64_t LastReceivedPictureID() const;
     void OnDecoderImplementationName(const char* implementation_name);

webrtc/modules/video_coding/include/mock/mock_video_codec_interface.h

@@ -57,6 +57,10 @@ class MockDecodedImageCallback : public DecodedImageCallback {
   MOCK_METHOD2(Decoded,
                int32_t(VideoFrame& decodedImage,  // NOLINT
                        int64_t decode_time_ms));
+  MOCK_METHOD3(Decoded,
+               void(VideoFrame& decodedImage,  // NOLINT
+                    rtc::Optional<int32_t> decode_time_ms,
+                    rtc::Optional<uint8_t> qp));
   MOCK_METHOD1(ReceivedDecodedReferenceFrame,
                int32_t(const uint64_t pictureId));
   MOCK_METHOD1(ReceivedDecodedFrame, int32_t(const uint64_t pictureId));

webrtc/modules/video_coding/include/video_coding_defines.h

@@ -63,7 +63,8 @@ struct VCMFrameCount {
 // rendered.
 class VCMReceiveCallback {
  public:
-  virtual int32_t FrameToRender(VideoFrame& videoFrame) = 0;  // NOLINT
+  virtual int32_t FrameToRender(VideoFrame& videoFrame,  // NOLINT
+                                rtc::Optional<uint8_t> qp) = 0;
   virtual int32_t ReceivedDecodedReferenceFrame(const uint64_t pictureId) {
     return -1;
   }

webrtc/modules/video_coding/test/test_util.cc

@@ -97,8 +97,8 @@ FileOutputFrameReceiver::~FileOutputFrameReceiver() {
   }
 }
 
-int32_t FileOutputFrameReceiver::FrameToRender(
+int32_t FileOutputFrameReceiver::FrameToRender(webrtc::VideoFrame& video_frame,
-    webrtc::VideoFrame& video_frame) {
+                                               rtc::Optional<uint8_t> qp) {
   if (timing_file_ == NULL) {
     std::string basename;
     std::string extension;

webrtc/modules/video_coding/test/test_util.h

@@ -29,26 +29,18 @@ class NullEvent : public webrtc::EventWrapper {
  public:
   virtual ~NullEvent() {}
 
-  virtual bool Set() { return true; }
+  bool Set() override { return true; }
 
-  virtual bool Reset() { return true; }
+  webrtc::EventTypeWrapper Wait(unsigned long max_time) override {  // NOLINT
-
-  virtual webrtc::EventTypeWrapper Wait(unsigned long max_time) {  // NOLINT
     return webrtc::kEventTimeout;
   }
-
-  virtual bool StartTimer(bool periodic, unsigned long time) {  // NOLINT
-    return true;
-  }
-
-  virtual bool StopTimer() { return true; }
 };
 
 class NullEventFactory : public webrtc::EventFactory {
  public:
   virtual ~NullEventFactory() {}
 
-  virtual webrtc::EventWrapper* CreateEvent() { return new NullEvent; }
+  webrtc::EventWrapper* CreateEvent() override { return new NullEvent; }
 };
 
 class FileOutputFrameReceiver : public webrtc::VCMReceiveCallback {
@@ -57,7 +49,8 @@ class FileOutputFrameReceiver : public webrtc::VCMReceiveCallback {
   virtual ~FileOutputFrameReceiver();
 
   // VCMReceiveCallback
-  virtual int32_t FrameToRender(webrtc::VideoFrame& video_frame);  // NOLINT
+  int32_t FrameToRender(webrtc::VideoFrame& video_frame,
+                        rtc::Optional<uint8_t> qp) override;
 
  private:
   std::string out_filename_;

webrtc/pc/statscollector.cc

@@ -221,6 +221,9 @@ void ExtractStats(const cricket::VideoReceiverInfo& info, StatsReport* report) {
     report->AddInt64(StatsReport::kStatsValueNameCaptureStartNtpTimeMs,
                      info.capture_start_ntp_time_ms);
   }
+  if (info.qp_sum)
+    report->AddInt64(StatsReport::kStatsValueNameQpSum, *info.qp_sum);
+
   const IntForAdd ints[] = {
     { StatsReport::kStatsValueNameCurrentDelayMs, info.current_delay_ms },
     { StatsReport::kStatsValueNameDecodeMs, info.decode_ms },

webrtc/pc/statscollector_unittest.cc

@@ -2018,6 +2018,7 @@ TEST_F(StatsCollectorTest, VerifyVideoReceiveSsrcStats) {
   // Construct a stats value to read.
   video_receiver_info.add_ssrc(1234);
   video_receiver_info.frames_decoded = 10;
+  video_receiver_info.qp_sum = rtc::Optional<uint64_t>(11);
   stats_read.receivers.push_back(video_receiver_info);
 
   EXPECT_CALL(session_, video_channel()).WillRepeatedly(Return(&video_channel));
@@ -2029,6 +2030,8 @@ TEST_F(StatsCollectorTest, VerifyVideoReceiveSsrcStats) {
   EXPECT_EQ(rtc::ToString(video_receiver_info.frames_decoded),
             ExtractSsrcStatsValue(reports,
                                   StatsReport::kStatsValueNameFramesDecoded));
+  EXPECT_EQ(rtc::ToString(*video_receiver_info.qp_sum),
+            ExtractSsrcStatsValue(reports, StatsReport::kStatsValueNameQpSum));
 }
 
 }  // namespace webrtc

webrtc/video/receive_statistics_proxy.cc

@@ -398,11 +398,26 @@ void ReceiveStatisticsProxy::DataCountersUpdated(
   }
 }
 
-void ReceiveStatisticsProxy::OnDecodedFrame() {
+void ReceiveStatisticsProxy::OnDecodedFrame(rtc::Optional<uint8_t> qp) {
   uint64_t now = clock_->TimeInMilliseconds();
 
   rtc::CritScope lock(&crit_);
   ++stats_.frames_decoded;
+  if (qp) {
+    if (!stats_.qp_sum) {
+      if (stats_.frames_decoded != 1) {
+        LOG(LS_WARNING)
+            << "Frames decoded was not 1 when first qp value was received.";
+        stats_.frames_decoded = 1;
+      }
+      stats_.qp_sum = rtc::Optional<uint64_t>(0);
+    }
+    *stats_.qp_sum += *qp;
+  } else if (stats_.qp_sum) {
+    LOG(LS_WARNING)
+        << "QP sum was already set and no QP was given for a frame.";
+    stats_.qp_sum = rtc::Optional<uint64_t>();
+  }
   decode_fps_estimator_.Update(1, now);
   stats_.decode_frame_rate = decode_fps_estimator_.Rate(now).value_or(0);
 }

webrtc/video/receive_statistics_proxy.h

@@ -45,7 +45,7 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback,
 
   VideoReceiveStream::Stats GetStats() const;
 
-  void OnDecodedFrame();
+  void OnDecodedFrame(rtc::Optional<uint8_t> qp);
   void OnSyncOffsetUpdated(int64_t sync_offset_ms, double estimated_freq_khz);
   void OnRenderedFrame(const VideoFrame& frame);
   void OnIncomingPayloadType(int payload_type);

webrtc/video/receive_statistics_proxy_unittest.cc

@@ -54,11 +54,44 @@ class ReceiveStatisticsProxyTest : public ::testing::Test {
 TEST_F(ReceiveStatisticsProxyTest, OnDecodedFrameIncreasesFramesDecoded) {
   EXPECT_EQ(0u, statistics_proxy_->GetStats().frames_decoded);
   for (uint32_t i = 1; i <= 3; ++i) {
-    statistics_proxy_->OnDecodedFrame();
+    statistics_proxy_->OnDecodedFrame(rtc::Optional<uint8_t>());
     EXPECT_EQ(i, statistics_proxy_->GetStats().frames_decoded);
   }
 }
 
+TEST_F(ReceiveStatisticsProxyTest, OnDecodedFrameWithQpResetsFramesDecoded) {
+  EXPECT_EQ(0u, statistics_proxy_->GetStats().frames_decoded);
+  for (uint32_t i = 1; i <= 3; ++i) {
+    statistics_proxy_->OnDecodedFrame(rtc::Optional<uint8_t>());
+    EXPECT_EQ(i, statistics_proxy_->GetStats().frames_decoded);
+  }
+  statistics_proxy_->OnDecodedFrame(rtc::Optional<uint8_t>(1u));
+  EXPECT_EQ(1u, statistics_proxy_->GetStats().frames_decoded);
+}
+
+TEST_F(ReceiveStatisticsProxyTest, OnDecodedFrameIncreasesQpSum) {
+  EXPECT_EQ(rtc::Optional<uint64_t>(), statistics_proxy_->GetStats().qp_sum);
+  statistics_proxy_->OnDecodedFrame(rtc::Optional<uint8_t>(3u));
+  EXPECT_EQ(rtc::Optional<uint64_t>(3u), statistics_proxy_->GetStats().qp_sum);
+  statistics_proxy_->OnDecodedFrame(rtc::Optional<uint8_t>(127u));
+  EXPECT_EQ(rtc::Optional<uint64_t>(130u),
+            statistics_proxy_->GetStats().qp_sum);
+}
+
+TEST_F(ReceiveStatisticsProxyTest, OnDecodedFrameWithoutQpQpSumWontExist) {
+  EXPECT_EQ(rtc::Optional<uint64_t>(), statistics_proxy_->GetStats().qp_sum);
+  statistics_proxy_->OnDecodedFrame(rtc::Optional<uint8_t>());
+  EXPECT_EQ(rtc::Optional<uint64_t>(), statistics_proxy_->GetStats().qp_sum);
+}
+
+TEST_F(ReceiveStatisticsProxyTest, OnDecodedFrameWithoutQpResetsQpSum) {
+  EXPECT_EQ(rtc::Optional<uint64_t>(), statistics_proxy_->GetStats().qp_sum);
+  statistics_proxy_->OnDecodedFrame(rtc::Optional<uint8_t>(3u));
+  EXPECT_EQ(rtc::Optional<uint64_t>(3u), statistics_proxy_->GetStats().qp_sum);
+  statistics_proxy_->OnDecodedFrame(rtc::Optional<uint8_t>());
+  EXPECT_EQ(rtc::Optional<uint64_t>(), statistics_proxy_->GetStats().qp_sum);
+}
+
 TEST_F(ReceiveStatisticsProxyTest, OnRenderedFrameIncreasesFramesRendered) {
   EXPECT_EQ(0u, statistics_proxy_->GetStats().frames_rendered);
   webrtc::VideoFrame frame(

webrtc/video/video_receive_stream.cc

@@ -376,10 +376,6 @@ void VideoReceiveStream::EnableEncodedFrameRecording(rtc::PlatformFile file,
 
 // TODO(tommi): This method grabs a lock 6 times.
 void VideoReceiveStream::OnFrame(const VideoFrame& video_frame) {
-  // TODO(tommi): OnDecodedFrame grabs a lock, incidentally the same lock
-  // that OnSyncOffsetUpdated() and OnRenderedFrame() below grab.
-  stats_proxy_.OnDecodedFrame();
-
   int64_t sync_offset_ms;
   double estimated_freq_khz;
   // TODO(tommi): GetStreamSyncOffsetInMs grabs three locks.  One inside the

webrtc/video/video_stream_decoder.cc

@@ -73,8 +73,11 @@ VideoStreamDecoder::~VideoStreamDecoder() {
 // callback won't necessarily be called from the decoding thread. The decoding
 // thread may have held the lock when calling VideoDecoder::Decode, Reset, or
 // Release. Acquiring the same lock in the path of decode callback can deadlock.
-int32_t VideoStreamDecoder::FrameToRender(VideoFrame& video_frame) {  // NOLINT
+int32_t VideoStreamDecoder::FrameToRender(VideoFrame& video_frame,
+                                          rtc::Optional<uint8_t> qp) {
+  receive_stats_callback_->OnDecodedFrame(qp);
   incoming_video_stream_->OnFrame(video_frame);
+
   return 0;
 }
 

webrtc/video/video_stream_decoder.h

@@ -59,7 +59,8 @@ class VideoStreamDecoder : public VCMReceiveCallback,
   ~VideoStreamDecoder();
 
   // Implements VCMReceiveCallback.
-  int32_t FrameToRender(VideoFrame& video_frame) override;  // NOLINT
+  int32_t FrameToRender(VideoFrame& video_frame,
+                        rtc::Optional<uint8_t> qp) override;
   int32_t ReceivedDecodedReferenceFrame(const uint64_t picture_id) override;
   void OnIncomingPayloadType(int payload_type) override;
   void OnDecoderImplementationName(const char* implementation_name) override;

webrtc/video_decoder.h

@@ -35,11 +35,18 @@ class DecodedImageCallback {
   // decode time excluding waiting time for any previous pending frame to
   // return. This is necessary for breaking positive feedback in the delay
   // estimation when the decoder has a single output buffer.
-  // TODO(perkj): Remove default implementation when chromium has been updated.
   virtual int32_t Decoded(VideoFrame& decodedImage, int64_t decode_time_ms) {
     // The default implementation ignores custom decode time value.
     return Decoded(decodedImage);
   }
+  // TODO(sakal): Remove other implementations when upstream projects have been
+  // updated.
+  virtual void Decoded(VideoFrame& decodedImage,
+                       rtc::Optional<int32_t> decode_time_ms,
+                       rtc::Optional<uint8_t> qp) {
+    Decoded(decodedImage,
+            decode_time_ms ? static_cast<int32_t>(*decode_time_ms) : -1);
+  }
 
   virtual int32_t ReceivedDecodedReferenceFrame(const uint64_t pictureId) {
     return -1;

webrtc/video_receive_stream.h

@@ -70,6 +70,7 @@ class VideoReceiveStream {
     int min_playout_delay_ms = 0;
     int render_delay_ms = 10;
     uint32_t frames_decoded = 0;
+    rtc::Optional<uint64_t> qp_sum;
 
     int current_payload_type = -1;