Skip to content

Commit

Permalink
Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending …
Browse files Browse the repository at this point in the history 
…black frame on source removal.

BUG=webrtc:6371,webrtc:6983

Review-Url: https://codereview.webrtc.org/2469993003
Cr-Commit-Position: refs/heads/master@{#16048}
  • Loading branch information
@perkj
perkj authored and Commit bot committed Jan 13, 2017
1 parent c5da08f commit d533aec
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 132 deletions.
4 changes: 1 addition & 3 deletions webrtc/media/base/videoengine_unittest.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -830,14 +830,12 @@ class VideoMediaChannelTest : public testing::Test,
rtc::Thread::Current()->ProcessMessages(30);
// Remove the capturer.
EXPECT_TRUE(channel_->SetVideoSend(kSsrc, true, nullptr, nullptr));
// Wait for one black frame for removing the capturer.
EXPECT_FRAME_WAIT(2, kVideoWidth, kVideoHeight, kTimeout);

// No capturer was added, so this SetVideoSend shouldn't do anything.
EXPECT_TRUE(channel_->SetVideoSend(kSsrc, true, nullptr, nullptr));
rtc::Thread::Current()->ProcessMessages(300);
// Verify no more frames were sent.
EXPECT_EQ(2, renderer_.num_rendered_frames());
EXPECT_EQ(1, renderer_.num_rendered_frames());
}

// Tests that we can add and remove capturer as unique sources.
Expand Down
116 changes: 22 additions & 94 deletions webrtc/media/engine/webrtcvideoengine2.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1553,8 +1553,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
parameters_(std::move(config), options, max_bitrate_bps, codec_settings),
rtp_parameters_(CreateRtpParametersWithOneEncoding()),
allocated_encoder_(nullptr, cricket::VideoCodec(), false),
sending_(false),
last_frame_timestamp_us_(0) {
sending_(false) {
parameters_.config.rtp.max_packet_size = kVideoMtu;
parameters_.conference_mode = send_params.conference_mode;

Expand Down Expand Up @@ -1612,43 +1611,6 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::~WebRtcVideoSendStream() {
DestroyVideoEncoder(&allocated_encoder_);
}

void WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame(
const webrtc::VideoFrame& frame) {
TRACE_EVENT0("webrtc", "WebRtcVideoSendStream::OnFrame");
webrtc::VideoFrame video_frame(frame.video_frame_buffer(),
frame.rotation(),
frame.timestamp_us());

rtc::CritScope cs(&lock_);

if (video_frame.width() != last_frame_info_.width ||
video_frame.height() != last_frame_info_.height ||
video_frame.rotation() != last_frame_info_.rotation ||
video_frame.is_texture() != last_frame_info_.is_texture) {
last_frame_info_.width = video_frame.width();
last_frame_info_.height = video_frame.height();
last_frame_info_.rotation = video_frame.rotation();
last_frame_info_.is_texture = video_frame.is_texture();

LOG(LS_INFO) << "Video frame parameters changed: dimensions="
<< last_frame_info_.width << "x" << last_frame_info_.height
<< ", rotation=" << last_frame_info_.rotation
<< ", texture=" << last_frame_info_.is_texture;
}

if (encoder_sink_ == NULL) {
// Frame input before send codecs are configured, dropping frame.
return;
}

last_frame_timestamp_us_ = video_frame.timestamp_us();

// Forward frame to the encoder regardless if we are sending or not. This is
// to ensure that the encoder can be reconfigured with the correct frame size
// as quickly as possible.
encoder_sink_->OnFrame(video_frame);
}

bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetVideoSend(
bool enable,
const VideoOptions* options,
Expand All @@ -1658,7 +1620,6 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetVideoSend(

// Ignore |options| pointer if |enable| is false.
bool options_present = enable && options;
bool source_changing = source_ != source;

if (options_present) {
VideoOptions old_options = parameters_.options;
Expand All @@ -1668,29 +1629,6 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetVideoSend(
}
}

if (source_changing) {
rtc::CritScope cs(&lock_);
if (source == nullptr && last_frame_info_.width > 0 && encoder_sink_) {
LOG(LS_VERBOSE) << "Disabling capturer, sending black frame.";
// Force this black frame not to be dropped due to timestamp order
// check. As IncomingCapturedFrame will drop the frame if this frame's
// timestamp is less than or equal to last frame's timestamp, it is
// necessary to give this black frame a larger timestamp than the
// previous one.
last_frame_timestamp_us_ += rtc::kNumMicrosecsPerMillisec;
rtc::scoped_refptr<webrtc::I420Buffer> black_buffer(
webrtc::I420Buffer::Create(last_frame_info_.width,
last_frame_info_.height));
webrtc::I420Buffer::SetBlack(black_buffer);

encoder_sink_->OnFrame(webrtc::VideoFrame(
black_buffer, last_frame_info_.rotation, last_frame_timestamp_us_));
}
}

// TODO(perkj, nisse): Remove |source_| and directly call
// |stream_|->SetSource(source) once the video frame types have been
// merged.
if (source_ && stream_) {
stream_->SetSource(
nullptr, webrtc::VideoSendStream::DegradationPreference::kBalanced);
Expand All @@ -1700,6 +1638,8 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetVideoSend(
if (source && stream_) {
// Do not adapt resolution for screen content as this will likely
// result in blurry and unreadable text.
// |this| acts like a VideoSource to make sure SinkWants are handled on the
// correct thread.
stream_->SetSource(
this, enable_cpu_overuse_detection_ &&
!parameters_.options.is_screencast.value_or(false)
Expand Down Expand Up @@ -1973,45 +1913,35 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetSend(bool send) {
}

void WebRtcVideoChannel2::WebRtcVideoSendStream::RemoveSink(
VideoSinkInterface<webrtc::VideoFrame>* sink) {
rtc::VideoSinkInterface<webrtc::VideoFrame>* sink) {
RTC_DCHECK_RUN_ON(&thread_checker_);
{
rtc::CritScope cs(&lock_);
RTC_DCHECK(encoder_sink_ == sink);
encoder_sink_ = nullptr;
}
source_->RemoveSink(this);
RTC_DCHECK(encoder_sink_ == sink);
encoder_sink_ = nullptr;
source_->RemoveSink(sink);
}

void WebRtcVideoChannel2::WebRtcVideoSendStream::AddOrUpdateSink(
VideoSinkInterface<webrtc::VideoFrame>* sink,
rtc::VideoSinkInterface<webrtc::VideoFrame>* sink,
const rtc::VideoSinkWants& wants) {
if (worker_thread_ == rtc::Thread::Current()) {
// AddOrUpdateSink is called on |worker_thread_| if this is the first
// registration of |sink|.
RTC_DCHECK_RUN_ON(&thread_checker_);
{
rtc::CritScope cs(&lock_);
encoder_sink_ = sink;
}
source_->AddOrUpdateSink(this, wants);
encoder_sink_ = sink;
source_->AddOrUpdateSink(encoder_sink_, wants);
} else {
// Subsequent calls to AddOrUpdateSink will happen on the encoder task
// queue.
invoker_.AsyncInvoke<void>(RTC_FROM_HERE, worker_thread_, [this, wants] {
RTC_DCHECK_RUN_ON(&thread_checker_);
bool encoder_sink_valid = true;
{
rtc::CritScope cs(&lock_);
encoder_sink_valid = encoder_sink_ != nullptr;
}
// Since |source_| is still valid after a call to RemoveSink, check if
// |encoder_sink_| is still valid to check if this call should be
// cancelled.
if (source_ && encoder_sink_valid) {
source_->AddOrUpdateSink(this, wants);
}
});
invoker_.AsyncInvoke<void>(
RTC_FROM_HERE, worker_thread_, [this, sink, wants] {
RTC_DCHECK_RUN_ON(&thread_checker_);
// |sink| may be invalidated after this task was posted since
// RemoveSink is called on the worker thread.
bool encoder_sink_valid = (sink == encoder_sink_);
if (source_ && encoder_sink_valid) {
source_->AddOrUpdateSink(encoder_sink_, wants);
}
});
}
}

Expand Down Expand Up @@ -2135,12 +2065,10 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::RecreateWebRtcStream() {
parameters_.encoder_config.encoder_specific_settings = NULL;

if (source_) {
// TODO(perkj, nisse): Remove |source_| and directly call
// |stream_|->SetSource(source) once the video frame types have been
// merged and |stream_| internally reconfigure the encoder on frame
// resolution change.
// Do not adapt resolution for screen content as this will likely result in
// blurry and unreadable text.
// |this| acts like a VideoSource to make sure SinkWants are handled on the
// correct thread.
stream_->SetSource(
this, enable_cpu_overuse_detection_ &&
!parameters_.options.is_screencast.value_or(false)
Expand Down
39 changes: 7 additions & 32 deletions webrtc/media/engine/webrtcvideoengine2.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,11 +231,9 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport {
static std::string CodecSettingsVectorToString(
const std::vector<VideoCodecSettings>& codecs);

// Wrapper for the sender part, this is where the source is connected and
// frames are then converted from cricket frames to webrtc frames.
// Wrapper for the sender part.
class WebRtcVideoSendStream
: public rtc::VideoSinkInterface<webrtc::VideoFrame>,
public rtc::VideoSourceInterface<webrtc::VideoFrame> {
: public rtc::VideoSourceInterface<webrtc::VideoFrame> {
public:
WebRtcVideoSendStream(
webrtc::Call* call,
Expand All @@ -256,14 +254,12 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport {

// Implements rtc::VideoSourceInterface<webrtc::VideoFrame>.
// WebRtcVideoSendStream acts as a source to the webrtc::VideoSendStream
// in |stream_|.
// TODO(perkj, nisse): Refactor WebRtcVideoSendStream to directly connect
// the camera input |source_|
void AddOrUpdateSink(VideoSinkInterface<webrtc::VideoFrame>* sink,
// in |stream_|. This is done to proxy VideoSinkWants from the encoder to
// the worker thread.
void AddOrUpdateSink(rtc::VideoSinkInterface<webrtc::VideoFrame>* sink,
const rtc::VideoSinkWants& wants) override;
void RemoveSink(VideoSinkInterface<webrtc::VideoFrame>* sink) override;
void RemoveSink(rtc::VideoSinkInterface<webrtc::VideoFrame>* sink) override;

void OnFrame(const webrtc::VideoFrame& frame) override;
bool SetVideoSend(bool mute,
const VideoOptions* options,
rtc::VideoSourceInterface<webrtc::VideoFrame>* source);
Expand Down Expand Up @@ -306,21 +302,6 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport {
bool external;
};

// TODO(perkj): VideoFrameInfo is currently used for sending a black frame
// when the video source is removed. Consider moving that logic to
// VieEncoder or remove it.
struct VideoFrameInfo {
VideoFrameInfo()
: width(0),
height(0),
rotation(webrtc::kVideoRotation_0),
is_texture(false) {}
int width;
int height;
webrtc::VideoRotation rotation;
bool is_texture;
};

rtc::scoped_refptr<webrtc::VideoEncoderConfig::EncoderSpecificSettings>
ConfigureVideoEncoderSettings(const VideoCodec& codec);
AllocatedEncoder CreateVideoEncoder(const VideoCodec& codec);
Expand Down Expand Up @@ -348,10 +329,9 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport {
WebRtcVideoEncoderFactory* const external_encoder_factory_
ACCESS_ON(&thread_checker_);

rtc::CriticalSection lock_;
webrtc::VideoSendStream* stream_ ACCESS_ON(&thread_checker_);
rtc::VideoSinkInterface<webrtc::VideoFrame>* encoder_sink_
GUARDED_BY(lock_);
ACCESS_ON(&thread_checker_);
// Contains settings that are the same for all streams in the MediaChannel,
// such as codecs, header extensions, and the global bitrate limit for the
// entire channel.
Expand All @@ -363,13 +343,8 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport {
// one stream per MediaChannel.
webrtc::RtpParameters rtp_parameters_ ACCESS_ON(&thread_checker_);
AllocatedEncoder allocated_encoder_ ACCESS_ON(&thread_checker_);
VideoFrameInfo last_frame_info_ GUARDED_BY(lock_);

bool sending_ ACCESS_ON(&thread_checker_);

// The timestamp of the last frame received
// Used to generate timestamp for the black frame when source is removed
int64_t last_frame_timestamp_us_ GUARDED_BY(lock_);
};

// Wrapper for the receiver part, contains configs etc. that are needed to
Expand Down
5 changes: 2 additions & 3 deletions webrtc/media/engine/webrtcvideoengine2_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1654,16 +1654,15 @@ TEST_F(WebRtcVideoChannel2Test, UsesCorrectSettingsForScreencast) {
<< "Non-screenshare shouldn't use min-transmit bitrate.";

EXPECT_TRUE(channel_->SetVideoSend(last_ssrc_, true, nullptr, nullptr));
// Removing a capturer triggers a black frame to be sent.
EXPECT_EQ(2, send_stream->GetNumberOfSwappedFrames());
EXPECT_EQ(1, send_stream->GetNumberOfSwappedFrames());
VideoOptions screencast_options;
screencast_options.is_screencast = rtc::Optional<bool>(true);
EXPECT_TRUE(
channel_->SetVideoSend(last_ssrc_, true, &screencast_options, &capturer));
EXPECT_TRUE(capturer.CaptureFrame());
// Send stream not recreated after option change.
ASSERT_EQ(send_stream, fake_call_->GetVideoSendStreams().front());
EXPECT_EQ(3, send_stream->GetNumberOfSwappedFrames());
EXPECT_EQ(2, send_stream->GetNumberOfSwappedFrames());

// Verify screencast settings.
encoder_config = send_stream->GetEncoderConfig().Copy();
Expand Down

0 comments on commit d533aec

Please sign in to comment.