From dcc023816eed052349578d41321d0a3cfdeb518a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Wed, 10 Oct 2018 14:58:46 +0200 Subject: [PATCH] Don't increment timestamp on drop/reencode in LibvpxVp8Encoder. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I don't think this has any impact, just wanted to have a first unit test to play around with. Bug: None Change-Id: I892e2642f0243c5c9ee807cf71abcd96112b25f4 Reviewed-on: https://webrtc-review.googlesource.com/c/105000 Commit-Queue: Erik Språng Reviewed-by: Rasmus Brandt Cr-Commit-Position: refs/heads/master@{#25089} --- modules/video_coding/BUILD.gn | 5 +++ .../codecs/vp8/libvpx_vp8_encoder.cc | 3 +- .../codecs/vp8/test/mock_libvpx_interface.h | 4 +- .../codecs/vp8/test/vp8_impl_unittest.cc | 45 +++++++++++++++++++ 4 files changed, 54 insertions(+), 3 deletions(-) diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn index 82ac1325ef..e97f4c82eb 100644 --- a/modules/video_coding/BUILD.gn +++ b/modules/video_coding/BUILD.gn @@ -723,6 +723,7 @@ if (rtc_include_tests) { } deps = [ + ":mock_headers", ":video_codecs_test_framework", ":video_coding_utility", ":videocodec_test_impl", @@ -778,6 +779,10 @@ if (rtc_include_tests) { # Suppress warnings from the Chromium Clang plugin (bugs.webrtc.org/163). suppressed_configs += [ "//build/config/clang:find_bad_constructs" ] } + + if (rtc_build_libvpx) { + deps += [ rtc_libvpx_dir ] + } } rtc_source_set("video_coding_unittests") { diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc index 41dad40d9e..06b2b22095 100644 --- a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc +++ b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc @@ -831,10 +831,11 @@ int LibvpxVp8Encoder::Encode(const VideoFrame& frame, } if (error) return WEBRTC_VIDEO_CODEC_ERROR; - timestamp_ += duration; // Examines frame timestamps only. error = GetEncodedPartitions(frame); } + // TODO(sprang): Shouldn't we use the frame timestamp instead? + timestamp_ += duration; return error; } diff --git a/modules/video_coding/codecs/vp8/test/mock_libvpx_interface.h b/modules/video_coding/codecs/vp8/test/mock_libvpx_interface.h index 5804e814ec..dcff1e6a18 100644 --- a/modules/video_coding/codecs/vp8/test/mock_libvpx_interface.h +++ b/modules/video_coding/codecs/vp8/test/mock_libvpx_interface.h @@ -32,7 +32,7 @@ class MockLibvpxVp8Interface : public LibvpxInterface { unsigned int, unsigned int, unsigned char*)); - MOCK_CONST_METHOD1(img_free, vpx_codec_err_t(const vpx_codec_enc_cfg_t*)); + MOCK_CONST_METHOD1(img_free, void(vpx_image_t* img)); MOCK_CONST_METHOD2(codec_enc_config_set, vpx_codec_err_t(vpx_codec_ctx_t*, const vpx_codec_enc_cfg_t*)); @@ -84,7 +84,7 @@ class MockLibvpxVp8Interface : public LibvpxInterface { uint64_t, vpx_enc_frame_flags_t, uint64_t)); - MOCK_CONST_METHOD3(codec_get_cx_data, + MOCK_CONST_METHOD2(codec_get_cx_data, const vpx_codec_cx_pkt_t*(vpx_codec_ctx_t*, vpx_codec_iter_t*)); }; diff --git a/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc b/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc index 45a090df37..23f141696b 100644 --- a/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc +++ b/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc @@ -16,12 +16,20 @@ #include "modules/video_coding/codecs/test/video_codec_unittest.h" #include "modules/video_coding/codecs/vp8/include/vp8.h" #include "modules/video_coding/codecs/vp8/include/vp8_temporal_layers.h" +#include "modules/video_coding/codecs/vp8/libvpx_vp8_encoder.h" +#include "modules/video_coding/codecs/vp8/test/mock_libvpx_interface.h" +#include "modules/video_coding/include/mock/mock_video_codec_interface.h" #include "modules/video_coding/utility/vp8_header_parser.h" #include "rtc_base/timeutils.h" #include "test/video_codec_settings.h" namespace webrtc { +using testing::Invoke; +using testing::NiceMock; +using testing::Return; +using testing::_; + namespace { constexpr uint32_t kInitialTimestampRtp = 123; constexpr int64_t kTestNtpTimeMs = 456; @@ -380,4 +388,41 @@ TEST_F(TestVp8Impl, DontDropKeyframes) { EncodeAndExpectFrameWith(*NextInputFrame(), 0, true); } +TEST_F(TestVp8Impl, KeepsTimestampOnReencode) { + auto* const vpx = new NiceMock(); + LibvpxVp8Encoder encoder((std::unique_ptr(vpx))); + + // Settings needed to trigger ScreenshareLayers usage, which is required for + // overshoot-drop-reencode logic. + codec_settings_.targetBitrate = 200; + codec_settings_.maxBitrate = 1000; + codec_settings_.mode = VideoCodecMode::kScreensharing; + codec_settings_.VP8()->numberOfTemporalLayers = 2; + + EXPECT_CALL(*vpx, img_wrap(_, _, _, _, _, _)) + .WillOnce(Invoke([](vpx_image_t* img, vpx_img_fmt_t fmt, unsigned int d_w, + unsigned int d_h, unsigned int stride_align, + unsigned char* img_data) { + img->fmt = fmt; + img->d_w = d_w; + img->d_h = d_h; + img->img_data = img_data; + return img; + })); + EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, + encoder.InitEncode(&codec_settings_, 1, 1000)); + MockEncodedImageCallback callback; + encoder.RegisterEncodeCompleteCallback(&callback); + + // Simulate overshoot drop, re-encode: encode function will be called twice + // with the same parameters. codec_get_cx_data() will by default return no + // image data and be interpreted as drop. + EXPECT_CALL(*vpx, codec_encode(_, _, /* pts = */ 0, _, _, _)) + .Times(2) + .WillRepeatedly(Return(vpx_codec_err_t::VPX_CODEC_OK)); + + auto delta_frame = std::vector{kVideoFrameDelta}; + encoder.Encode(*NextInputFrame(), nullptr, &delta_frame); +} + } // namespace webrtc