From e67c59e7d2fce1fcdb013605f37344a802dfc9e5 Mon Sep 17 00:00:00 2001 From: ilnik Date: Thu, 9 Feb 2017 04:08:56 -0800 Subject: [PATCH] Revert of Added VP8 simulcast tests. Fixed analyzer to correctly infer timestamps. (patchset #5 id:80001 of https://codereview.webrtc.org/2668763004/ ) Reason for revert: Speculative revert due to regression in perf tests. Original issue's description: > Added VP8 simulcast tests. Fixed analyzer to correctly infer timestamps. > > > BUG=webrtc:7095 > > Review-Url: https://codereview.webrtc.org/2668763004 > Cr-Commit-Position: refs/heads/master@{#16428} > Committed: https://chromium.googlesource.com/external/webrtc/+/5f4712686550ba1d069c5e4c456ffcabe7ccba97 TBR=sprang@webrtc.org,nisse@webrtc.org,mflodman@webrtc.org,magjed@webrtc.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=webrtc:7095 Review-Url: https://codereview.webrtc.org/2687073002 Cr-Commit-Position: refs/heads/master@{#16510} --- webrtc/common_video/include/frame_callback.h | 25 +---- webrtc/video/full_stack_tests.cc | 111 ------------------- webrtc/video/video_quality_test.cc | 61 ++++------ webrtc/video/video_receive_stream.cc | 4 +- webrtc/video/video_send_stream.cc | 3 +- 5 files changed, 27 insertions(+), 177 deletions(-) diff --git a/webrtc/common_video/include/frame_callback.h b/webrtc/common_video/include/frame_callback.h index 45624f1554..cc39eaa1a5 100644 --- a/webrtc/common_video/include/frame_callback.h +++ b/webrtc/common_video/include/frame_callback.h @@ -22,32 +22,13 @@ class VideoFrame; struct EncodedFrame { public: - EncodedFrame() - : data_(nullptr), - length_(0), - frame_type_(kEmptyFrame), - encoded_width_(0), - encoded_height_(0), - timestamp_(0) {} - EncodedFrame(const uint8_t* data, - size_t length, - FrameType frame_type, - uint32_t encoded_width, - uint32_t encoded_height, - uint32_t timestamp) - : data_(data), - length_(length), - frame_type_(frame_type), - encoded_width_(encoded_width), - encoded_height_(encoded_height), - timestamp_(timestamp) {} + EncodedFrame() : data_(NULL), length_(0), frame_type_(kEmptyFrame) {} + EncodedFrame(const uint8_t* data, size_t length, FrameType frame_type) + : data_(data), length_(length), frame_type_(frame_type) {} const uint8_t* data_; const size_t length_; const FrameType frame_type_; - const uint32_t encoded_width_; - const uint32_t encoded_height_; - const uint32_t timestamp_; }; class EncodedFrameObserver { diff --git a/webrtc/video/full_stack_tests.cc b/webrtc/video/full_stack_tests.cc index c7f9fc1be1..c42d48f94d 100644 --- a/webrtc/video/full_stack_tests.cc +++ b/webrtc/video/full_stack_tests.cc @@ -362,115 +362,4 @@ TEST_F(FullStackTest, ScreenshareSlidesVP9_2SL) { } #endif // !defined(RTC_DISABLE_VP9) -TEST_F(FullStackTest, SimulcastVP8_3SL_High) { - VideoQualityTest::Params simulcast; - simulcast.call.send_side_bwe = true; - simulcast.video = {true, 1280, 720, 50, - 800000, 2500000, 2500000, false, - "VP8", 1, 0, 400000, - false, false, "", "ConferenceMotion_1280_720_50"}; - simulcast.analyzer = {"simulcast_vp8_3sl_demo", 0.0, 0.0, - kFullStackTestDurationSecs}; - simulcast.pipe.loss_percent = 0; - simulcast.pipe.queue_delay_ms = 100; - VideoQualityTest::Params video_params_high; - video_params_high.video = { - true, 1280, 720, 50, - 800000, 2500000, 2500000, false, - "VP8", 1, 0, 400000, - false, false, "", "ConferenceMotion_1280_720_50"}; - VideoQualityTest::Params video_params_medium; - video_params_medium.video = { - true, 640, 360, 50, - 150000, 500000, 700000, false, - "VP8", 1, 0, 400000, - false, false, "", "ConferenceMotion_1280_720_50"}; - VideoQualityTest::Params video_params_low; - video_params_low.video = { - true, 320, 180, 50, - 30000, 150000, 200000, false, - "VP8", 1, 0, 400000, - false, false, "", "ConferenceMotion_1280_720_50"}; - - std::vector streams = {DefaultVideoStream(video_params_low), - DefaultVideoStream(video_params_medium), - DefaultVideoStream(video_params_high)}; - simulcast.ss = {streams, 2, 1, 0}; - RunTest(simulcast); -} - -TEST_F(FullStackTest, SimulcastVP8_3SL_Medium) { - VideoQualityTest::Params simulcast; - simulcast.call.send_side_bwe = true; - simulcast.video = {true, 1280, 720, 50, - 800000, 2500000, 2500000, false, - "VP8", 1, 0, 400000, - false, false, "", "ConferenceMotion_1280_720_50"}; - simulcast.analyzer = {"simulcast_vp8_3sl_demo", 0.0, 0.0, - kFullStackTestDurationSecs}; - simulcast.pipe.loss_percent = 0; - simulcast.pipe.queue_delay_ms = 100; - VideoQualityTest::Params video_params_high; - video_params_high.video = { - true, 1280, 720, 50, - 800000, 2500000, 2500000, false, - "VP8", 1, 0, 400000, - false, false, "", "ConferenceMotion_1280_720_50"}; - VideoQualityTest::Params video_params_medium; - video_params_medium.video = { - true, 640, 360, 50, - 150000, 500000, 700000, false, - "VP8", 1, 0, 400000, - false, false, "", "ConferenceMotion_1280_720_50"}; - VideoQualityTest::Params video_params_low; - video_params_low.video = { - true, 320, 180, 50, - 30000, 150000, 200000, false, - "VP8", 1, 0, 400000, - false, false, "", "ConferenceMotion_1280_720_50"}; - - std::vector streams = {DefaultVideoStream(video_params_low), - DefaultVideoStream(video_params_medium), - DefaultVideoStream(video_params_high)}; - simulcast.ss = {streams, 1, 1, 0}; - RunTest(simulcast); -} - -TEST_F(FullStackTest, SimulcastVP8_3SL_Low) { - VideoQualityTest::Params simulcast; - simulcast.call.send_side_bwe = true; - simulcast.video = {true, 1280, 720, 50, - 800000, 2500000, 2500000, false, - "VP8", 1, 0, 400000, - false, false, "", "ConferenceMotion_1280_720_50"}; - simulcast.analyzer = {"simulcast_vp8_3sl_demo", 0.0, 0.0, - kFullStackTestDurationSecs}; - simulcast.pipe.loss_percent = 0; - simulcast.pipe.queue_delay_ms = 100; - VideoQualityTest::Params video_params_high; - video_params_high.video = { - true, 1280, 720, 50, - 800000, 2500000, 2500000, false, - "VP8", 1, 0, 400000, - false, false, "", "ConferenceMotion_1280_720_50"}; - VideoQualityTest::Params video_params_medium; - video_params_medium.video = { - true, 640, 360, 50, - 150000, 500000, 700000, false, - "VP8", 1, 0, 400000, - false, false, "", "ConferenceMotion_1280_720_50"}; - VideoQualityTest::Params video_params_low; - video_params_low.video = { - true, 320, 180, 50, - 30000, 150000, 200000, false, - "VP8", 1, 0, 400000, - false, false, "", "ConferenceMotion_1280_720_50"}; - - std::vector streams = {DefaultVideoStream(video_params_low), - DefaultVideoStream(video_params_medium), - DefaultVideoStream(video_params_high)}; - simulcast.ss = {streams, 0, 1, 0}; - RunTest(simulcast); -} - } // namespace webrtc diff --git a/webrtc/video/video_quality_test.cc b/webrtc/video/video_quality_test.cc index 42d057daa3..1df648bf04 100644 --- a/webrtc/video/video_quality_test.cc +++ b/webrtc/video/video_quality_test.cc @@ -133,9 +133,7 @@ class VideoAnalyzer : public PacketReceiver, int duration_frames, FILE* graph_data_output_file, const std::string& graph_title, - uint32_t ssrc_to_analyze, - uint32_t selected_width, - uint32_t selected_height) + uint32_t ssrc_to_analyze) : transport_(transport), receiver_(nullptr), send_stream_(nullptr), @@ -145,8 +143,6 @@ class VideoAnalyzer : public PacketReceiver, graph_data_output_file_(graph_data_output_file), graph_title_(graph_title), ssrc_to_analyze_(ssrc_to_analyze), - selected_width_(selected_width), - selected_height_(selected_height), pre_encode_proxy_(this), encode_timing_proxy_(this), frames_to_process_(duration_frames), @@ -230,11 +226,10 @@ class VideoAnalyzer : public PacketReceiver, RtpUtility::RtpHeaderParser parser(packet, length); RTPHeader header; parser.Parse(&header); - if (!IsFlexfec(header.payloadType) && header.ssrc != ssrc_to_analyze_) { + if (!IsFlexfec(header.payloadType)) { // Ignore FlexFEC timestamps, to avoid collisions with media timestamps. // (FlexFEC and media are sent on different SSRCs, which have different // timestamps spaces.) - // Also ignore packets from wrong SSRC. rtc::CritScope lock(&crit_); int64_t timestamp = wrap_handler_.Unwrap(header.timestamp - rtp_timestamp_delta_); @@ -252,23 +247,13 @@ class VideoAnalyzer : public PacketReceiver, void PreEncodeOnFrame(const VideoFrame& video_frame) { rtc::CritScope lock(&crit_); - if (!first_encoded_timestamp_) { + if (!first_send_timestamp_ && rtp_timestamp_delta_ == 0) { while (frames_.front().timestamp() != video_frame.timestamp()) { ++dropped_frames_before_first_encode_; frames_.pop_front(); RTC_CHECK(!frames_.empty()); } - first_encoded_timestamp_ = - rtc::Optional(video_frame.timestamp()); - } - } - - void PostEncodeFrameCallback(const EncodedFrame& encoded_frame) { - rtc::CritScope lock(&crit_); - if (!first_sent_timestamp_ && - encoded_frame.encoded_width_ == selected_width_ && - encoded_frame.encoded_height_ == selected_height_) { - first_sent_timestamp_ = rtc::Optional(encoded_frame.timestamp_); + first_send_timestamp_ = rtc::Optional(video_frame.timestamp()); } } @@ -284,15 +269,15 @@ class VideoAnalyzer : public PacketReceiver, bool result = transport_->SendRtp(packet, length, options); { rtc::CritScope lock(&crit_); - if (rtp_timestamp_delta_ == 0 && header.ssrc == ssrc_to_analyze_) { - rtp_timestamp_delta_ = header.timestamp - *first_sent_timestamp_; - } - if (!IsFlexfec(header.payloadType) && header.ssrc == ssrc_to_analyze_) { + if (rtp_timestamp_delta_ == 0) { + rtp_timestamp_delta_ = header.timestamp - *first_send_timestamp_; + first_send_timestamp_ = rtc::Optional(); + } + if (!IsFlexfec(header.payloadType)) { // Ignore FlexFEC timestamps, to avoid collisions with media timestamps. // (FlexFEC and media are sent on different SSRCs, which have different // timestamps spaces.) - // Also ignore packets from wrong SSRC. int64_t timestamp = wrap_handler_.Unwrap(header.timestamp - rtp_timestamp_delta_); send_times_[timestamp] = current_time; @@ -490,9 +475,7 @@ class VideoAnalyzer : public PacketReceiver, void OnEncodeTiming(int64_t ntp_time_ms, int encode_time_ms) override { parent_->MeasuredEncodeTiming(ntp_time_ms, encode_time_ms); } - void EncodedFrameCallback(const EncodedFrame& frame) override { - parent_->PostEncodeFrameCallback(frame); - } + void EncodedFrameCallback(const EncodedFrame& frame) override {} private: VideoAnalyzer* const parent_; @@ -802,11 +785,9 @@ class VideoAnalyzer : public PacketReceiver, private: void OnFrame(const VideoFrame& video_frame) override { VideoFrame copy = video_frame; - // Frames from the capturer does not have a rtp timestamp. - // Create one so it can be used for comparison. - RTC_DCHECK_EQ(0, video_frame.timestamp()); copy.set_timestamp(copy.ntp_time_ms() * 90); - analyzer_->AddCapturedFrameForComparison(copy); + + analyzer_->AddCapturedFrameForComparison(video_frame); rtc::CritScope lock(&crit_); if (send_stream_input_) send_stream_input_->OnFrame(video_frame); @@ -834,7 +815,12 @@ class VideoAnalyzer : public PacketReceiver, void AddCapturedFrameForComparison(const VideoFrame& video_frame) { rtc::CritScope lock(&crit_); - frames_.push_back(video_frame); + RTC_DCHECK_EQ(0, video_frame.timestamp()); + // Frames from the capturer does not have a rtp timestamp. Create one so it + // can be used for comparison. + VideoFrame copy = video_frame; + copy.set_timestamp(copy.ntp_time_ms() * 90); + frames_.push_back(copy); } VideoSendStream* send_stream_; @@ -844,8 +830,6 @@ class VideoAnalyzer : public PacketReceiver, FILE* const graph_data_output_file_; const std::string graph_title_; const uint32_t ssrc_to_analyze_; - const uint32_t selected_width_; - const uint32_t selected_height_; PreEncodeProxy pre_encode_proxy_; OnEncodeTimingProxy encode_timing_proxy_; std::vector samples_ GUARDED_BY(comparison_lock_); @@ -880,8 +864,7 @@ class VideoAnalyzer : public PacketReceiver, std::map send_times_ GUARDED_BY(crit_); std::map recv_times_ GUARDED_BY(crit_); std::map encoded_frame_sizes_ GUARDED_BY(crit_); - rtc::Optional first_encoded_timestamp_ GUARDED_BY(crit_); - rtc::Optional first_sent_timestamp_ GUARDED_BY(crit_); + rtc::Optional first_send_timestamp_ GUARDED_BY(crit_); const double avg_psnr_threshold_; const double avg_ssim_threshold_; @@ -1329,7 +1312,7 @@ void VideoQualityTest::RunWithAnalyzer(const Params& params) { if (disable_quality_check) { fprintf(stderr, "Warning: Calculating PSNR and SSIM for downsized resolution " - "not implemented yet! Skipping PSNR and SSIM calculations!\n"); + "not implemented yet! Skipping PSNR and SSIM calculations!"); } VideoAnalyzer analyzer( @@ -1338,9 +1321,7 @@ void VideoQualityTest::RunWithAnalyzer(const Params& params) { disable_quality_check ? -1.1 : params_.analyzer.avg_ssim_threshold, params_.analyzer.test_durations_secs * params_.video.fps, graph_data_output_file, graph_title, - kVideoSendSsrcs[params_.ss.selected_stream], - static_cast(selected_stream.width), - static_cast(selected_stream.height)); + kVideoSendSsrcs[params_.ss.selected_stream]); analyzer.SetReceiver(receiver_call_->Receiver()); send_transport.SetReceiver(&analyzer); diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc index dd980c9091..0f35fde5fa 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -393,6 +393,7 @@ void VideoReceiveStream::OnFrame(const VideoFrame& video_frame) { // TODO(tommi): OnSyncOffsetUpdated grabs a lock. stats_proxy_.OnSyncOffsetUpdated(sync_offset_ms, estimated_freq_khz); } + // config_.renderer must never be null if we're getting this callback. config_.renderer->OnFrame(video_frame); @@ -410,8 +411,7 @@ EncodedImageCallback::Result VideoReceiveStream::OnEncodedImage( if (config_.pre_decode_callback) { config_.pre_decode_callback->EncodedFrameCallback( EncodedFrame(encoded_image._buffer, encoded_image._length, - encoded_image._frameType, encoded_image._encodedWidth, - encoded_image._encodedHeight, encoded_image._timeStamp)); + encoded_image._frameType)); } { rtc::CritScope lock(&ivf_writer_lock_); diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 466b5c2f00..9b5c81b9f8 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -1032,8 +1032,7 @@ EncodedImageCallback::Result VideoSendStreamImpl::OnEncodedImage( if (config_->post_encode_callback) { config_->post_encode_callback->EncodedFrameCallback( EncodedFrame(encoded_image._buffer, encoded_image._length, - encoded_image._frameType, encoded_image._encodedWidth, - encoded_image._encodedHeight, encoded_image._timeStamp)); + encoded_image._frameType)); } { rtc::CritScope lock(&encoder_activity_crit_sect_);