Replace SetCapturer and SetCaptureDevice by SetSource.

Replace SetCapturer and SetCaptureDevice by SetSource.
Drop return value.

BUG=webrtc:5426
Committed: https://crrev.com/2ded9b19d1cb2ad4cf4d69c34094fbddc3809888
Cr-Commit-Position: refs/heads/master@{#12291}

[nisse-webrtc, 2016-03-04 09:51:53]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-04 09:52:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1766653002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1766653002/1

[commit-bot: I haz the power, 2016-03-04 09:55:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-04 09:55:30]

Dry run: Try jobs failed on following builders:
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/11435)
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5675)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/8012)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7927)
  linux_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
  linux_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
  linux_ubsan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/243)
  mac_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/9556)
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/12939)

[nisse-webrtc, 2016-03-04 10:06:07]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-04 10:06:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1766653002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1766653002/20001

[commit-bot: I haz the power, 2016-03-04 10:17:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-04 10:17:44]

Dry run: Try jobs failed on following builders:
  linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)

[nisse-webrtc, 2016-03-04 10:36:46]

nisse@webrtc.org changed reviewers:
+ perkj@webrtc.org, pthatcher@webrtc.org

[nisse-webrtc, 2016-03-04 10:36:46]

Here's a first attempt at deleting SetCapturer.

Stats are disabled (I'm curious if there are any tests to notice that; I
couldn't find any).

CPU adaptation and propagation of SinkWants likely broken too, I'm lost track of
what the current state is there. Again, I'm curious if there are any test at
high-enough level to catch that.

[nisse-webrtc, 2016-03-04 12:32:08]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-04 12:32:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1766653002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1766653002/40001

[commit-bot: I haz the power, 2016-03-04 14:33:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-04 14:33:05]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[pthatcher1, 2016-03-05 01:45:41]

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1282:
send_streams_.find(ssrc)->second->SetSource(source);
Why not kv->second->SetSource(source)?

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1555: // TODO(nisse): Keep?
Yes, we need to keep this.  It's part of the standard to black-out frames for a
disabled VideoTrack.

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1616: source_->AddOrUpdateSink(this,
sink_wants_);
{} please

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2001: #endif
What?

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/pc/channel.h#newco...
webrtc/pc/channel.h:448: // send stream.
We should specify what happens if it isn't (ie nothing, except a debug build
will explode)

[nisse-webrtc, 2016-03-15 16:28:00]

("done" means in my local copy, no update to the cl today).

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1282:
send_streams_.find(ssrc)->second->SetSource(source);
On 2016/03/05 01:45:41, pthatcher1 wrote:
> Why not kv->second->SetSource(source)?

Done.

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1555: // TODO(nisse): Keep?
On 2016/03/05 01:45:41, pthatcher1 wrote:
> Yes, we need to keep this.  It's part of the standard to black-out frames for
a
> disabled VideoTrack.

A while ago, I added similar logic in the VideoTrack class, so that it could be
removed here. My gut feeling is that the enabled_ flag makes sense in a
VideoTrack, but that the muted_ state doesn't really belong here. I'd like to
delete it, together with the |enable| argument for SetVideoSend.

And more recently, we've been discussing pushing black frames back to the source
with another VideoSinkWants flag (one potential advantage is that a source
producing textures might be able to produce black textures, and not switch
between textures and ordinary frames in main memory).

So we have to keep the logic here for now, but I really don't think this is the
right place to produce black frames.

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1616: source_->AddOrUpdateSink(this,
sink_wants_);
On 2016/03/05 01:45:41, pthatcher1 wrote:
> {} please

Done.

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2001: #endif
On 2016/03/05 01:45:41, pthatcher1 wrote:
> What?

To get the cl in a landable state, we have to figure out how to get the stats;
this class shouldn't know about the capturer. I don't know yet what's the right
way to do that.

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/pc/channel.h#newco...
webrtc/pc/channel.h:448: // send stream.
On 2016/03/05 01:45:41, pthatcher1 wrote:
> We should specify what happens if it isn't (ie nothing, except a debug build
> will explode)

The current implementation will always explode, except if source is NULL. I
don't quite like the exception, but it seemed hard to avoid (I think I described
the problem a week or two ago).

And do we really need to document in which way things break in case a caller
violates the documented interface?

[pthatcher1, 2016-03-15 16:56:46]

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1555: // TODO(nisse): Keep?
Yes, I saw that CL while Per was here.  If the VideoTrack does it via
wants.black_frames, then we don't need it here.  We can remove this once that CL
has landed.



On 2016/03/15 16:28:00, nisse-webrtc wrote:
> On 2016/03/05 01:45:41, pthatcher1 wrote:
> > Yes, we need to keep this.  It's part of the standard to black-out frames
for
> a
> > disabled VideoTrack.
> 
> A while ago, I added similar logic in the VideoTrack class, so that it could
be
> removed here. My gut feeling is that the enabled_ flag makes sense in a
> VideoTrack, but that the muted_ state doesn't really belong here. I'd like to
> delete it, together with the |enable| argument for SetVideoSend.
> 
> And more recently, we've been discussing pushing black frames back to the
source
> with another VideoSinkWants flag (one potential advantage is that a source
> producing textures might be able to produce black textures, and not switch
> between textures and ordinary frames in main memory).
> 
> So we have to keep the logic here for now, but I really don't think this is
the
> right place to produce black frames.

https://codereview.webrtc.org/1766653002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2001: #endif
On 2016/03/15 16:28:00, nisse-webrtc wrote:
> On 2016/03/05 01:45:41, pthatcher1 wrote:
> > What?
> 
> To get the cl in a landable state, we have to figure out how to get the stats;
> this class shouldn't know about the capturer. I don't know yet what's the
right
> way to do that.

adapt_frame_drops, effects_frame_drops, and capturer_frame_time are never used
by anything, as far as I can tell.

The input_frame sizes is used.  I guess it's the non-adapted frame size.  We
could get that by having the unscaled resolution on the VideoFrame, which is
something I talked to Per about while he was here.

[nisse-webrtc, 2016-03-17 15:16:46]

In patchset 6, I had to temporarily add in cl 1790633002, since otherwise tests
fail because there are VideoTracks with a NULL source. The size of this cl
should shrink once that is landed.

The interesting changes are:

* VideoRtpSender now passes the track itself to provider_->SetSource, without
calling GetSource()->GetVideoCapturer()

* Mute logic is deleted from the video engine.

* For stats, I had to add a GetInfo method to the VideoSourceInterface. Not sure
this is the best way, but it makes some sense to have it there.

Questions: Are the stats called "googFrameHeightInput" and "googFrameWidthInput"
ever used?

The logic to send a black frame when a source is removed, is that needed? The
purpose of that feature seems unclear.

The objc class RTCAVFoundationVideoSource defines a capturer method, which calls
GetVideoCapturer. Is that ever used? It's not declared in the header file, as
far as I can see. I'd really like to kill GetVideoCapturer.
We hope that this change also restores relaying of video streams, but not yet
tested.

[perkj_webrtc, 2016-03-20 15:01:46]

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/api/videotracksou...
File webrtc/api/videotracksource.cc (right):

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/api/videotracksou...
webrtc/api/videotracksource.cc:62: source_->GetInfo(info);
Humm. This looks like a potential race with OnSourceDestroyed. 
Actually - you call GetInfo on the worker thread from webrtcvideoengine2.
Everything else in this class is called on the signaling thread.

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/media/base/videos...
File webrtc/media/base/videosourcebase.cc (right):

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/media/base/videos...
webrtc/media/base/videosourcebase.cc:47: void
VideoSourceBase::GetInfo(VideoSourceInfo *info) {
Can you leave this as a pure virtual?

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/media/base/videos...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/media/base/videos...
webrtc/media/base/videosourceinterface.h:53: virtual void
GetInfo(VideoSourceInfo *info) = 0;
So we have the option of adding some kind of stats API here. It might make sense
to me but if there are other options lets discuss them. 

Another option is to have the original size in the frame. But that does not make
sense to me in the case when a frame has already been scaled. Ie, it does not
make sence to have information about an original frame size when the memory used
by the actual frame buffer do not have the original size. Ie, say that two sinks
wants the same frame size and therefore the frame is scaled in the source. In
that case, the stats API should probably still say that the original size is the
size the capturer runs at. Also - a capturer may choose to only color convert as
much as is needed by a sink, that may be less than the original size.

A third option - why bother at all? Why not just store the frame size first seen
in webrtcvideoengine2::sendstream before any CPU adaptation or bandwidth
estimation kicks in? That would of course not give you the correct size if the
capturer actually reconfigure itself but that currently does not happen. I think
I vote for this option because it is the simplest.

[nisse-webrtc, 2016-03-21 09:18:50]

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/media/base/videos...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/media/base/videos...
webrtc/media/base/videosourceinterface.h:53: virtual void
GetInfo(VideoSourceInfo *info) = 0;
On 2016/03/20 15:01:46, perkj_webrtc wrote:
> So we have the option of adding some kind of stats API here. It might make
sense
> to me but if there are other options lets discuss them. 
> 
> Another option is to have the original size in the frame. But that does not
make
> sense to me in the case when a frame has already been scaled. Ie, it does not
> make sence to have information about an original frame size when the memory
used
> by the actual frame buffer do not have the original size. Ie, say that two
sinks
> wants the same frame size and therefore the frame is scaled in the source. In
> that case, the stats API should probably still say that the original size is
the
> size the capturer runs at. Also - a capturer may choose to only color convert
as
> much as is needed by a sink, that may be less than the original size.
> 
> A third option - why bother at all? Why not just store the frame size first
seen
> in webrtcvideoengine2::sendstream before any CPU adaptation or bandwidth
> estimation kicks in? That would of course not give you the correct size if the
> capturer actually reconfigure itself but that currently does not happen. I
think
> I vote for this option because it is the simplest.

I've tried the third approach now. I like the simplicity.

The input_frame_width and _height now gets the size of the first frame seen
after SetSource. I guess this won't be totally correct in all cases (e.g., set a
source, have it undergo some adaptation, switch to a different source, then
switch back to the first one). But maybe it's good enough for now? And there
seems to be no tests for this feature.

[nisse-webrtc, 2016-03-21 09:19:47]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-21 09:19:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1766653002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1766653002/120001

[commit-bot: I haz the power, 2016-03-21 11:50:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-21 11:50:56]

Dry run: Try jobs failed on following builders:
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/11923)

[nisse-webrtc, 2016-03-21 13:12:06]

nisse@webrtc.org changed reviewers:
+ pbos@webrtc.org

[nisse-webrtc, 2016-03-21 13:12:07]

Adding pbos, who didn't quite like the latest statistics hack. So what should we
do?

Can you all live with adding original size as properties in the frame object
instead?

[perkj_webrtc, 2016-03-21 15:58:14]

On 2016/03/21 13:12:07, nisse-webrtc wrote:
> Adding pbos, who didn't quite like the latest statistics hack. So what should
we
> do?
> 
> Can you all live with adding original size as properties in the frame object
> instead?

rebase?

[perkj_webrtc, 2016-03-22 08:56:36]

I think this looks good. But can you remove VideoSourceInteface::GetCapturer in
this cl?

https://codereview.webrtc.org/1766653002/diff/180001/webrtc/api/mediastreamin...
File webrtc/api/mediastreaminterface.h (right):

https://codereview.webrtc.org/1766653002/diff/180001/webrtc/api/mediastreamin...
webrtc/api/mediastreaminterface.h:136: virtual cricket::VideoCapturer*
GetVideoCapturer() = 0;
Remove in this cl please! (if you can)

https://codereview.webrtc.org/1766653002/diff/180001/webrtc/media/base/videoc...
File webrtc/media/base/videocapturer.cc (right):

https://codereview.webrtc.org/1766653002/diff/180001/webrtc/media/base/videoc...
webrtc/media/base/videocapturer.cc:538: void VideoCapturer::UpdateStats(const
CapturedFrame* captured_frame) {
Remove?

https://codereview.webrtc.org/1766653002/diff/180001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1766653002/diff/180001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1331: RTC_CHECK(ssrc != 0);
nit DCHECK

[nisse-webrtc, 2016-03-22 10:18:59]

GetVideoCapturer now gone!

If you can now only agree on how to do the pre-adaptation stats, we can get
ready to land this. To recap, alternatives are:

1. Current hack, using the dimension of the first frame seen after SetSource.

2. A GetInfo method in the source interface. Was sort-of implemented in an
earlier patch set, but that implementation had some threading issues.

3. Add original size as attributes of cricket::VideoFrame.

https://codereview.webrtc.org/1766653002/diff/180001/webrtc/media/base/videoc...
File webrtc/media/base/videocapturer.cc (right):

https://codereview.webrtc.org/1766653002/diff/180001/webrtc/media/base/videoc...
webrtc/media/base/videocapturer.cc:538: void VideoCapturer::UpdateStats(const
CapturedFrame* captured_frame) {
On 2016/03/22 08:56:36, perkj_webrtc wrote:
> Remove?

Done, deleted together with last_capturered_frame_format_.

https://codereview.webrtc.org/1766653002/diff/180001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1766653002/diff/180001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1331: RTC_CHECK(ssrc != 0);
On 2016/03/22 08:56:36, perkj_webrtc wrote:
> nit DCHECK

Done.

[pbos-webrtc, 2016-03-22 13:50:22]

On 2016/03/22 10:18:59, nisse-webrtc wrote:
> GetVideoCapturer now gone!
> 
> If you can now only agree on how to do the pre-adaptation stats, we can get
> ready to land this. To recap, alternatives are:
> 
> 1. Current hack, using the dimension of the first frame seen after SetSource.
> 
> 2. A GetInfo method in the source interface. Was sort-of implemented in an
> earlier patch set, but that implementation had some threading issues.
> 
> 3. Add original size as attributes of cricket::VideoFrame.
> 

Personally I'd 2 or 3 and I don't find 1 acceptable. I worrying that 3 because
that sounds likely to bleed all over the code, but I'm also worried that 2 would
bleed over all sources' code.

Do you prefer any of these two?

[pbos-webrtc, 2016-03-22 13:50:48]

pbos@webrtc.org changed reviewers:
+ magjed@webrtc.org

[pbos-webrtc, 2016-03-22 13:50:49]

Adding magjed@ because I'd like his opinion on options 2 & 3.

[magjed_webrtc, 2016-03-22 13:57:35]

On 2016/03/22 13:50:49, pbos-webrtc wrote:
> Adding magjed@ because I'd like his opinion on options 2 & 3.

I prefer option 2, because I don't like unnecessary attributes in the VideoFrame
classes, even if it's a convenient way of passing information.

[perkj_webrtc, 2016-03-22 14:00:41]

perkj@webrtc.org changed reviewers:
- magjed@webrtc.org

[perkj_webrtc, 2016-03-22 14:00:42]

I think this is the right approach and its simple. 

An original frame size field on a frame does not make sense to me here and would
only be usefull for statistics. The frame itself will not be of size  "original
size" since we would only color convert to the destination size in the video
frame factory. 


lgtm

https://codereview.webrtc.org/1766653002/diff/200001/webrtc/media/base/videoc...
File webrtc/media/base/videocapturer.h (right):

https://codereview.webrtc.org/1766653002/diff/200001/webrtc/media/base/videoc...
webrtc/media/base/videocapturer.h:308: rtc::CriticalSection frame_stats_crit_;
remove?

[nisse-webrtc, 2016-03-22 14:14:26]

Description was changed from

==========
Replace SetCapturer and SetCaptureDevice by SetSource.
Drop return value.

Propagation of SinkWants not yet working.

BUG=webrtc:5426
==========

to

==========
Replace SetCapturer and SetCaptureDevice by SetSource.
Drop return value.

BUG=webrtc:5426
==========

[nisse-webrtc, 2016-03-22 14:16:50]

@pthatcher: I'd really like to hear your opinion on how to get the
pre-adaptation frame size into the stats (I asked mflodman the other day, and it
seems the hangouts folks want us to keep it). A source GetInfo method, or
attributes on each video frame?

https://codereview.webrtc.org/1766653002/diff/200001/webrtc/media/base/videoc...
File webrtc/media/base/videocapturer.h (right):

https://codereview.webrtc.org/1766653002/diff/200001/webrtc/media/base/videoc...
webrtc/media/base/videocapturer.h:308: rtc::CriticalSection frame_stats_crit_;
On 2016/03/22 14:00:42, perkj_webrtc wrote:
> remove?

Done.

[nisse-webrtc, 2016-03-22 14:24:32]

On 2016/03/22 13:57:35, magjed_webrtc wrote:
> On 2016/03/22 13:50:49, pbos-webrtc wrote:
> > Adding magjed@ because I'd like his opinion on options 2 & 3.
> 
> I prefer option 2, because I don't like unnecessary attributes in the
VideoFrame
> classes, even if it's a convenient way of passing information.

I'm a bit worried that (2) is going to cause threading complications (Per
commented on the race between GetInfo and OnSourceDestroyed). Maybe it's easy,
but if we need extra locking that seems a bit heavy, when all we really want it
to get two ints into the stats.

(3) has no complications like that, and if we're moving towards lazy scaling,
maybe we're going to have original size in the frames in any case?

[pthatcher1, 2016-03-22 21:42:27]

On 2016/03/22 14:24:32, nisse-webrtc wrote:
> On 2016/03/22 13:57:35, magjed_webrtc wrote:
> > On 2016/03/22 13:50:49, pbos-webrtc wrote:
> > > Adding magjed@ because I'd like his opinion on options 2 & 3.
> > 
> > I prefer option 2, because I don't like unnecessary attributes in the
> VideoFrame
> > classes, even if it's a convenient way of passing information.
> 
> I'm a bit worried that (2) is going to cause threading complications (Per
> commented on the race between GetInfo and OnSourceDestroyed). Maybe it's easy,
> but if we need extra locking that seems a bit heavy, when all we really want
it
> to get two ints into the stats.
> 
> (3) has no complications like that, and if we're moving towards lazy scaling,
> maybe we're going to have original size in the frames in any case?

I like #3.  I can see having information about the original frame being useful
for other things, and I think it fits in well with lazy scaling (where we may
want the lazy scale to be relative to the original size, not the current size).

[pbos-webrtc, 2016-03-22 21:45:22]

On 2016/03/22 21:42:27, pthatcher1 wrote:
> On 2016/03/22 14:24:32, nisse-webrtc wrote:
> > On 2016/03/22 13:57:35, magjed_webrtc wrote:
> > > On 2016/03/22 13:50:49, pbos-webrtc wrote:
> > > > Adding magjed@ because I'd like his opinion on options 2 & 3.
> > > 
> > > I prefer option 2, because I don't like unnecessary attributes in the
> > VideoFrame
> > > classes, even if it's a convenient way of passing information.
> > 
> > I'm a bit worried that (2) is going to cause threading complications (Per
> > commented on the race between GetInfo and OnSourceDestroyed). Maybe it's
easy,
> > but if we need extra locking that seems a bit heavy, when all we really want
> it
> > to get two ints into the stats.
> > 
> > (3) has no complications like that, and if we're moving towards lazy
scaling,
> > maybe we're going to have original size in the frames in any case?
> 
> I like #3.  I can see having information about the original frame being useful
> for other things, and I think it fits in well with lazy scaling (where we may
> want the lazy scale to be relative to the original size, not the current
size).

That must mean that we're storing both original frame size and current frame
size (if we're ever allowed to apply the scaling to the frame), btw, to not
discard information in later steps.

What other things would it be useful for do you think?

[pthatcher1, 2016-03-22 21:47:11]

lgtm

Even though I prefer #3, if this hack works for now, go ahead and submit it and
do #3 later.

https://codereview.webrtc.org/1766653002/diff/220001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1766653002/diff/220001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1001: // has been moved elsewhere.
In that case, you can just remove this whole method and use SetOptions instead.

https://codereview.webrtc.org/1766653002/diff/220001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1339: else {
} else {

[nisse-webrtc, 2016-03-23 08:28:11]

New patch set, with only formatting tweaks and updated TODO comments.

pbos, do you insist that we do the pre-adaptation stats properly in this cl (and
I think "properly" means option 3, adding attributes to cricket::VideoFrame), or
is it ok to do that later?

https://codereview.webrtc.org/1766653002/diff/220001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1766653002/diff/220001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1001: // has been moved elsewhere.
On 2016/03/22 21:47:11, pthatcher1 wrote:
> In that case, you can just remove this whole method and use SetOptions
instead.

Good idea, but I think I'd prefer to do that in a follow up cl, it's going to be
a pretty large diff. It should be deleted all the way up to
VideoProviderInterface.

https://codereview.webrtc.org/1766653002/diff/220001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1339: else {
On 2016/03/22 21:47:11, pthatcher1 wrote:
> } else {
> 

Done.

[pbos-webrtc, 2016-03-23 12:45:23]

https://codereview.webrtc.org/1766653002/diff/240001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1766653002/diff/240001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1657: source_->AddOrUpdateSink(this,
sink_wants_);
This causes the current code to be incorrect (since changing a source on
something that has sink_wants_ will cause adaptation on the first frame).

[nisse-webrtc, 2016-03-23 14:07:03]

After discussion with pbos, I think I'll go back to the GetInfo approach in
patchset 6 (aka option 2). Advice on the threading issues is appreciated.

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/api/videotracksou...
File webrtc/api/videotracksource.cc (right):

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/api/videotracksou...
webrtc/api/videotracksource.cc:62: source_->GetInfo(info);
On 2016/03/20 15:01:46, perkj_webrtc wrote:
> Humm. This looks like a potential race with OnSourceDestroyed. 
> Actually - you call GetInfo on the worker thread from webrtcvideoengine2.
> Everything else in this class is called on the signaling thread. 
> 

I'd need some guidance on how to get the threading right. As for
OnSourceDestroyed, it seems like calling GetInfo at the same time (or after) the
source is destroyed is a plain use-after-free. It should be invalid to destroy a
source while it is registered with VideoMediaChannel->SetSource, right?

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/media/base/videos...
File webrtc/media/base/videosourcebase.cc (right):

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/media/base/videos...
webrtc/media/base/videosourcebase.cc:47: void
VideoSourceBase::GetInfo(VideoSourceInfo *info) {
On 2016/03/20 15:01:46, perkj_webrtc wrote:
> Can you leave this as a pure virtual?

I could, but then it needs to be implemented in VideoBroadcaster instead, since
that class is instantiated as a member of VideoCapturer and VideoRtpReceiver
(none of which wants a real implementation). The implementation which matters is
the one in VideoCapturer itself.

[perkj_webrtc, 2016-03-24 11:02:50]

It feels to me that  getting the stats for what resolution the capturer is
actually opened  with  (VideoSenderInfo.input_frame_width) has nothing to do
with a PeerConnection or a WebRtcVideoSendStream.  It belongs more in video
source implementation. But the WebRtcVideoSendStream should not even ask for
it.. 

So instead of adding a stats method on rtc::VideoSourceInterface - can we add it
on webrtc::VideoTrackSource and get it directly in the statscolllector?

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/api/videotracksou...
File webrtc/api/videotracksource.cc (right):

https://codereview.webrtc.org/1766653002/diff/100001/webrtc/api/videotracksou...
webrtc/api/videotracksource.cc:62: source_->GetInfo(info);
On 2016/03/23 14:07:02, nisse-webrtc wrote:
> On 2016/03/20 15:01:46, perkj_webrtc wrote:
> > Humm. This looks like a potential race with OnSourceDestroyed. 
> > Actually - you call GetInfo on the worker thread from webrtcvideoengine2.
> > Everything else in this class is called on the signaling thread. 
> > 
> 
> I'd need some guidance on how to get the threading right. As for
> OnSourceDestroyed, it seems like calling GetInfo at the same time (or after)
the
> source is destroyed is a plain use-after-free. It should be invalid to destroy
a
> source while it is registered with VideoMediaChannel->SetSource, right?

That is correct. And should in practice never happen since it means we have a
bug else where. Maybe we can just document that somehow.

https://codereview.webrtc.org/1766653002/diff/240001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1766653002/diff/240001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1657: source_->AddOrUpdateSink(this,
sink_wants_);
On 2016/03/23 12:45:23, pbos-webrtc wrote:
> This causes the current code to be incorrect (since changing a source on
> something that has sink_wants_ will cause adaptation on the first frame).

Is your concern just input_dimensions_? 

I realise we already have introduced a bug here since if you switch to screen
share- sink_wants will contain old wants that screen share is supposed to
ignore..

https://codereview.webrtc.org/1766653002/diff/240001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1937: // cpu adaptation. The proper
solution is to add attributes to the
I am not sure that is a proper solution either since we only want to do color
conversion up to the frame size the CPU adapter tell us to.

[nisse-webrtc, 2016-03-24 11:36:33]

On 2016/03/24 11:02:50, perkj_webrtc wrote:
> So instead of adding a stats method on rtc::VideoSourceInterface - can we add
it
> on webrtc::VideoTrackSource and get it directly in the statscolllector?

Makes sense, question is what "directly" means... 

Given a peerconnection, how do I get to the relevant videotracks? Using the
local_streams() method, I guess. I can give it a try.

[nisse-webrtc, 2016-03-24 12:48:33]

On 2016/03/24 11:36:33, nisse-webrtc wrote:
> On 2016/03/24 11:02:50, perkj_webrtc wrote:
> > So instead of adding a stats method on rtc::VideoSourceInterface - can we
add
> it
> > on webrtc::VideoTrackSource and get it directly in the statscolllector?
> 
> Makes sense, question is what "directly" means... 
> 
> Given a peerconnection, how do I get to the relevant videotracks? Using the
> local_streams() method, I guess. I can give it a try.

See cl https://codereview.webrtc.org/1827023002/

I think it's a sane approach, but it seems a bit non-trivial to get values
properly grouped.

[pthatcher1, 2016-03-24 19:08:43]

On 2016/03/24 12:48:33, nisse-webrtc wrote:
> On 2016/03/24 11:36:33, nisse-webrtc wrote:
> > On 2016/03/24 11:02:50, perkj_webrtc wrote:
> > > So instead of adding a stats method on rtc::VideoSourceInterface - can we
> add
> > it
> > > on webrtc::VideoTrackSource and get it directly in the statscolllector?
> > 
> > Makes sense, question is what "directly" means... 
> > 
> > Given a peerconnection, how do I get to the relevant videotracks? Using the
> > local_streams() method, I guess. I can give it a try.
> 
> See cl https://codereview.webrtc.org/1827023002/
> 
> I think it's a sane approach, but it seems a bit non-trivial to get values
> properly grouped.

I like that CL.  But I think it could be improved a bit.  Comments in the CL.

[pbos-webrtc, 2016-03-31 14:14:31]

overall LG, I wanna see this rebased on top of the other stats CL first as this
is currently broken for stats in special cases

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/api/rtpsender.cc
File webrtc/api/rtpsender.cc (right):

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/api/rtpsender.cc#...
webrtc/api/rtpsender.cc:281: // to slip out. Really, what we need is for
SetSource and
Can this fit on one line now?

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/base/mediac...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:997: // Register a source. The |ssrc| must
correspond to a registered
reformat comment (send fits on previous line)

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1002: // been moved elsewhere. So
delete this method, and use SetOptions
say where they moved please

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1638: input_dimensions_.width =
input_dimensions_.height = 0;
Rebase on top of other CL when it lands so this hack goes away

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1657: source_ = NULL;
nullptr

[nisse-webrtc, 2016-04-01 09:35:39]

Now rebased on top of the stats changes which were landed this morning.

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/api/rtpsender.cc
File webrtc/api/rtpsender.cc (right):

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/api/rtpsender.cc#...
webrtc/api/rtpsender.cc:281: // to slip out. Really, what we need is for
SetSource and
On 2016/03/31 14:14:31, pbos-webrtc wrote:
> Can this fit on one line now?

Done.

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/base/mediac...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:997: // Register a source. The |ssrc| must
correspond to a registered
On 2016/03/31 14:14:31, pbos-webrtc wrote:
> reformat comment (send fits on previous line)

Done. You use fill-column 80?

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1638: input_dimensions_.width =
input_dimensions_.height = 0;
On 2016/03/31 14:14:31, pbos-webrtc wrote:
> Rebase on top of other CL when it lands so this hack goes away

Sure.

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1657: source_ = NULL;
On 2016/03/31 14:14:31, pbos-webrtc wrote:
> nullptr

Done. But it's really inconsistent in this file.

[nisse-webrtc, 2016-04-01 09:35:51]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-01 09:36:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1766653002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1766653002/300001

[commit-bot: I haz the power, 2016-04-01 09:41:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-01 09:41:52]

Dry run: Try jobs failed on following builders:
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/12551)

[pbos-webrtc, 2016-04-01 10:55:28]

lgtm, thanks!

I trust you'll take care of the remaining compile failures

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/base/mediac...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:997: // Register a source. The |ssrc| must
correspond to a registered
On 2016/04/01 09:35:39, nisse-webrtc wrote:
> On 2016/03/31 14:14:31, pbos-webrtc wrote:
> > reformat comment (send fits on previous line)
> 
> Done. You use fill-column 80?

Not sure what that is in vim speak, we have 80 characters per line allowed (col
81 wraps)

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1766653002/diff/260001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1657: source_ = NULL;
On 2016/04/01 09:35:39, nisse-webrtc wrote:
> On 2016/03/31 14:14:31, pbos-webrtc wrote:
> > nullptr
> 
> Done. But it's really inconsistent in this file.

Acknowledged.

https://codereview.webrtc.org/1766653002/diff/300001/webrtc/api/videotracksou...
File webrtc/api/videotracksource.h (right):

https://codereview.webrtc.org/1766653002/diff/300001/webrtc/api/videotracksou...
webrtc/api/videotracksource.h:41: return rtc::Optional<bool>(); }
This formatting looks wrong, but I assume it's not your CL.

[nisse-webrtc, 2016-04-01 13:08:52]

nisse@webrtc.org changed reviewers:
+ tkchin@webrtc.org

[nisse-webrtc, 2016-04-01 13:08:53]

@tkchin: I need a little advice on the small objc changes. This cl deletes the
GetVideoCapturer method, which was used for the capturer attribute of
RTCAVFoundationVideoSource.

As a substitute, I add an instance variable pointing to the capturer. I think a
raw pointer is ok, since the capturer is owned by the source, and we own the
source. But I can't type the pointer propertly as a
webrtc::AVFoundationVideoCapturer, because using namespaces in the public header
breaks inclusion in plain objc files (not Objective-C++).

So I resorted to void * and static casts in the init method and in the capturer
property access method. Do you think that is acceptable, or can you suggest a
better way? The problem is discussed in some detail at
http://stackoverflow.com/questions/1056911/c-classes-as-instance-variables-of...

(And less importantly, proper formatting is confusing as usual. git cl format
wants dirrent styles in the talk version and the webrtc version of the files).

Have a nice weekend!

[nisse-webrtc, 2016-04-01 13:09:06]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-01 13:09:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1766653002/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1766653002/420001

[commit-bot: I haz the power, 2016-04-01 15:09:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-01 15:09:45]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[nisse-webrtc, 2016-04-05 09:05:55]

The android failures on patch set 22 were due to a threading bug, related to the
proxy objects.

The cl changes VideoRtpSender from doing

provider_->SetCapturer(..., track_->GetSource()->GetVideoCapturer());

to 

provider_->SetSource(..., track_);

This results in a call to WebRtcVideoSendStream  on the worker thread,
previously it was SetCapturer, now SetSource.

Next, SetSource calls source->AddOrUpdateSink(this, ...).

AddOrUpdateSink is ultimately implemented in cricket::VideoCapturer, which wants
to be called on the worker thread. So when the source was a pointer directly to
the capturer, everything was fine.

However, in this cl, source is a VideoTrack, or rather, a VideoTrackProxy, which
marchals all methods to calls on the signalling thread.  The call is then passed
to the AddOrUpdateSink method in VideoTrackSource, which knows its source wants
calls on the worker thread, so it uses worker_thread_->Invoke.

So the worker thread makes a blocking invoke (in the proxy) to the signalling
thread, and when the message is handled, the signalling thread makes a blocking
invoke to the worker thread. Which obviously hangs, and no videoframes get
delivered.  It's a mystery why no end-to-end tests on other platforms failed
too.

The signal_thread/worker_thread architecture is a bit strange, but I guess it's
a bad idea to ever have the worker thread make a blocking request to the
signalling thread.

To try to fix this, I'm extending the proxy abstraction so that certain methods
can be attached to the worker thread rather than the signalling thread. I intend
to make a separate cl to sort of how to do that properly.

I'd also like to note that there's no need for AddOrUpdateSink to be blocking. I
think it might be a good idea to extend the proxy abstraction to allow some void
methods to be fire-and-forget, say, PROXY_ASYNC_METHODn. That won't solve this
problem, though, since RemoveSink still needs to be blocking, in particular, to
avoid a race between  the WebRtcVideoSendStream destructor and its OnFrame
method.

[nisse-webrtc, 2016-04-05 09:07:05]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-05 09:07:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1766653002/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1766653002/460001

[commit-bot: I haz the power, 2016-04-05 10:26:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-05 10:26:37]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2016-04-05 12:18:25]

On 2016/04/05 09:05:55, nisse-webrtc wrote:
> 
> To try to fix this, I'm extending the proxy abstraction so that certain
methods
> can be attached to the worker thread rather than the signalling thread. I
intend
> to make a separate cl to sort of how to do that properly.

That separate cl is https://codereview.webrtc.org/1861633002/

[tkchin_webrtc, 2016-04-05 16:10:28]

https://codereview.webrtc.org/1766653002/diff/460001/talk/app/webrtc/objc/RTC...
File talk/app/webrtc/objc/RTCAVFoundationVideoSource.mm (right):

https://codereview.webrtc.org/1766653002/diff/460001/talk/app/webrtc/objc/RTC...
talk/app/webrtc/objc/RTCAVFoundationVideoSource.mm:37: @implementation
RTCAVFoundationVideoSource
just add the instance var here to avoid casting etc 

@implementation RTCAVFoundationVideoSource {
  webrtc::AVFoundationVideoCapturer* _capturer;
}

https://codereview.webrtc.org/1766653002/diff/460001/webrtc/api/objc/RTCAVFou...
File webrtc/api/objc/RTCAVFoundationVideoSource.mm (right):

https://codereview.webrtc.org/1766653002/diff/460001/webrtc/api/objc/RTCAVFou...
webrtc/api/objc/RTCAVFoundationVideoSource.mm:17: @implementation
RTCAVFoundationVideoSource
ditto

[nisse-webrtc, 2016-04-06 08:09:07]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-06 08:09:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1766653002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1766653002/500001

[nisse-webrtc, 2016-04-06 08:11:48]

In other notes, I have confirmed that after rebasing on top of
https://codereview.webrtc.org/1859933002/, the threading problem can be
reproduced using peerconnection_unittests on gnu/linux.

https://codereview.webrtc.org/1766653002/diff/460001/talk/app/webrtc/objc/RTC...
File talk/app/webrtc/objc/RTCAVFoundationVideoSource.mm (right):

https://codereview.webrtc.org/1766653002/diff/460001/talk/app/webrtc/objc/RTC...
talk/app/webrtc/objc/RTCAVFoundationVideoSource.mm:37: @implementation
RTCAVFoundationVideoSource
On 2016/04/05 16:10:27, tkchin_webrtc wrote:
> just add the instance var here to avoid casting etc 
> 
> @implementation RTCAVFoundationVideoSource {
>   webrtc::AVFoundationVideoCapturer* _capturer;
> }

Thanks, I had no idea you could define instance variables here. I thought
everything had to be in the header, to give the object a known storage size. Are
the objc objects always allocated dynamically, no static or stack allocation?

[commit-bot: I haz the power, 2016-04-06 09:24:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-06 09:24:59]

Dry run: This issue passed the CQ dry run.

[tkchin_webrtc, 2016-04-06 19:41:47]

lgtm

looking forward to not needing a videocapturer anymore in the future though so
we can get rid of that unsafe ptr.

https://codereview.webrtc.org/1766653002/diff/460001/talk/app/webrtc/objc/RTC...
File talk/app/webrtc/objc/RTCAVFoundationVideoSource.mm (right):

https://codereview.webrtc.org/1766653002/diff/460001/talk/app/webrtc/objc/RTC...
talk/app/webrtc/objc/RTCAVFoundationVideoSource.mm:37: @implementation
RTCAVFoundationVideoSource
On 2016/04/06 08:11:48, nisse-webrtc wrote:
> On 2016/04/05 16:10:27, tkchin_webrtc wrote:
> > just add the instance var here to avoid casting etc 
> > 
> > @implementation RTCAVFoundationVideoSource {
> >   webrtc::AVFoundationVideoCapturer* _capturer;
> > }
> 
> Thanks, I had no idea you could define instance variables here. I thought
> everything had to be in the header, to give the object a known storage size.
Are
> the objc objects always allocated dynamically, no static or stack allocation?

Yes, all objc objects are dynamically allocated.

https://codereview.webrtc.org/1766653002/diff/500001/talk/app/webrtc/objc/RTC...
File talk/app/webrtc/objc/RTCAVFoundationVideoSource.mm (right):

https://codereview.webrtc.org/1766653002/diff/500001/talk/app/webrtc/objc/RTC...
talk/app/webrtc/objc/RTCAVFoundationVideoSource.mm:38:
webrtc::AVFoundationVideoCapturer* _capturer;
nit: Capturer *_capturer; (the style in the file is old)

https://codereview.webrtc.org/1766653002/diff/500001/webrtc/api/objc/RTCAVFou...
File webrtc/api/objc/RTCAVFoundationVideoSource.mm (right):

https://codereview.webrtc.org/1766653002/diff/500001/webrtc/api/objc/RTCAVFou...
webrtc/api/objc/RTCAVFoundationVideoSource.mm:18:
webrtc::AVFoundationVideoCapturer* _capturer;
space * like the rest here.

[nisse-webrtc, 2016-04-08 08:34:51]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-04-08 08:34:51]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org,
pthatcher@webrtc.org, pbos@webrtc.org, tkchin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1766653002/#ps540001 (title:
"Rebased, no longer any proxy object changes.")

[commit-bot: I haz the power, 2016-04-08 08:34:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1766653002/540001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1766653002/540001

[commit-bot: I haz the power, 2016-04-08 09:23:59]

Description was changed from

==========
Replace SetCapturer and SetCaptureDevice by SetSource.
Drop return value.

BUG=webrtc:5426
==========

to

==========
Replace SetCapturer and SetCaptureDevice by SetSource.
Drop return value.

BUG=webrtc:5426
==========

[commit-bot: I haz the power, 2016-04-08 09:24:01]

Committed patchset #28 (id:540001)

[commit-bot: I haz the power, 2016-04-08 09:24:10]

Description was changed from

==========
Replace SetCapturer and SetCaptureDevice by SetSource.
Drop return value.

BUG=webrtc:5426
==========

to

==========
Replace SetCapturer and SetCaptureDevice by SetSource.
Drop return value.

BUG=webrtc:5426

Committed: https://crrev.com/2ded9b19d1cb2ad4cf4d69c34094fbddc3809888
Cr-Commit-Position: refs/heads/master@{#12291}
==========

[commit-bot: I haz the power, 2016-04-08 09:24:12]

Patchset 28 (id:??) landed as
https://crrev.com/2ded9b19d1cb2ad4cf4d69c34094fbddc3809888
Cr-Commit-Position: refs/heads/master@{#12291}