Issue 46899004: Added RemoteVideoSource

Can't Edit
Can't Publish+Mail
Start Review

Created:
6 years ago by hbos

Modified:
5 years, 10 months ago

Reviewers:
magjed, tommi

CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie.mao, ajm, tterriberry, qiang.lu, Niklas

Base URL:
https://chromium.googlesource.com/external/webrtc.git@master

Target Ref:
refs/pending/heads/master

Project:
webrtc

Visibility:
Public.

More Reviews

Description

Added RemoteVideoSource, a VideoSourceInterface implementation now used (instead of VideoSource) for remote video sources. Because this is a remote video source, it gets its frame through the cricket::VideoRenderer* FrameInput(), and only needs to pass the frame on to its sinks. Unlike VideoSource, this implementation does not rely on a (Remote or otherwise)VideoCapturer for frame delivery to sinks. However, a VideoCapturer is still necessary for further sending frames through peer connection. In order to support this, a RemoteVideoCapturer is still used, but is created lazily the first time RemoteVideoSource::GetVideoCapturer() is called. RemoteVideoSource will pass along FrameInput's frames to the capturer which in turn will signal that frames have been delivered. In Chrome, where the video capturer of a remote video source is never used, this should avoid creating and registering the RemoteVideoCapturer to the CaptureManager altogether. As such, there is no risk of reference counted objects keeping remote capturers alive. The referenced chromium bug is a crash that occurs at ~CaptureManager due to all capturers not having been unregistered, specifically RemoteVideoCapturers. This happens because blink still holds references to reference counted objects despite the peer connection closing and destructing. But in chromium, there should be no need for a RemoteVideoCapturer to exist in the first place. Hence this new video source. (See my post in chromium:320200 for details.) BUG=chromium:320200

Patch Set 1 : #

Total comments: 44

Patch Set 2 : #

Total comments: 1

Created: 5 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+339 lines, -10 lines)			Patch
M	talk/app/webrtc/mediastreamsignaling.cc	View	1	5 chunks	+5 lines, -10 lines	1 comment	Download
A	talk/app/webrtc/remotevideosource.h	View	1	1 chunk	+152 lines, -0 lines	0 comments	Download
A	talk/app/webrtc/remotevideosource.cc	View	1	1 chunk	+180 lines, -0 lines	0 comments	Download
M	talk/libjingle.gyp	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download

Trybot results: mac_baremetal android_arm64_rel android android_rel win_baremetal mac_baremetal linux_baremetal linux_tsan2 win_drmemory_light linux_msan win mac mac_x64 win_rel mac_rel mac_x64_rel mac_asan linux_asan win_x64_rel linux ios32_sim ios_arm64_rel linux_rel android_gn_rel ios64_sim android_clang linux_gn mac_x64_gn_rel android_gn ios_rel ios win_x64_gn_rel ios_arm64 win_x64_gn linux_gn_rel mac_x64_gn Sign in to try more bots

Project "webrtc" does not have a commit queue.

Messages

Total messages: 6 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

hbos

perkj: Here is RemoteVideoSource we talked about. It does pass all the tests, but I'm ...

6 years ago (2015-04-14 09:41:41 UTC) #1

perkj: Here is RemoteVideoSource we talked about.

It does pass all the tests, but I'm not sure any of the tests actually use the
remove video source's VideoCapturer?

RemoteVideoSource::FrameInputImpl::RenderFrame calls the
sinks/RemoteVideoCapturer on whichever thread it is invoked on. Not sure if this
is correct or not, but it would be nice not to have to switch thread for every
single frame (and possibly have to do it sync?).

I also copy-pasted the RemoteVideoSource into my chromium repo and used it and
all content_browsertests pass (except 3 unrelated tests that fail regardless)
and ~CaptureManager's capture_states_ is empty where it was previously not.


As for the Chromium bug... while we avoid the crash due to capturers not being
deleted now that RemoteVideoCapturer is not used in Chrome, the fact remains
that blink keeps reference counted webrtc VideoTracks alive passed peer
connection tear-down. There's just not any other direct CHECK that cares about
this fact now that there are no "dangling capturers"... We're ignoring that with
this fix.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
File talk/app/webrtc/remotevideosource.cc (right):

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:39: return true;
I return true because that's what VideoSource's FrameInputWrapper was doing.
Should it not return false if it ignores the size?

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:82: state_(SourceState::kInitializing),
It should always start at kInitializing, correct? So that kInit->kLive triggers
a notification.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:113: { 1920, 1080, FPS_TO_INTERVAL(30),
cricket::FOURCC_ANY }));
It's just passing along frames so the VideoFormat shouldn't matter?
I deduced that this was probably the video format the RemoteVideoCapturer would
have ended up using in old code. But I'm just passing along something because it
is a required parameter.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:167: return &default_options_;
Again, I deduced that this was probably what the old code would have ended up
returning, even though the options are meaningless in this context. I thought
about doing DCHECK(false) << "RemoteVideoSource::options is not a supported
operation"; but that might be even more dangerous.

I thought about doing it at RemoteVideoSource::FrameInputImpl::SetSize too but
that would end up crashing so I'm not sure.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
File talk/app/webrtc/remotevideosource.h (right):

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.h:26: */
I just copy-pasted the copyright of another file. Should any specific one be
used or should I update the Copyright year?

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.h:120: SourceState state_;  // Note: only
modify using SetState.
The state_ of the old code would have reflected the state of the capturer. But
the remote video capturer was not truly used anyway so I'm guessing the state_
of the old VideoSource in the remote case could not have been very helpful
either?

Given that this class just passes along frames it is in the dark about what is
truly going on. Is it OK that we only go between kLive and kEnded based on
constructor/Stop/Restart/destructor?

Otherwise there would have to be communication between the peer connection (that
which is passing frames to frame_input_) and this class/frame_input_.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.h:128: rtc::CriticalSection lock_;
(Maybe I should add a comment next to each of these variables saying they're
guarded by lock_ unless that's overly verbose)

tommi

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/remotevideosource.cc File talk/app/webrtc/remotevideosource.cc (right): https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/remotevideosource.cc#newcode3 talk/app/webrtc/remotevideosource.cc:3: * Copyright 2012 Google Inc. fix year here too ...

6 years ago (2015-04-23 15:50:58 UTC) #2

hbos

PTAL https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/remotevideosource.cc File talk/app/webrtc/remotevideosource.cc (right): https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/remotevideosource.cc#newcode3 talk/app/webrtc/remotevideosource.cc:3: * Copyright 2012 Google Inc. On 2015/04/23 15:50:58, ...

5 years, 11 months ago (2015-05-18 14:32:20 UTC) #5

PTAL

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
File talk/app/webrtc/remotevideosource.cc (right):

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:3: * Copyright 2012 Google Inc.
On 2015/04/23 15:50:58, tommi wrote:
> fix year here too

Acknowledged.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:45: rtc::CritScope cs(&master_->lock_);
On 2015/04/23 15:50:57, tommi wrote:
> let's not touch member variables of other classes here.  Instead, let's call a
> method on the master_ object and use member variables there.

Acknowledged. (Even though it's a private nested helper class necessarily
tightly coupled with RemoteVideoSource?)

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:54: = master_->sinks_.begin(); it !=
master_->sinks_.end(); ++it) {
On 2015/04/23 15:50:57, tommi wrote:
> range based loops

Acknowledged.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:66:
master_->output_capturer_->SignalVideoFrame(
On 2015/04/23 15:50:58, tommi wrote:
> can we avoid making this call while holding the lock?

We can but the lock is rarely held due to the class being largely used
single-threadedly for public methods. Only when a write operation occur.
I doubt the performance benefits are measurable but I moved it out of the
critical section just in case by using a local variable.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:73: rtc::scoped_refptr<RemoteVideoSource>
RemoteVideoSource::Create() {
On 2015/04/23 15:50:57, tommi wrote:
> what about not having this function and just allocate RemoteVideoSource()
> directly where we need it? or is there some benefit in having the factory
> function?

It's similar to VideoSource which also has a factory function, same "pattern". I
believe I've seen similar static Create() functions in several places of
existing code.

I do find our ref counting classes to be confusing the first time you look at
them. If you try to do "new RemoteVideoSource()" you'll be told you can't
because it's an abstract class, a true but somewhat misleading statement since
in all other respects it is fully implemented. The proper way to instantiate it
is of course to wrap it in a RefCountedObject<RemoteVideoSource>, but this is
only apparent if you've come across RefCountedInterface implementations before
and know of the existence of RefCountedObject.

Benefit: easy helper for instantiation that can be reused in case we want to
create it in multiple places. Downside: four lines of otherwise "unnecessary"
factory code.

Anyway that was a long reply. If you still think I should remove it I will.
magjed might agree I should remove it.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:94: if (output_capturer_ == nullptr) {
On 2015/04/23 15:50:58, tommi wrote:
> this block strikes me as a bit weird... do we expect that the object being
> deleted might be in use on other threads?

Hah, oops, no this is just as utterly pointless as it looks.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:111: cricket::VideoFormat video_format(
On 2015/04/23 15:50:57, tommi wrote:
> we can construct this before grabbing the lock

Acknowledged.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:116: CHECK(false) << "Unable to start
lazily created RemoteVideoCapturer.";
On 2015/04/23 15:50:58, tommi wrote:
> do we need to delete the capturer first?  why not just do:
> 
> CHECK(output_capturer_->StartCapturing(video_format));

Acknowledged.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:142: DCHECK(false) << "|output| is already
a sink to this source.";
On 2015/04/23 15:50:57, tommi wrote:
> if we're DCHECKing, then that's equivalent of 'this should never happen'.  So
in
> that case, the code should be:
> 
> DCHECK(std::find(sinks_.begin(), sinks_.end(), output) == sinks_.end());
> sinks_.push_back(output);

Acknowledged.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.cc:150: it != sinks_.end(); ++it) {
On 2015/04/23 15:50:58, tommi wrote:
> C++ 11 range loops... actually could we use erase(remove_if())?
> http://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom

(I would have preferred the range-based for loop for readability but that only
works when you're iterating the values and not the iterator, right?)

Yeah we can use the erase-remove (we end up iterating until the end of the list
even if the renderer is found early though).

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
File talk/app/webrtc/remotevideosource.h (right):

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.h:26: */
On 2015/04/23 15:50:58, tommi wrote:
> On 2015/04/14 09:41:41, hbos wrote:
> > I just copy-pasted the copyright of another file. Should any specific one be
> > used or should I update the Copyright year?
> 
> year needs to be fixed

Acknowledged.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.h:33: #include
"talk/media/base/mediachannel.h"
On 2015/04/23 15:50:58, tommi wrote:
> alphabetical order

Acknowledged.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.h:51: public sigslot::has_slots<> {
On 2015/04/23 15:50:58, tommi wrote:
> is sigslot necessary?  If so, is the single threaded implementation the right
> one?

I'm not sure what is best. I made it has_slots because that's what VideoSource
is and I thought RemoteVideoSource should be used the same way. If VideoSource
is used correctly as a single-threaded has_slots then this should as well.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.h:65: void AddSink(cricket::VideoRenderer*
output) override;
On 2015/04/23 15:50:58, tommi wrote:
> s/output/renderer

Acknowledged.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.h:68: // See |state_|.
On 2015/04/23 15:50:58, tommi wrote:
> nit: the documentation of the public interface and implementation detail isn't
> always the same, so it might be good to document the state here.  Also, I'm
> assuming this is only safe to call on one specific thread, right?

Acknowledged. All public methods must be invoked on the same thread so I added a
comment about that to the class instead of here.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.h:72: // a default set of options.
On 2015/04/23 15:50:58, tommi wrote:
> what about ownership of the returned object?  Is it owned by
RemoteVideoSource? 
> If so, I'd argue that this should be |const cricket::VideoOptions&| (I know
that
> would be an interface change).  If not, then I'd like to change the return
value
> to be a scoped_ptr<cricket::VideoOptions>. That is, unless those are huge
> changes.

Just like with VideoSource, the RemoteVideoSource owns the returned object. I
could change the interface if you like but I would prefer if any interface
changes are kept to separate CLs.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.h:78: cricket::VideoRenderer* FrameInput()
override;
On 2015/04/23 15:50:58, tommi wrote:
> can the return value be null?  I don't suppose it could be const?

No, |frame_input_| is not nullable.
Again, I could look into the possibility of an interface change in a separate
CL.

https://webrtc-codereview.appspot.com/46899004/diff/20001/talk/app/webrtc/rem...
talk/app/webrtc/remotevideosource.h:128: rtc::CriticalSection lock_;
On 2015/04/23 15:50:58, tommi wrote:
> On 2015/04/14 09:41:41, hbos wrote:
> > (Maybe I should add a comment next to each of these variables saying they're
> > guarded by lock_ unless that's overly verbose)
> 
> sure - thread annotations?

You mean GUARDED_BY? That doesn't work because I take advantage of the fact that
write only occurs on a single thread and don't always acquire the lock, so it
complains if I use GUARDED_BY.
(sinks_ is an exception but I think it's confusing if some of the variables that
are guarded have GUARDED_BY and not others)

https://webrtc-codereview.appspot.com/46899004/diff/40001/talk/app/webrtc/med...
File talk/app/webrtc/mediastreamsignaling.cc (right):

https://webrtc-codereview.appspot.com/46899004/diff/40001/talk/app/webrtc/med...
talk/app/webrtc/mediastreamsignaling.cc:219: cricket::ChannelManager*
channel_manager)
Unused variable..

hbos

5 years, 10 months ago (2015-06-12 13:12:19 UTC) #6

http://stream1.gifsoup.com/view2/2054482/single-tumbleweed-o.gif

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages