From 569397fec738fbdacd2b057d56f179e712becc50 Mon Sep 17 00:00:00 2001 From: philipel Date: Wed, 26 Sep 2018 12:25:31 +0200 Subject: [PATCH] Reland "Added field trial WebRTC-GenericDescriptor for the new generic descriptor." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 6f68324adbf52b247e10b33a4e83a586e66cc6df. Reason for revert: Removed full stack tests that cause timeout. Original change's description: > Revert "Added field trial WebRTC-GenericDescriptor for the new generic descriptor." > > This reverts commit 3f4a4fad8cd661309ff5d9a631e89518f32e7c5e. > > Reason for revert: Breaking internal tests > > Original change's description: > > Added field trial WebRTC-GenericDescriptor for the new generic descriptor. > > > > Also parameterized tests to test the new generic descriptor and > > added --generic_descriptor flag to loopback tests. > > > > Bug: webrtc:9361 > > Change-Id: I2b76889606fe2e81249ecdebb0b7b61151682485 > > Reviewed-on: https://webrtc-review.googlesource.com/101900 > > Commit-Queue: Philip Eliasson > > Reviewed-by: Erik Språng > > Reviewed-by: Danil Chapovalov > > Reviewed-by: Stefan Holmer > > Cr-Commit-Position: refs/heads/master@{#24835} > > TBR=danilchap@webrtc.org,sprang@webrtc.org,stefan@webrtc.org,philipel@webrtc.org > > Change-Id: I4d4714a9f4ab0e95adf0f4130bc1a932efc448fa > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: webrtc:9361 > Reviewed-on: https://webrtc-review.googlesource.com/101940 > Reviewed-by: Lu Liu > Commit-Queue: Lu Liu > Cr-Commit-Position: refs/heads/master@{#24839} TBR=danilchap@webrtc.org,sprang@webrtc.org,stefan@webrtc.org,philipel@webrtc.org,lliuu@webrtc.org Change-Id: Ibcf0a1d3aa947b84e3b891b1975d0fc2c730f2ae No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:9361 Reviewed-on: https://webrtc-review.googlesource.com/102064 Commit-Queue: Philip Eliasson Reviewed-by: Philip Eliasson Cr-Commit-Position: refs/heads/master@{#24845} --- api/test/video_quality_test_fixture.h | 1 + call/rtp_payload_params.cc | 8 +++-- call/rtp_payload_params.h | 1 + call/rtp_payload_params_unittest.cc | 6 +++- .../rtp_generic_frame_descriptor_extension.h | 1 + modules/rtp_rtcp/source/rtp_sender.cc | 3 ++ test/call_test.cc | 2 ++ test/constants.cc | 1 + test/constants.h | 1 + video/end_to_end_tests/codec_tests.cc | 25 +++++++++----- video/screenshare_loopback.cc | 5 ++- video/sv_loopback.cc | 5 ++- video/video_loopback.cc | 5 ++- video/video_quality_test.cc | 33 ++++++++++++------- 14 files changed, 72 insertions(+), 25 deletions(-) diff --git a/api/test/video_quality_test_fixture.h b/api/test/video_quality_test_fixture.h index 5c0e8ce169..99451f9ee8 100644 --- a/api/test/video_quality_test_fixture.h +++ b/api/test/video_quality_test_fixture.h @@ -34,6 +34,7 @@ class VideoQualityTestFixtureInterface { ~Params(); struct CallConfig { bool send_side_bwe; + bool generic_descriptor; BitrateConstraints call_bitrate_config; int num_thumbnails; // Indicates if secondary_(video|ss|screenshare) structures are used. diff --git a/call/rtp_payload_params.cc b/call/rtp_payload_params.cc index 0c8b30755b..ffbda1a2db 100644 --- a/call/rtp_payload_params.cc +++ b/call/rtp_payload_params.cc @@ -116,7 +116,9 @@ RtpPayloadParams::RtpPayloadParams(const uint32_t ssrc, const RtpPayloadState* state) : ssrc_(ssrc), generic_picture_id_experiment_( - field_trial::IsEnabled("WebRTC-GenericPictureId")) { + field_trial::IsEnabled("WebRTC-GenericPictureId")), + generic_descriptor_experiment_( + field_trial::IsEnabled("WebRTC-GenericDescriptor")) { for (auto& spatial_layer : last_shared_frame_id_) spatial_layer.fill(-1); @@ -152,7 +154,9 @@ RTPVideoHeader RtpPayloadParams::GetRtpVideoHeader( : true; SetCodecSpecific(&rtp_video_header, first_frame_in_picture); - SetGeneric(shared_frame_id, is_keyframe, &rtp_video_header); + + if (generic_descriptor_experiment_) + SetGeneric(shared_frame_id, is_keyframe, &rtp_video_header); return rtp_video_header; } diff --git a/call/rtp_payload_params.h b/call/rtp_payload_params.h index 664ac17346..f6829cace5 100644 --- a/call/rtp_payload_params.h +++ b/call/rtp_payload_params.h @@ -60,6 +60,7 @@ class RtpPayloadParams final { RtpPayloadState state_; const bool generic_picture_id_experiment_; + const bool generic_descriptor_experiment_; }; } // namespace webrtc #endif // CALL_RTP_PAYLOAD_PARAMS_H_ diff --git a/call/rtp_payload_params_unittest.cc b/call/rtp_payload_params_unittest.cc index c125c4ee8f..a3cb379d18 100644 --- a/call/rtp_payload_params_unittest.cc +++ b/call/rtp_payload_params_unittest.cc @@ -300,7 +300,10 @@ class RtpPayloadParamsVp8ToGenericTest : public ::testing::Test { public: enum LayerSync { kNoSync, kSync }; - RtpPayloadParamsVp8ToGenericTest() : state_(), params_(123, &state_) {} + RtpPayloadParamsVp8ToGenericTest() + : generic_descriptor_field_trial_("WebRTC-GenericDescriptor/Enabled/"), + state_(), + params_(123, &state_) {} void ConvertAndCheck(int temporal_index, int64_t shared_frame_id, @@ -330,6 +333,7 @@ class RtpPayloadParamsVp8ToGenericTest : public ::testing::Test { } protected: + test::ScopedFieldTrials generic_descriptor_field_trial_; RtpPayloadState state_; RtpPayloadParams params_; }; diff --git a/modules/rtp_rtcp/source/rtp_generic_frame_descriptor_extension.h b/modules/rtp_rtcp/source/rtp_generic_frame_descriptor_extension.h index d59a9b03a4..2dd9dd49eb 100644 --- a/modules/rtp_rtcp/source/rtp_generic_frame_descriptor_extension.h +++ b/modules/rtp_rtcp/source/rtp_generic_frame_descriptor_extension.h @@ -25,6 +25,7 @@ class RtpGenericFrameDescriptorExtension { static constexpr char kUri[] = "http://www.webrtc.org/experiments/rtp-hdrext/" "generic-frame-descriptor-00"; + static constexpr int kMaxSizeBytes = 16; static bool Parse(rtc::ArrayView data, RtpGenericFrameDescriptor* descriptor); diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 90ad9e9a9f..62ac066ffa 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -22,6 +22,7 @@ #include "modules/rtp_rtcp/include/rtp_cvo.h" #include "modules/rtp_rtcp/source/byte_io.h" #include "modules/rtp_rtcp/source/playout_delay_oracle.h" +#include "modules/rtp_rtcp/source/rtp_generic_frame_descriptor_extension.h" #include "modules/rtp_rtcp/source/rtp_header_extensions.h" #include "modules/rtp_rtcp/source/rtp_packet_to_send.h" #include "modules/rtp_rtcp/source/rtp_sender_audio.h" @@ -74,6 +75,8 @@ constexpr RtpExtensionSize kVideoExtensionSizes[] = { CreateExtensionSize(), CreateExtensionSize(), {RtpMid::kId, RtpMid::kMaxValueSizeBytes}, + {RtpGenericFrameDescriptorExtension::kId, + RtpGenericFrameDescriptorExtension::kMaxSizeBytes}, }; const char* FrameTypeToString(FrameType frame_type) { diff --git a/test/call_test.cc b/test/call_test.cc index f7828a4c3f..89ac329302 100644 --- a/test/call_test.cc +++ b/test/call_test.cc @@ -238,6 +238,8 @@ void CallTest::CreateVideoSendConfig(VideoSendStream::Config* video_config, kTransportSequenceNumberExtensionId)); video_config->rtp.extensions.push_back(RtpExtension( RtpExtension::kVideoContentTypeUri, kVideoContentTypeExtensionId)); + video_config->rtp.extensions.push_back(RtpExtension( + RtpExtension::kGenericFrameDescriptorUri, kGenericDescriptorExtensionId)); if (video_encoder_configs_.empty()) { video_encoder_configs_.emplace_back(); FillEncoderConfiguration(kVideoCodecGeneric, num_video_streams, diff --git a/test/constants.cc b/test/constants.cc index 0835777570..12ae8e8449 100644 --- a/test/constants.cc +++ b/test/constants.cc @@ -20,6 +20,7 @@ const int kTransportSequenceNumberExtensionId = 8; const int kVideoRotationExtensionId = 9; const int kVideoContentTypeExtensionId = 10; const int kVideoTimingExtensionId = 11; +const int kGenericDescriptorExtensionId = 12; } // namespace test } // namespace webrtc diff --git a/test/constants.h b/test/constants.h index 85e8c18d53..3cee828b20 100644 --- a/test/constants.h +++ b/test/constants.h @@ -18,5 +18,6 @@ extern const int kTransportSequenceNumberExtensionId; extern const int kVideoRotationExtensionId; extern const int kVideoContentTypeExtensionId; extern const int kVideoTimingExtensionId; +extern const int kGenericDescriptorExtensionId; } // namespace test } // namespace webrtc diff --git a/video/end_to_end_tests/codec_tests.cc b/video/end_to_end_tests/codec_tests.cc index 31742ee3cd..5c5255567a 100644 --- a/video/end_to_end_tests/codec_tests.cc +++ b/video/end_to_end_tests/codec_tests.cc @@ -23,9 +23,13 @@ namespace webrtc { -class CodecEndToEndTest : public test::CallTest { +class CodecEndToEndTest : public test::CallTest, + public testing::WithParamInterface { public: - CodecEndToEndTest() = default; + CodecEndToEndTest() : field_trial_(GetParam()) {} + + private: + test::ScopedFieldTrials field_trial_; }; class CodecObserver : public test::EndToEndTest, @@ -89,7 +93,12 @@ class CodecObserver : public test::EndToEndTest, int frame_counter_; }; -TEST_F(CodecEndToEndTest, SendsAndReceivesVP8) { +INSTANTIATE_TEST_CASE_P(GenericDescriptor, + CodecEndToEndTest, + ::testing::Values("WebRTC-GenericDescriptor/Disabled/", + "WebRTC-GenericDescriptor/Enabled/")); + +TEST_P(CodecEndToEndTest, SendsAndReceivesVP8) { test::FunctionVideoEncoderFactory encoder_factory( []() { return VP8Encoder::Create(); }); CodecObserver test(5, kVideoRotation_0, "VP8", &encoder_factory, @@ -97,7 +106,7 @@ TEST_F(CodecEndToEndTest, SendsAndReceivesVP8) { RunBaseTest(&test); } -TEST_F(CodecEndToEndTest, SendsAndReceivesVP8Rotation90) { +TEST_P(CodecEndToEndTest, SendsAndReceivesVP8Rotation90) { test::FunctionVideoEncoderFactory encoder_factory( []() { return VP8Encoder::Create(); }); CodecObserver test(5, kVideoRotation_90, "VP8", &encoder_factory, @@ -106,7 +115,7 @@ TEST_F(CodecEndToEndTest, SendsAndReceivesVP8Rotation90) { } #if !defined(RTC_DISABLE_VP9) -TEST_F(CodecEndToEndTest, SendsAndReceivesVP9) { +TEST_P(CodecEndToEndTest, SendsAndReceivesVP9) { test::FunctionVideoEncoderFactory encoder_factory( []() { return VP9Encoder::Create(); }); CodecObserver test(500, kVideoRotation_0, "VP9", &encoder_factory, @@ -114,7 +123,7 @@ TEST_F(CodecEndToEndTest, SendsAndReceivesVP9) { RunBaseTest(&test); } -TEST_F(CodecEndToEndTest, SendsAndReceivesVP9VideoRotation90) { +TEST_P(CodecEndToEndTest, SendsAndReceivesVP9VideoRotation90) { test::FunctionVideoEncoderFactory encoder_factory( []() { return VP9Encoder::Create(); }); CodecObserver test(5, kVideoRotation_90, "VP9", &encoder_factory, @@ -123,7 +132,7 @@ TEST_F(CodecEndToEndTest, SendsAndReceivesVP9VideoRotation90) { } // Mutiplex tests are using VP9 as the underlying implementation. -TEST_F(CodecEndToEndTest, SendsAndReceivesMultiplex) { +TEST_P(CodecEndToEndTest, SendsAndReceivesMultiplex) { InternalEncoderFactory internal_encoder_factory; InternalDecoderFactory decoder_factory; test::FunctionVideoEncoderFactory encoder_factory( @@ -138,7 +147,7 @@ TEST_F(CodecEndToEndTest, SendsAndReceivesMultiplex) { RunBaseTest(&test); } -TEST_F(CodecEndToEndTest, SendsAndReceivesMultiplexVideoRotation90) { +TEST_P(CodecEndToEndTest, SendsAndReceivesMultiplexVideoRotation90) { InternalEncoderFactory internal_encoder_factory; InternalDecoderFactory decoder_factory; test::FunctionVideoEncoderFactory encoder_factory( diff --git a/video/screenshare_loopback.cc b/video/screenshare_loopback.cc index 804fbccf25..ec1d59e6a4 100644 --- a/video/screenshare_loopback.cc +++ b/video/screenshare_loopback.cc @@ -220,6 +220,8 @@ DEFINE_bool(logs, false, "print logs to stderr"); DEFINE_bool(send_side_bwe, true, "Use send-side bandwidth estimation"); +DEFINE_bool(generic_descriptor, false, "Use the generic frame descriptor."); + DEFINE_bool(allow_reordering, false, "Allow packet reordering to occur"); DEFINE_string( @@ -288,7 +290,8 @@ void Loopback() { call_bitrate_config.max_bitrate_bps = -1; // Don't cap bandwidth estimate. VideoQualityTest::Params params; - params.call = {flags::FLAG_send_side_bwe, call_bitrate_config}; + params.call = {flags::FLAG_send_side_bwe, flags::FLAG_generic_descriptor, + call_bitrate_config}; params.video[0] = {true, flags::Width(), flags::Height(), diff --git a/video/sv_loopback.cc b/video/sv_loopback.cc index 0555fb3552..25bfa01dbc 100644 --- a/video/sv_loopback.cc +++ b/video/sv_loopback.cc @@ -419,6 +419,8 @@ DEFINE_bool(logs, false, "print logs to stderr"); DEFINE_bool(send_side_bwe, true, "Use send-side bandwidth estimation"); +DEFINE_bool(generic_descriptor, false, "Use the generic frame descriptor."); + DEFINE_bool(allow_reordering, false, "Allow packet reordering to occur"); DEFINE_bool(use_ulpfec, false, "Use RED+ULPFEC forward error correction."); @@ -492,7 +494,8 @@ void Loopback() { 1000; VideoQualityTest::Params params, camera_params, screenshare_params; - params.call = {flags::FLAG_send_side_bwe, call_bitrate_config, 0}; + params.call = {flags::FLAG_send_side_bwe, flags::FLAG_generic_descriptor, + call_bitrate_config, 0}; params.call.dual_video = true; params.video[screenshare_idx] = { true, diff --git a/video/video_loopback.cc b/video/video_loopback.cc index 52f0a65084..71a86f73d6 100644 --- a/video/video_loopback.cc +++ b/video/video_loopback.cc @@ -235,6 +235,8 @@ DEFINE_bool(logs, false, "print logs to stderr"); DEFINE_bool(send_side_bwe, true, "Use send-side bandwidth estimation"); +DEFINE_bool(generic_descriptor, false, "Use the generic frame descriptor."); + DEFINE_bool(allow_reordering, false, "Allow packet reordering to occur"); DEFINE_bool(use_ulpfec, false, "Use RED+ULPFEC forward error correction."); @@ -292,7 +294,8 @@ void Loopback() { call_bitrate_config.max_bitrate_bps = -1; // Don't cap bandwidth estimate. VideoQualityTest::Params params; - params.call = {flags::FLAG_send_side_bwe, call_bitrate_config, 0}; + params.call = {flags::FLAG_send_side_bwe, flags::FLAG_generic_descriptor, + call_bitrate_config, 0}; params.video[0] = {flags::FLAG_video, flags::Width(), flags::Height(), diff --git a/video/video_quality_test.cc b/video/video_quality_test.cc index 7f7c8737a1..cd4c5f7608 100644 --- a/video/video_quality_test.cc +++ b/video/video_quality_test.cc @@ -214,7 +214,7 @@ VideoQualityTest::VideoQualityTest( } VideoQualityTest::Params::Params() - : call({false, BitrateConstraints(), 0}), + : call({false, false, BitrateConstraints(), 0}), video{{false, 640, 480, 30, 50, 800, 800, false, "VP8", 1, -1, 0, false, false, false, ""}, {false, 640, 480, 30, 50, 800, 800, false, "VP8", 1, -1, 0, false, @@ -536,18 +536,29 @@ void VideoQualityTest::SetupVideo(Transport* send_transport, } video_send_configs_[video_idx].rtp.extensions.clear(); if (params_.call.send_side_bwe) { - video_send_configs_[video_idx].rtp.extensions.push_back( - RtpExtension(RtpExtension::kTransportSequenceNumberUri, - test::kTransportSequenceNumberExtensionId)); + video_send_configs_[video_idx].rtp.extensions.emplace_back( + RtpExtension::kTransportSequenceNumberUri, + test::kTransportSequenceNumberExtensionId); } else { - video_send_configs_[video_idx].rtp.extensions.push_back(RtpExtension( - RtpExtension::kAbsSendTimeUri, test::kAbsSendTimeExtensionId)); + video_send_configs_[video_idx].rtp.extensions.emplace_back( + RtpExtension::kAbsSendTimeUri, test::kAbsSendTimeExtensionId); } - video_send_configs_[video_idx].rtp.extensions.push_back( - RtpExtension(RtpExtension::kVideoContentTypeUri, - test::kVideoContentTypeExtensionId)); - video_send_configs_[video_idx].rtp.extensions.push_back(RtpExtension( - RtpExtension::kVideoTimingUri, test::kVideoTimingExtensionId)); + + if (params_.call.generic_descriptor) { + // The generic descriptor is currently behind a field trial, so it needs + // to be set for this flag to have any effect. + // TODO(philipel): Remove this check when the experiment is removed. + RTC_CHECK(field_trial::IsEnabled("WebRTC-GenericDescriptor")); + + video_send_configs_[video_idx].rtp.extensions.emplace_back( + RtpExtension::kGenericFrameDescriptorUri, + test::kGenericDescriptorExtensionId); + } + + video_send_configs_[video_idx].rtp.extensions.emplace_back( + RtpExtension::kVideoContentTypeUri, test::kVideoContentTypeExtensionId); + video_send_configs_[video_idx].rtp.extensions.emplace_back( + RtpExtension::kVideoTimingUri, test::kVideoTimingExtensionId); video_encoder_configs_[video_idx].video_format.name = params_.video[video_idx].codec;