Skip to content

Commit

Permalink
Revert "Provide Environment to construct VideoBitrateAllocator"
Browse files Browse the repository at this point in the history
This reverts commit 4bf4e17.

Reason for revert: break upstream 

Original change's description:
> Provide Environment to construct VideoBitrateAllocator
>
> To allow various VideoBitrateAllocators to use propagated rather than global field trials
>
> Bug: webrtc:42220378
> Change-Id: I52816628169a54b18a4405d84fee69b101f92f72
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/349920
> Reviewed-by: Erik Språng <sprang@webrtc.org>
> Reviewed-by: Philip Eliasson <philipel@webrtc.org>
> Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
> Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#42288}

Bug: webrtc:42220378
Change-Id: I7d47eb635c2d312d97a870c2a8eca0b23d2f86a0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/350307
Owners-Override: Jeremy Leconte <jleconte@google.com>
Commit-Queue: Jeremy Leconte <jleconte@google.com>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#42290}
  • Loading branch information
jleconte2 authored and WebRTC LUCI CQ committed May 13, 2024
1 parent dcdb140 commit 16fb790
Show file tree
Hide file tree
Showing 20 changed files with 58 additions and 89 deletions.
7 changes: 4 additions & 3 deletions api/test/mock_video_bitrate_allocator_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@

namespace webrtc {

class MockVideoBitrateAllocatorFactory : public VideoBitrateAllocatorFactory {
class MockVideoBitrateAllocatorFactory
: public webrtc::VideoBitrateAllocatorFactory {
public:
~MockVideoBitrateAllocatorFactory() override { Die(); }
MOCK_METHOD(std::unique_ptr<VideoBitrateAllocator>,
Create,
(const Environment&, const VideoCodec&),
CreateVideoBitrateAllocator,
(const VideoCodec&),
(override));
MOCK_METHOD(void, Die, ());
};
Expand Down
2 changes: 0 additions & 2 deletions api/video/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -264,8 +264,6 @@ rtc_source_set("video_bitrate_allocator_factory") {
sources = [ "video_bitrate_allocator_factory.h" ]
deps = [
":video_bitrate_allocator",
"../../rtc_base:checks",
"../environment",
"../video_codecs:video_codecs_api",
]
}
Expand Down
5 changes: 1 addition & 4 deletions api/video/builtin_video_bitrate_allocator_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@ class BuiltinVideoBitrateAllocatorFactory
BuiltinVideoBitrateAllocatorFactory() = default;
~BuiltinVideoBitrateAllocatorFactory() override = default;

// TODO: bugs.webrtc.org/42220378 - propagate Environment into RateAllocators
// so that they wouldn't use global field trials string to query field trials.
std::unique_ptr<VideoBitrateAllocator> Create(
const Environment& /*env*/,
std::unique_ptr<VideoBitrateAllocator> CreateVideoBitrateAllocator(
const VideoCodec& codec) override {
// TODO(https://crbug.com/webrtc/14884): Update SvcRateAllocator to
// support simulcast and use it for VP9/AV1 simulcast as well.
Expand Down
14 changes: 1 addition & 13 deletions api/video/video_bitrate_allocator_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@

#include <memory>

#include "api/environment/environment.h"
#include "api/video/video_bitrate_allocator.h"
#include "api/video_codecs/video_codec.h"
#include "rtc_base/checks.h"

namespace webrtc {

Expand All @@ -25,19 +23,9 @@ namespace webrtc {
class VideoBitrateAllocatorFactory {
public:
virtual ~VideoBitrateAllocatorFactory() = default;

// Creates a VideoBitrateAllocator for a specific video codec.
virtual std::unique_ptr<VideoBitrateAllocator> Create(
const Environment& env,
const VideoCodec& codec) {
return CreateVideoBitrateAllocator(codec);
}
virtual std::unique_ptr<VideoBitrateAllocator> CreateVideoBitrateAllocator(
const VideoCodec& codec) {
// Newer code shouldn't call this function,
// Older code should implement it in derived classes.
RTC_CHECK_NOTREACHED();
}
const VideoCodec& codec) = 0;
};

} // namespace webrtc
Expand Down
6 changes: 3 additions & 3 deletions media/engine/webrtc_video_engine_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1363,9 +1363,9 @@ TEST(WebRtcVideoEngineNewVideoCodecFactoryTest, Vp8) {
std::unique_ptr<webrtc::MockVideoBitrateAllocatorFactory>
rate_allocator_factory =
std::make_unique<webrtc::MockVideoBitrateAllocatorFactory>();
EXPECT_CALL(
*rate_allocator_factory,
Create(_, Field(&webrtc::VideoCodec::codecType, webrtc::kVideoCodecVP8)))
EXPECT_CALL(*rate_allocator_factory,
CreateVideoBitrateAllocator(Field(&webrtc::VideoCodec::codecType,
webrtc::kVideoCodecVP8)))
.WillOnce(
[] { return std::make_unique<webrtc::MockVideoBitrateAllocator>(); });
webrtc::FieldTrialBasedConfig trials;
Expand Down
1 change: 0 additions & 1 deletion modules/video_coding/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1204,7 +1204,6 @@ if (rtc_include_tests) {
"../../api:scoped_refptr",
"../../api:simulcast_test_fixture_api",
"../../api:videocodec_test_fixture_api",
"../../api/environment",
"../../api/environment:environment_factory",
"../../api/task_queue",
"../../api/task_queue:default_task_queue_factory",
Expand Down
12 changes: 6 additions & 6 deletions modules/video_coding/codecs/test/video_codec_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ TEST_P(SpatialQualityTest, SpatialQuality) {
VideoSourceSettings source_settings = ToSourceSettings(video_info);

EncodingSettings encoding_settings = VideoCodecTester::CreateEncodingSettings(
env, codec_type, /*scalability_mode=*/"L1T1", width, height,
codec_type, /*scalability_mode=*/"L1T1", width, height,
{DataRate::KilobitsPerSec(bitrate_kbps)},
Frequency::Hertz(framerate_fps));

Expand Down Expand Up @@ -398,14 +398,14 @@ TEST_P(BitrateAdaptationTest, BitrateAdaptation) {
VideoSourceSettings source_settings = ToSourceSettings(video_info);

EncodingSettings encoding_settings = VideoCodecTester::CreateEncodingSettings(
env, codec_type, /*scalability_mode=*/"L1T1",
codec_type, /*scalability_mode=*/"L1T1",
/*width=*/640, /*height=*/360,
{DataRate::KilobitsPerSec(bitrate_kbps.first)},
/*framerate=*/Frequency::Hertz(30));

EncodingSettings encoding_settings2 =
VideoCodecTester::CreateEncodingSettings(
env, codec_type, /*scalability_mode=*/"L1T1",
codec_type, /*scalability_mode=*/"L1T1",
/*width=*/640, /*height=*/360,
{DataRate::KilobitsPerSec(bitrate_kbps.second)},
/*framerate=*/Frequency::Hertz(30));
Expand Down Expand Up @@ -484,14 +484,14 @@ TEST_P(FramerateAdaptationTest, FramerateAdaptation) {
VideoSourceSettings source_settings = ToSourceSettings(video_info);

EncodingSettings encoding_settings = VideoCodecTester::CreateEncodingSettings(
env, codec_type, /*scalability_mode=*/"L1T1",
codec_type, /*scalability_mode=*/"L1T1",
/*width=*/640, /*height=*/360,
/*bitrate=*/{DataRate::KilobitsPerSec(512)},
Frequency::Hertz(framerate_fps.first));

EncodingSettings encoding_settings2 =
VideoCodecTester::CreateEncodingSettings(
env, codec_type, /*scalability_mode=*/"L1T1",
codec_type, /*scalability_mode=*/"L1T1",
/*width=*/640, /*height=*/360,
/*bitrate=*/{DataRate::KilobitsPerSec(512)},
Frequency::Hertz(framerate_fps.second));
Expand Down Expand Up @@ -571,7 +571,7 @@ TEST(VideoCodecTest, DISABLED_EncodeDecode) {
.value_or(absl::GetFlag(FLAGS_input_framerate_fps)));

EncodingSettings encoding_settings = VideoCodecTester::CreateEncodingSettings(
env, CodecNameToCodecType(absl::GetFlag(FLAGS_encoder)),
CodecNameToCodecType(absl::GetFlag(FLAGS_encoder)),
absl::GetFlag(FLAGS_scalability_mode),
absl::GetFlag(FLAGS_width).value_or(absl::GetFlag(FLAGS_input_width)),
absl::GetFlag(FLAGS_height).value_or(absl::GetFlag(FLAGS_input_height)),
Expand Down
10 changes: 5 additions & 5 deletions modules/video_coding/codecs/test/videocodec_test_fixture_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,6 @@ VideoCodecTestFixtureImpl::VideoCodecTestFixtureImpl(Config config)
webrtc::LibvpxVp9DecoderTemplateAdapter,
webrtc::OpenH264DecoderTemplateAdapter,
webrtc::Dav1dDecoderTemplateAdapter>>()),
env_(CreateEnvironment()),
config_(config) {}

VideoCodecTestFixtureImpl::VideoCodecTestFixtureImpl(
Expand All @@ -455,7 +454,6 @@ VideoCodecTestFixtureImpl::VideoCodecTestFixtureImpl(
std::unique_ptr<VideoEncoderFactory> encoder_factory)
: encoder_factory_(std::move(encoder_factory)),
decoder_factory_(std::move(decoder_factory)),
env_(CreateEnvironment()),
config_(config) {}

VideoCodecTestFixtureImpl::~VideoCodecTestFixtureImpl() = default;
Expand Down Expand Up @@ -697,6 +695,8 @@ void VideoCodecTestFixtureImpl::VerifyVideoStatistic(
}

bool VideoCodecTestFixtureImpl::CreateEncoderAndDecoder() {
const Environment env = CreateEnvironment();

SdpVideoFormat encoder_format(CreateSdpVideoFormat(config_));
SdpVideoFormat decoder_format = encoder_format;

Expand All @@ -711,7 +711,7 @@ bool VideoCodecTestFixtureImpl::CreateEncoderAndDecoder() {
decoder_format = *config_.decoder_format;
}

encoder_ = encoder_factory_->Create(env_, encoder_format);
encoder_ = encoder_factory_->Create(env, encoder_format);
EXPECT_TRUE(encoder_) << "Encoder not successfully created.";
if (encoder_ == nullptr) {
return false;
Expand All @@ -721,7 +721,7 @@ bool VideoCodecTestFixtureImpl::CreateEncoderAndDecoder() {
config_.NumberOfSimulcastStreams(), config_.NumberOfSpatialLayers());
for (size_t i = 0; i < num_simulcast_or_spatial_layers; ++i) {
std::unique_ptr<VideoDecoder> decoder =
decoder_factory_->Create(env_, decoder_format);
decoder_factory_->Create(env, decoder_format);
EXPECT_TRUE(decoder) << "Decoder not successfully created.";
if (decoder == nullptr) {
return false;
Expand Down Expand Up @@ -818,7 +818,7 @@ bool VideoCodecTestFixtureImpl::SetUpAndInitObjects(

task_queue->SendTask([this]() {
processor_ = std::make_unique<VideoProcessor>(
env_, encoder_.get(), &decoders_, source_frame_reader_.get(), config_,
encoder_.get(), &decoders_, source_frame_reader_.get(), config_,
&stats_, &encoded_frame_writers_,
decoded_frame_writers_.empty() ? nullptr : &decoded_frame_writers_);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#include <string>
#include <vector>

#include "api/environment/environment.h"
#include "api/test/videocodec_test_fixture.h"
#include "api/video_codecs/video_decoder_factory.h"
#include "api/video_codecs/video_encoder_factory.h"
Expand Down Expand Up @@ -93,7 +92,6 @@ class VideoCodecTestFixtureImpl : public VideoCodecTestFixture {
VideoProcessor::VideoDecoderList decoders_;

// Helper objects.
const Environment env_;
Config config_;
VideoCodecTestStatsImpl stats_;
std::unique_ptr<FrameReader> source_frame_reader_;
Expand Down
9 changes: 4 additions & 5 deletions modules/video_coding/codecs/test/videoprocessor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,7 @@ void CalculateFrameQuality(const I420BufferInterface& ref_buffer,

} // namespace

VideoProcessor::VideoProcessor(const Environment& env,
webrtc::VideoEncoder* encoder,
VideoProcessor::VideoProcessor(webrtc::VideoEncoder* encoder,
VideoDecoderList* decoders,
FrameReader* input_frame_reader,
const VideoCodecTestFixture::Config& config,
Expand All @@ -151,9 +150,9 @@ VideoProcessor::VideoProcessor(const Environment& env,
stats_(stats),
encoder_(encoder),
decoders_(decoders),
bitrate_allocator_(CreateBuiltinVideoBitrateAllocatorFactory()->Create(
env,
config_.codec_settings)),
bitrate_allocator_(
CreateBuiltinVideoBitrateAllocatorFactory()
->CreateVideoBitrateAllocator(config_.codec_settings)),
encode_callback_(this),
input_frame_reader_(input_frame_reader),
merged_encoded_frames_(num_simulcast_or_spatial_layers_),
Expand Down
4 changes: 1 addition & 3 deletions modules/video_coding/codecs/test/videoprocessor.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include <vector>

#include "absl/types/optional.h"
#include "api/environment/environment.h"
#include "api/sequence_checker.h"
#include "api/task_queue/task_queue_base.h"
#include "api/test/videocodec_test_fixture.h"
Expand Down Expand Up @@ -62,8 +61,7 @@ class VideoProcessor {
using FrameWriterList = std::vector<std::unique_ptr<FrameWriter>>;
using FrameStatistics = VideoCodecTestStats::FrameStatistics;

VideoProcessor(const Environment& env,
VideoEncoder* encoder,
VideoProcessor(webrtc::VideoEncoder* encoder,
VideoDecoderList* decoders,
FrameReader* input_frame_reader,
const VideoCodecTestFixture::Config& config,
Expand Down
6 changes: 2 additions & 4 deletions modules/video_coding/codecs/test/videoprocessor_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

#include <memory>

#include "api/environment/environment_factory.h"
#include "api/scoped_refptr.h"
#include "api/test/mock_video_decoder.h"
#include "api/test/mock_video_encoder.h"
Expand Down Expand Up @@ -54,9 +53,8 @@ class VideoProcessorTest : public ::testing::Test {
ExpectInit();
q_.SendTask([this] {
video_processor_ = std::make_unique<VideoProcessor>(
CreateEnvironment(), &encoder_mock_, &decoders_, &frame_reader_mock_,
config_, &stats_, &encoded_frame_writers_,
/*decoded_frame_writers=*/nullptr);
&encoder_mock_, &decoders_, &frame_reader_mock_, config_, &stats_,
&encoded_frame_writers_, /*decoded_frame_writers=*/nullptr);
});
}

Expand Down
29 changes: 13 additions & 16 deletions modules/video_coding/video_codec_initializer_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
#include <memory>

#include "absl/types/optional.h"
#include "api/environment/environment.h"
#include "api/environment/environment_factory.h"
#include "api/scoped_refptr.h"
#include "api/test/mock_fec_controller_override.h"
#include "api/video/builtin_video_bitrate_allocator_factory.h"
Expand Down Expand Up @@ -94,10 +92,10 @@ class VideoCodecInitializerTest : public ::testing::Test {

void InitializeCodec() {
frame_buffer_controller_.reset();
codec_out_ = VideoCodecInitializer::SetupCodec(env_.field_trials(), config_,
streams_);
bitrate_allocator_ =
CreateBuiltinVideoBitrateAllocatorFactory()->Create(env_, codec_out_);
codec_out_ =
VideoCodecInitializer::SetupCodec(field_trials_, config_, streams_);
bitrate_allocator_ = CreateBuiltinVideoBitrateAllocatorFactory()
->CreateVideoBitrateAllocator(codec_out_);
RTC_CHECK(bitrate_allocator_);

// Make sure temporal layers instances have been created.
Expand Down Expand Up @@ -139,7 +137,7 @@ class VideoCodecInitializerTest : public ::testing::Test {
return stream;
}

const Environment env_ = CreateEnvironment();
const test::ExplicitKeyValueConfig field_trials_{""};
MockFecControllerOverride fec_controller_override_;

// Input settings.
Expand Down Expand Up @@ -507,7 +505,7 @@ TEST_F(VideoCodecInitializerTest, Av1SingleSpatialLayerBitratesAreConsistent) {
streams[0].scalability_mode = ScalabilityMode::kL1T2;

VideoCodec codec =
VideoCodecInitializer::SetupCodec(env_.field_trials(), config, streams);
VideoCodecInitializer::SetupCodec(field_trials_, config, streams);

EXPECT_GE(codec.spatialLayers[0].targetBitrate,
codec.spatialLayers[0].minBitrate);
Expand All @@ -522,7 +520,7 @@ TEST_F(VideoCodecInitializerTest, Av1TwoSpatialLayersBitratesAreConsistent) {
streams[0].scalability_mode = ScalabilityMode::kL2T2;

VideoCodec codec =
VideoCodecInitializer::SetupCodec(env_.field_trials(), config, streams);
VideoCodecInitializer::SetupCodec(field_trials_, config, streams);

EXPECT_GE(codec.spatialLayers[0].targetBitrate,
codec.spatialLayers[0].minBitrate);
Expand All @@ -543,7 +541,7 @@ TEST_F(VideoCodecInitializerTest, Av1TwoSpatialLayersActiveByDefault) {
config.spatial_layers = {};

VideoCodec codec =
VideoCodecInitializer::SetupCodec(env_.field_trials(), config, streams);
VideoCodecInitializer::SetupCodec(field_trials_, config, streams);

EXPECT_TRUE(codec.spatialLayers[0].active);
EXPECT_TRUE(codec.spatialLayers[1].active);
Expand All @@ -559,7 +557,7 @@ TEST_F(VideoCodecInitializerTest, Av1TwoSpatialLayersOneDeactivated) {
config.spatial_layers[1].active = false;

VideoCodec codec =
VideoCodecInitializer::SetupCodec(env_.field_trials(), config, streams);
VideoCodecInitializer::SetupCodec(field_trials_, config, streams);

EXPECT_TRUE(codec.spatialLayers[0].active);
EXPECT_FALSE(codec.spatialLayers[1].active);
Expand All @@ -577,7 +575,7 @@ TEST_F(VideoCodecInitializerTest, Vp9SingleSpatialLayerBitratesAreConsistent) {
streams[0].scalability_mode = ScalabilityMode::kL1T2;

VideoCodec codec =
VideoCodecInitializer::SetupCodec(env_.field_trials(), config, streams);
VideoCodecInitializer::SetupCodec(field_trials_, config, streams);

EXPECT_EQ(1u, codec.VP9()->numberOfSpatialLayers);
// Target is consistent with min and max (min <= target <= max).
Expand Down Expand Up @@ -605,7 +603,7 @@ TEST_F(VideoCodecInitializerTest, Vp9TwoSpatialLayersBitratesAreConsistent) {
streams[0].scalability_mode = ScalabilityMode::kL2T2;

VideoCodec codec =
VideoCodecInitializer::SetupCodec(env_.field_trials(), config, streams);
VideoCodecInitializer::SetupCodec(field_trials_, config, streams);

EXPECT_EQ(2u, codec.VP9()->numberOfSpatialLayers);
EXPECT_GE(codec.spatialLayers[0].targetBitrate,
Expand All @@ -629,15 +627,14 @@ TEST_F(VideoCodecInitializerTest, UpdatesVp9SpecificFieldsWithScalabilityMode) {
streams[0].scalability_mode = ScalabilityMode::kL2T3_KEY;

VideoCodec codec =
VideoCodecInitializer::SetupCodec(env_.field_trials(), config, streams);
VideoCodecInitializer::SetupCodec(field_trials_, config, streams);

EXPECT_EQ(codec.VP9()->numberOfSpatialLayers, 2u);
EXPECT_EQ(codec.VP9()->numberOfTemporalLayers, 3u);
EXPECT_EQ(codec.VP9()->interLayerPred, InterLayerPredMode::kOnKeyPic);

streams[0].scalability_mode = ScalabilityMode::kS3T1;
codec =
VideoCodecInitializer::SetupCodec(env_.field_trials(), config, streams);
codec = VideoCodecInitializer::SetupCodec(field_trials_, config, streams);

EXPECT_EQ(codec.VP9()->numberOfSpatialLayers, 3u);
EXPECT_EQ(codec.VP9()->numberOfTemporalLayers, 1u);
Expand Down
10 changes: 5 additions & 5 deletions rtc_tools/video_encoder/video_encoder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -333,13 +333,13 @@ class TestVideoEncoderFactoryWrapper final {
RTC_CHECK_EQ(ret, WEBRTC_VIDEO_CODEC_OK);

// Set bitrates.
std::unique_ptr<VideoBitrateAllocator> bitrate_allocator =
CreateBuiltinVideoBitrateAllocatorFactory()->Create(
env, video_codec_setting);
RTC_CHECK(bitrate_allocator);
std::unique_ptr<webrtc::VideoBitrateAllocator> bitrate_allocator_;
bitrate_allocator_ = webrtc::CreateBuiltinVideoBitrateAllocatorFactory()
->CreateVideoBitrateAllocator(video_codec_setting);
RTC_CHECK(bitrate_allocator_);

webrtc::VideoBitrateAllocation allocation =
bitrate_allocator->GetAllocation(bitrate_kbps * 1000, frame_rate_fps);
bitrate_allocator_->GetAllocation(bitrate_kbps * 1000, frame_rate_fps);
RTC_LOG(LS_INFO) << allocation.ToString();

video_encoder->SetRates(webrtc::VideoEncoder::RateControlParameters(
Expand Down
Loading

0 comments on commit 16fb790

Please sign in to comment.