From 0b1e2f327912806a3f070ce72c19dff01b130de2 Mon Sep 17 00:00:00 2001 From: ilnik Date: Mon, 10 Jul 2017 12:25:29 -0700 Subject: [PATCH] Revert of Refactor timing frame logic to work with encoders with internal sources (patchset #2 id:20001 of https://codereview.webrtc.org/2968153002/ ) Reason for revert: Failing chromoting tests Original issue's description: > Refactor timing frame logic to work with encoders with internal sources > > BUG=webrtc:7594,webrtc:7893 > > Review-Url: https://codereview.webrtc.org/2968153002 > Cr-Commit-Position: refs/heads/master@{#18955} > Committed: https://chromium.googlesource.com/external/webrtc/+/a7a4535a35e2fccfab06c79d8704c1ca92970a6e TBR=sprang@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:7594,webrtc:7893 Review-Url: https://codereview.webrtc.org/2980533002 Cr-Commit-Position: refs/heads/master@{#18957} --- .../modules/video_coding/generic_encoder.cc | 74 +++++++++---------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/webrtc/modules/video_coding/generic_encoder.cc b/webrtc/modules/video_coding/generic_encoder.cc index 8db70e27f8..17e4032144 100644 --- a/webrtc/modules/video_coding/generic_encoder.cc +++ b/webrtc/modules/video_coding/generic_encoder.cc @@ -216,6 +216,9 @@ EncodedImageCallback::Result VCMEncodedFrameCallback::OnEncodedImage( const RTPFragmentationHeader* fragmentation_header) { TRACE_EVENT_INSTANT1("webrtc", "VCMEncodedFrameCallback::Encoded", "timestamp", encoded_image._timeStamp); + bool is_timing_frame = false; + size_t outlier_frame_size = 0; + int64_t encode_start_ms = -1; size_t simulcast_svc_idx = 0; if (codec_specific->codecType == kVideoCodecVP9) { if (codec_specific->codecSpecific.VP9.num_spatial_layers > 1) @@ -228,18 +231,13 @@ EncodedImageCallback::Result VCMEncodedFrameCallback::OnEncodedImage( // TODO(ilnik): When h264 simulcast is landed, extract simulcast idx here. } - // Larger than current frame size, so it will not trigger timing frame as an - // outlier. If encoder didn't call OnFramerateChanged it will be used. - size_t outlier_frame_size = encoded_image._length + 1; - // Default best guess. If encoder didn't call OnEncodeStarted it will be used. - int64_t encode_start_ms = encoded_image.capture_time_ms_; - bool is_timing_frame = false; { rtc::CritScope crit(&timing_params_lock_); - - // Encoders with internal sources do not call OnEncodeStarted and - // OnFrameRateChanged. |timing_frames_info_| may be not filled here. + // TODO(ilnik): Workaround for hardware encoders, which do not call + // |OnEncodeStarted| correctly. Once fixed, remove conditional check. if (simulcast_svc_idx < timing_frames_info_.size()) { + RTC_CHECK_LT(simulcast_svc_idx, timing_frames_info_.size()); + auto encode_start_map = &timing_frames_info_[simulcast_svc_idx].encode_start_time_ms; auto it = encode_start_map->find(encoded_image.capture_time_ms_); @@ -251,43 +249,45 @@ EncodedImageCallback::Result VCMEncodedFrameCallback::OnEncodedImage( encode_start_map->erase(encode_start_map->begin(), it); encode_start_map->erase(it); } else { - // Encoder is with internal source: free our records of any frames just - // in case to free memory. - encode_start_map->clear(); + // Some chromium remoting unittests use generic encoder incorrectly + // If timestamps do not match, purge them all. + encode_start_map->erase(encode_start_map->begin(), + encode_start_map->end()); } - size_t target_bitrate = - timing_frames_info_[simulcast_svc_idx].target_bitrate_bytes_per_sec; - if (framerate_ > 0 && target_bitrate > 0) { - // framerate and target bitrate were reported by encoder. - size_t average_frame_size = target_bitrate / framerate_; + int64_t timing_frame_delay_ms = + encoded_image.capture_time_ms_ - last_timing_frame_time_ms_; + if (last_timing_frame_time_ms_ == -1 || + timing_frame_delay_ms >= timing_frames_thresholds_.delay_ms || + timing_frame_delay_ms == 0) { + is_timing_frame = true; + last_timing_frame_time_ms_ = encoded_image.capture_time_ms_; + } + // TODO(ilnik): Once OnFramerateChanged is called correctly by hardware + // encoders, remove the conditional check below. + if (framerate_ > 0) { + RTC_CHECK_GT(framerate_, 0); + size_t average_frame_size = + timing_frames_info_[simulcast_svc_idx].target_bitrate_bytes_per_sec + / framerate_; outlier_frame_size = average_frame_size * timing_frames_thresholds_.outlier_ratio_percent / 100; + } else { + outlier_frame_size = encoded_image._length + 1; } - } - - // Check if it's time to send a timing frame. - int64_t timing_frame_delay_ms = - encoded_image.capture_time_ms_ - last_timing_frame_time_ms_; - // Trigger threshold if it's a first frame, too long passed since the last - // timing frame, or we already sent timing frame on a different simulcast - // stream with the same capture time. - if (last_timing_frame_time_ms_ == -1 || - timing_frame_delay_ms >= timing_frames_thresholds_.delay_ms || - timing_frame_delay_ms == 0) { - is_timing_frame = true; - last_timing_frame_time_ms_ = encoded_image.capture_time_ms_; - } - - // Outliers trigger timing frames, but do not affect scheduled timing - // frames. - if (encoded_image._length >= outlier_frame_size) { - is_timing_frame = true; + } else { + // We don't have any information prior to encode start, thus we can't + // reliably detect outliers. Set outlier size to anything larger than + // current frame size. + outlier_frame_size = encoded_image._length + 1; } } - if (is_timing_frame) { + if (encoded_image._length >= outlier_frame_size) { + is_timing_frame = true; + } + if (encode_start_ms >= 0 && is_timing_frame) { encoded_image.SetEncodeTime(encode_start_ms, rtc::TimeMillis()); }