Refactor WebRtcVideoCapturer.

Refactor WebRtcVideoCapturer.

Pass incoming frames directly to VideoCapturer::OnFrame (after
conversion to cricket::VideoFrame), without using SignalFrameCaptured
or WebRtcCapturedFrame.

BUG=webrtc:5682
Committed: https://crrev.com/792469709d9edea8af82c74d56116bcdfc2428a1
Cr-Commit-Position: refs/heads/master@{#13861}

[nisse-webrtc, 2016-08-19 12:01:16]

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

[nisse-webrtc, 2016-08-19 12:01:16]

PTAL. Known issues in commends below.

I'd like to get this done first, before attempting to kill CapturedFrame and
VideoFrameFactory.

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideocapturer.cc (right):

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideocapturer.cc:350: RTC_DCHECK(start_thread_);
I guess we can delete the async_invoker_. Can we delete start_thread_ too, or
what is that needed for?

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideocapturer_unittest.cc (right):

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideocapturer_unittest.cc:12: // without
-DHAVE_WEBRTC_VIDEO. How to fix?
Should this be set explicitly in media.gyp? Or should it be inherited by some
dependency?

Do we even need the conditional in this testfile, it seems saner to not build it
at all if video support is disabled.

[nisse-webrtc, 2016-08-19 12:48:42]

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideocapturer.cc (right):

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideocapturer.cc:22: #include
"webrtc/media/engine/webrtcvideoframefactory.h"
Note to self: Drop this include.

[perkj_webrtc, 2016-08-19 13:08:59]

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/fakewebrt...
File webrtc/media/engine/fakewebrtcvideocapturemodule.h (right):

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/fakewebrt...
webrtc/media/engine/fakewebrtcvideocapturemodule.h:78: return true;  // ignored
why this change?

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideocapturer.cc (right):

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideocapturer.cc:22: #include
"webrtc/media/engine/webrtcvideoframefactory.h"
On 2016/08/19 12:48:42, nisse-webrtc wrote:
> Note to self: Drop this include.

sg

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideocapturer.cc:350: RTC_DCHECK(start_thread_);
On 2016/08/19 12:01:16, nisse-webrtc wrote:
> I guess we can delete the async_invoker_. Can we delete start_thread_ too, or
> what is that needed for?

drop it all.

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideocapturer_unittest.cc (right):

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideocapturer_unittest.cc:12: // without
-DHAVE_WEBRTC_VIDEO. How to fix?
On 2016/08/19 12:01:16, nisse-webrtc wrote:
> Should this be set explicitly in media.gyp? Or should it be inherited by some
> dependency?
> 
> Do we even need the conditional in this testfile, it seems saner to not build
it
> at all if video support is disabled.

just see what happens if you drop if HAVE_WEBRTC_VIDEO at all. I don't think it
is needed any more.

[nisse-webrtc, 2016-08-19 13:30:02]

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/fakewebrt...
File webrtc/media/engine/fakewebrtcvideocapturemodule.h (right):

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/fakewebrt...
webrtc/media/engine/fakewebrtcvideocapturemodule.h:78: return true;  // ignored
On 2016/08/19 13:08:59, perkj_webrtc wrote:
> why this change? 

Because the change to have VideoCaptureListener use AddOrUpdateSink generates a
call to SetApplyRotation, and if that call fails,
WebRtcVideoCapturer::OnSinkWantsChanged dies with an assertion failure.

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideocapturer_unittest.cc (right):

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideocapturer_unittest.cc:12: // without
-DHAVE_WEBRTC_VIDEO. How to fix?
On 2016/08/19 13:08:59, perkj_webrtc wrote:
> On 2016/08/19 12:01:16, nisse-webrtc wrote:
> > Should this be set explicitly in media.gyp? Or should it be inherited by
some
> > dependency?
> > 
> > Do we even need the conditional in this testfile, it seems saner to not
build
> it
> > at all if video support is disabled.
> 
> just see what happens if you drop if HAVE_WEBRTC_VIDEO at all. I don't think
it
> is needed any more.

Let's see if kjellander has any comments on the other cl.
(https://codereview.webrtc.org/2250433008/).

[nisse-webrtc, 2016-08-23 08:36:26]

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideocapturer_unittest.cc (right):

https://codereview.webrtc.org/2258933003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideocapturer_unittest.cc:12: // without
-DHAVE_WEBRTC_VIDEO. How to fix?
On 2016/08/19 13:30:02, nisse-webrtc wrote:
> On 2016/08/19 13:08:59, perkj_webrtc wrote:
> > On 2016/08/19 12:01:16, nisse-webrtc wrote:
> > > Should this be set explicitly in media.gyp? Or should it be inherited by
> some
> > > dependency?
> > > 
> > > Do we even need the conditional in this testfile, it seems saner to not
> build
> > it
> > > at all if video support is disabled.
> > 
> > just see what happens if you drop if HAVE_WEBRTC_VIDEO at all. I don't think
> it
> > is needed any more.
> 
> Let's see if kjellander has any comments on the other cl.
> (https://codereview.webrtc.org/2250433008/).

That cl was landed, so reverted this change.

[perkj_webrtc, 2016-08-23 08:49:41]

lgtm

[nisse-webrtc, 2016-08-23 08:50:38]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-08-23 08:50:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-23 10:51:21]

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

[commit-bot: I haz the power, 2016-08-23 10:51:22]

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

[nisse-webrtc, 2016-08-23 10:53:23]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-08-23 10:53:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-23 12:50:14]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-23 12:50:27]

Description was changed from

==========
Refactor WebRtcVideoCapturer.

Pass incoming frames directly to VideoCapturer::OnFrame (after
conversion to cricket::VideoFrame), without using SignalFrameCaptured
or WebRtcCapturedFrame.

BUG=webrtc:5682
==========

to

==========
Refactor WebRtcVideoCapturer.

Pass incoming frames directly to VideoCapturer::OnFrame (after
conversion to cricket::VideoFrame), without using SignalFrameCaptured
or WebRtcCapturedFrame.

BUG=webrtc:5682

Committed: https://crrev.com/792469709d9edea8af82c74d56116bcdfc2428a1
Cr-Commit-Position: refs/heads/master@{#13861}
==========

[commit-bot: I haz the power, 2016-08-23 12:50:28]

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