From c0d31e915ca6d730c3d7a8219c229ad53a453201 Mon Sep 17 00:00:00 2001 From: Per Date: Thu, 31 Mar 2016 17:23:39 +0200 Subject: [PATCH] Change VideoSourceInterface::needs_denoising() to return rtc::Optional It turns out that it is used as if it has three states: on/off default. This reverts back to the behaviour prior to https://codereview.webrtc.org/1773993002 BUG=chromium:594434 R=pbos@webrtc.org, pthatcher@webrtc.org Review URL: https://codereview.webrtc.org/1842073002 . Cr-Commit-Position: refs/heads/master@{#12181} --- webrtc/api/mediastreaminterface.h | 7 +++++-- webrtc/api/rtpsender.cc | 3 +-- webrtc/api/videocapturertracksource.cc | 10 +++++----- webrtc/api/videocapturertracksource.h | 6 ++++-- .../api/videocapturertracksource_unittest.cc | 19 ++++++++++--------- webrtc/api/videosourceproxy.h | 2 +- webrtc/api/videotracksource.h | 3 ++- 7 files changed, 28 insertions(+), 22 deletions(-) diff --git a/webrtc/api/mediastreaminterface.h b/webrtc/api/mediastreaminterface.h index 1ea094b714..349fcc02bb 100644 --- a/webrtc/api/mediastreaminterface.h +++ b/webrtc/api/mediastreaminterface.h @@ -23,6 +23,7 @@ #include "webrtc/base/basictypes.h" #include "webrtc/base/refcount.h" #include "webrtc/base/scoped_ref_ptr.h" +#include "webrtc/base/optional.h" #include "webrtc/media/base/mediachannel.h" #include "webrtc/media/base/videosinkinterface.h" #include "webrtc/media/base/videosourceinterface.h" @@ -124,10 +125,12 @@ class VideoTrackSourceInterface // implicit behavior. virtual bool is_screencast() const = 0; - // Indicates that the encoder should denoise the video before encoding it. + // Indicates that the encoder should denoise video before encoding it. + // If it is not set, the default configuration is used which is different + // depending on video codec. // TODO(perkj): Remove this once denoising is done by the source, and not by // the encoder. - virtual bool needs_denoising() const = 0; + virtual rtc::Optional needs_denoising() const = 0; protected: virtual ~VideoTrackSourceInterface() {} diff --git a/webrtc/api/rtpsender.cc b/webrtc/api/rtpsender.cc index a7537867e2..94cea6c2c9 100644 --- a/webrtc/api/rtpsender.cc +++ b/webrtc/api/rtpsender.cc @@ -332,8 +332,7 @@ void VideoRtpSender::SetVideoSend() { VideoTrackSourceInterface* source = track_->GetSource(); if (source) { options.is_screencast = rtc::Optional(source->is_screencast()); - options.video_noise_reduction = - rtc::Optional(source->needs_denoising()); + options.video_noise_reduction = source->needs_denoising(); } provider_->SetVideoSend(ssrc_, track_->enabled(), &options); } diff --git a/webrtc/api/videocapturertracksource.cc b/webrtc/api/videocapturertracksource.cc index 1e9e35382c..0543dff3f3 100644 --- a/webrtc/api/videocapturertracksource.cc +++ b/webrtc/api/videocapturertracksource.cc @@ -240,10 +240,11 @@ const cricket::VideoFormat& GetBestCaptureFormat( // Return false if the key is mandatory, and the value is invalid. bool ExtractOption(const MediaConstraintsInterface* all_constraints, const std::string& key, - bool* option) { + rtc::Optional* option) { size_t mandatory = 0; - *option = false; - if (FindConstraint(all_constraints, key, option, &mandatory)) { + bool value; + if (FindConstraint(all_constraints, key, &value, &mandatory)) { + *option = rtc::Optional(value); return true; } @@ -288,8 +289,7 @@ VideoCapturerTrackSource::VideoCapturerTrackSource( : VideoTrackSource(capturer, worker_thread, remote), signaling_thread_(rtc::Thread::Current()), video_capturer_(capturer), - started_(false), - needs_denoising_(false) { + started_(false) { video_capturer_->SignalStateChange.connect( this, &VideoCapturerTrackSource::OnStateChange); } diff --git a/webrtc/api/videocapturertracksource.h b/webrtc/api/videocapturertracksource.h index 9a8d05d9ed..3673bf7fd4 100644 --- a/webrtc/api/videocapturertracksource.h +++ b/webrtc/api/videocapturertracksource.h @@ -54,7 +54,9 @@ class VideoCapturerTrackSource : public VideoTrackSource, bool is_screencast() const override { return video_capturer_->IsScreencast(); } - bool needs_denoising() const override { return needs_denoising_; } + rtc::Optional needs_denoising() const override { + return needs_denoising_; + } void Stop() override; void Restart() override; @@ -75,7 +77,7 @@ class VideoCapturerTrackSource : public VideoTrackSource, rtc::scoped_ptr video_capturer_; bool started_; cricket::VideoFormat format_; - bool needs_denoising_; + rtc::Optional needs_denoising_; }; } // namespace webrtc diff --git a/webrtc/api/videocapturertracksource_unittest.cc b/webrtc/api/videocapturertracksource_unittest.cc index 1a9d3ac5e1..39252934eb 100644 --- a/webrtc/api/videocapturertracksource_unittest.cc +++ b/webrtc/api/videocapturertracksource_unittest.cc @@ -334,17 +334,17 @@ TEST_F(VideoCapturerTrackSourceTest, InvalidOptionalConstraint) { TEST_F(VideoCapturerTrackSourceTest, SetValidDenoisingConstraint) { FakeConstraints constraints; - constraints.AddMandatory(MediaConstraintsInterface::kNoiseReduction, "true"); + constraints.AddMandatory(MediaConstraintsInterface::kNoiseReduction, "false"); CreateVideoCapturerSource(&constraints); - EXPECT_TRUE(source_->needs_denoising()); + EXPECT_EQ(rtc::Optional(false), source_->needs_denoising()); } TEST_F(VideoCapturerTrackSourceTest, NoiseReductionConstraintNotSet) { FakeConstraints constraints; CreateVideoCapturerSource(&constraints); - EXPECT_FALSE(source_->needs_denoising()); + EXPECT_EQ(rtc::Optional(), source_->needs_denoising()); } TEST_F(VideoCapturerTrackSourceTest, @@ -355,7 +355,7 @@ TEST_F(VideoCapturerTrackSourceTest, CreateVideoCapturerSource(&constraints); - EXPECT_FALSE(source_->needs_denoising()); + EXPECT_EQ(rtc::Optional(false), source_->needs_denoising()); } TEST_F(VideoCapturerTrackSourceTest, NoiseReductionAndInvalidKeyOptional) { @@ -367,7 +367,7 @@ TEST_F(VideoCapturerTrackSourceTest, NoiseReductionAndInvalidKeyOptional) { EXPECT_EQ_WAIT(MediaSourceInterface::kLive, state_observer_->state(), kMaxWaitMs); - EXPECT_TRUE(source_->needs_denoising()); + EXPECT_EQ(rtc::Optional(true), source_->needs_denoising()); } TEST_F(VideoCapturerTrackSourceTest, NoiseReductionAndInvalidKeyMandatory) { @@ -379,7 +379,7 @@ TEST_F(VideoCapturerTrackSourceTest, NoiseReductionAndInvalidKeyMandatory) { EXPECT_EQ_WAIT(MediaSourceInterface::kEnded, state_observer_->state(), kMaxWaitMs); - EXPECT_FALSE(source_->needs_denoising()); + EXPECT_EQ(rtc::Optional(), source_->needs_denoising()); } TEST_F(VideoCapturerTrackSourceTest, InvalidDenoisingValueOptional) { @@ -391,7 +391,8 @@ TEST_F(VideoCapturerTrackSourceTest, InvalidDenoisingValueOptional) { EXPECT_EQ_WAIT(MediaSourceInterface::kLive, state_observer_->state(), kMaxWaitMs); - EXPECT_FALSE(source_->needs_denoising()); + + EXPECT_EQ(rtc::Optional(), source_->needs_denoising()); } TEST_F(VideoCapturerTrackSourceTest, InvalidDenoisingValueMandatory) { @@ -405,7 +406,7 @@ TEST_F(VideoCapturerTrackSourceTest, InvalidDenoisingValueMandatory) { EXPECT_EQ_WAIT(MediaSourceInterface::kEnded, state_observer_->state(), kMaxWaitMs); - EXPECT_FALSE(source_->needs_denoising()); + EXPECT_EQ(rtc::Optional(), source_->needs_denoising()); } TEST_F(VideoCapturerTrackSourceTest, MixedOptionsAndConstraints) { @@ -426,7 +427,7 @@ TEST_F(VideoCapturerTrackSourceTest, MixedOptionsAndConstraints) { EXPECT_EQ(288, format->height); EXPECT_EQ(30, format->framerate()); - EXPECT_FALSE(source_->needs_denoising()); + EXPECT_EQ(rtc::Optional(false), source_->needs_denoising()); } // Tests that the source starts video with the default resolution for diff --git a/webrtc/api/videosourceproxy.h b/webrtc/api/videosourceproxy.h index ecd0e83912..fa6c428842 100644 --- a/webrtc/api/videosourceproxy.h +++ b/webrtc/api/videosourceproxy.h @@ -27,7 +27,7 @@ PROXY_METHOD0(cricket::VideoCapturer*, GetVideoCapturer) PROXY_METHOD0(void, Stop) PROXY_METHOD0(void, Restart) PROXY_CONSTMETHOD0(bool, is_screencast) -PROXY_CONSTMETHOD0(bool, needs_denoising) +PROXY_CONSTMETHOD0(rtc::Optional, needs_denoising) PROXY_METHOD2(void, AddOrUpdateSink, rtc::VideoSinkInterface*, diff --git a/webrtc/api/videotracksource.h b/webrtc/api/videotracksource.h index 78577aaba2..6d23d2e8ca 100644 --- a/webrtc/api/videotracksource.h +++ b/webrtc/api/videotracksource.h @@ -37,7 +37,8 @@ class VideoTrackSource : public Notifier { void Restart() override{}; virtual bool is_screencast() const { return false; } - virtual bool needs_denoising() const { return false; } + virtual rtc::Optional needs_denoising() const { + return rtc::Optional(); } void AddOrUpdateSink(rtc::VideoSinkInterface* sink, const rtc::VideoSinkWants& wants) override;