Adding audio only mode to video loopback test.

Adding audio only mode to video loopback test.

BUG=webrtc:6609
Committed: https://crrev.com/a27172d683e1cd7272dfb4197c979af4220b6bc6
Cr-Commit-Position: refs/heads/master@{#14875}

[minyue-webrtc, 2016-09-07 10:25:43]

Patchset #1 (id:1) has been deleted

[minyue-webrtc, 2016-09-07 10:28:26]

Description was changed from

==========
work


Getting started

BUG=
==========

to

==========
Adding audio only mode to video loopback test.

BUG=
==========

[minyue-webrtc, 2016-09-07 10:28:26]

minyue@webrtc.org changed reviewers:
+ stefan@webrtc.org

[minyue-webrtc, 2016-09-07 10:28:37]

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

[commit-bot: I haz the power, 2016-09-07 10:28:40]

Dry run: 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-09-07 10:44:30]

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

[commit-bot: I haz the power, 2016-09-07 10:44:33]

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

[minyue-webrtc, 2016-09-07 11:32:33]

Patchset #1 (id:20001) has been deleted

[minyue-webrtc, 2016-09-07 11:32:43]

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

[commit-bot: I haz the power, 2016-09-07 11:32:52]

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

[minyue-webrtc, 2016-09-07 12:45:20]

Hi Stefan,

This CL makes audio only mode possible on video loopback test. I do not know if
we should rename relevant classes. Let me know if we need, I can take it in a
following Patch set.

This CL depends on an earlier refactoring CL that I have sent you.

[commit-bot: I haz the power, 2016-09-07 12:55:19]

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

[commit-bot: I haz the power, 2016-09-07 12:55:20]

Dry run: Try jobs failed on following builders:
  linux_baremetal on master.tryserver.webrtc (JOB_FAILED, no build URL)

[stefan-webrtc, 2016-09-07 13:12:11]

https://codereview.webrtc.org/2321463002/diff/40001/webrtc/video/video_loopba...
File webrtc/video/video_loopback.cc (right):

https://codereview.webrtc.org/2321463002/diff/40001/webrtc/video/video_loopba...
webrtc/video/video_loopback.cc:199: DEFINE_bool(mute_video, false, "Mute video
stream");
Can we call this "video" instead to align it with "audio"?

https://codereview.webrtc.org/2321463002/diff/40001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2321463002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1245: // TODO(minyue): maybe move the
following to SetupVideo() to make code
Any chance we can do this now? It would look a lot better as it'd be more
aligned with the audio part

[stefan-webrtc, 2016-09-07 13:12:42]

On 2016/09/07 12:45:20, minyue-webrtc wrote:
> Hi Stefan,
> 
> This CL makes audio only mode possible on video loopback test. I do not know
if
> we should rename relevant classes. Let me know if we need, I can take it in a
> following Patch set.
> 
> This CL depends on an earlier refactoring CL that I have sent you.

I think it might make sense to rename things, but feel free to follow up with
that.

[minyue-webrtc, 2016-09-08 15:38:24]

Hi Stefan,

I updated the CL and PTAL again, thanks!

https://codereview.webrtc.org/2321463002/diff/40001/webrtc/video/video_loopba...
File webrtc/video/video_loopback.cc (right):

https://codereview.webrtc.org/2321463002/diff/40001/webrtc/video/video_loopba...
webrtc/video/video_loopback.cc:199: DEFINE_bool(mute_video, false, "Mute video
stream");
On 2016/09/07 13:12:10, stefan-webrtc (holmer) wrote:
> Can we call this "video" instead to align it with "audio"?

Done.

https://codereview.webrtc.org/2321463002/diff/40001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2321463002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1245: // TODO(minyue): maybe move the
following to SetupVideo() to make code
On 2016/09/07 13:12:10, stefan-webrtc (holmer) wrote:
> Any chance we can do this now? It would look a lot better as it'd be more
> aligned with the audio part

The reason that I am not very confident in modifying it is that SetupVideo() is
used by both RunWithRenderers and RunWithAnalyzer, the out-of-SetupVideo parts
of the two are hard to combine.

BTW, audio-only mode is not added to RunWithAnalyzer yet.

[stefan-webrtc, 2016-09-13 09:50:23]

https://codereview.webrtc.org/2321463002/diff/40001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2321463002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1245: // TODO(minyue): maybe move the
following to SetupVideo() to make code
On 2016/09/08 15:38:24, minyue-webrtc wrote:
> On 2016/09/07 13:12:10, stefan-webrtc (holmer) wrote:
> > Any chance we can do this now? It would look a lot better as it'd be more
> > aligned with the audio part
> 
> The reason that I am not very confident in modifying it is that SetupVideo()
is
> used by both RunWithRenderers and RunWithAnalyzer, the out-of-SetupVideo parts
> of the two are hard to combine.

I'm not sure I follow. This method is called RunWithRenderers, so I assume it's
only called for renderers? What is it that you have to combine?

> 
> BTW, audio-only mode is not added to RunWithAnalyzer yet.

[stefan-webrtc, 2016-09-29 11:41:01]

Ping, would be great to land this so we can measure overheads and tune how well
audio BWE works.

[minyue-webrtc, 2016-10-28 13:43:43]

Patchset #4 (id:100001) has been deleted

[minyue-webrtc, 2016-10-28 14:01:00]

Hi Stefan,

This is rebased, and I tried that it worked.

I answer to your earlier comment, and please see if you want to more changes.

https://codereview.webrtc.org/2321463002/diff/40001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2321463002/diff/40001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1245: // TODO(minyue): maybe move the
following to SetupVideo() to make code
On 2016/09/13 09:50:23, stefan-webrtc (holmer) wrote:
> On 2016/09/08 15:38:24, minyue-webrtc wrote:
> > On 2016/09/07 13:12:10, stefan-webrtc (holmer) wrote:
> > > Any chance we can do this now? It would look a lot better as it'd be more
> > > aligned with the audio part
> > 
> > The reason that I am not very confident in modifying it is that SetupVideo()
> is
> > used by both RunWithRenderers and RunWithAnalyzer, the out-of-SetupVideo
parts
> > of the two are hard to combine.
> 
> I'm not sure I follow. This method is called RunWithRenderers, so I assume
it's
> only called for renderers? What is it that you have to combine?
> 
> > 
> > BTW, audio-only mode is not added to RunWithAnalyzer yet.

If you compare the lines following SetupVideo() in RunWithRenderers() and
RunWithAnalyzer(), i.e. line 1247 - 1271 vs line 1119 - 1127, there are quite
many differences. E.g. one deals with FEC and the other does not. I am not sure
that we should unify them now in this CL.

[stefan-webrtc, 2016-10-31 09:06:56]

Thanks!

Please add a BUG= to this CL.

https://codereview.webrtc.org/2321463002/diff/120001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2321463002/diff/120001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:1320: if (params_.video.enabled) {
I'd move everything below to SetupVideo. Maybe there's a reason not to do that
though?

https://codereview.webrtc.org/2321463002/diff/120001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:1321: // Create video renders.
video renderers

[stefan-webrtc, 2016-10-31 09:09:42]

https://codereview.webrtc.org/2321463002/diff/120001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2321463002/diff/120001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:1320: if (params_.video.enabled) {
On 2016/10/31 09:06:56, stefan-webrtc (holmer) wrote:
> I'd move everything below to SetupVideo. Maybe there's a reason not to do that
> though?

Feel free to ignore, I missed the old discussion around this...

Maybe you could move the TODO on line 1339 up here and explain why it isn't
trivial at least?

[minyue-webrtc, 2016-10-31 09:26:31]

On 2016/10/31 09:09:42, stefan-webrtc (holmer) wrote:
>
https://codereview.webrtc.org/2321463002/diff/120001/webrtc/video/video_quali...
> File webrtc/video/video_quality_test.cc (right):
> 
>
https://codereview.webrtc.org/2321463002/diff/120001/webrtc/video/video_quali...
> webrtc/video/video_quality_test.cc:1320: if (params_.video.enabled) {
> On 2016/10/31 09:06:56, stefan-webrtc (holmer) wrote:
> > I'd move everything below to SetupVideo. Maybe there's a reason not to do
that
> > though?
> 
> Feel free to ignore, I missed the old discussion around this...
> 
> Maybe you could move the TODO on line 1339 up here and explain why it isn't
> trivial at least?

one pre-CL https://codereview.webrtc.org/2314403007/ was not properly done. will
reland that first.

[minyue-webrtc, 2016-11-01 11:53:34]

Description was changed from

==========
Adding audio only mode to video loopback test.

BUG=
==========

to

==========
Adding audio only mode to video loopback test.

BUG=webrtc:6609
==========

[minyue-webrtc, 2016-11-01 12:05:50]

Hi Stefan,

Please find in the latest patch set for changes according to your request.

https://codereview.webrtc.org/2321463002/diff/120001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2321463002/diff/120001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:1320: if (params_.video.enabled) {
On 2016/10/31 09:09:42, stefan-webrtc (holmer) wrote:
> On 2016/10/31 09:06:56, stefan-webrtc (holmer) wrote:
> > I'd move everything below to SetupVideo. Maybe there's a reason not to do
that
> > though?
> 
> Feel free to ignore, I missed the old discussion around this...
> 
> Maybe you could move the TODO on line 1339 up here and explain why it isn't
> trivial at least?

I think video renderer creation (1321-1335) should be outside SetVideo() any
ways, it is like VideoAnalyzer creation (1195). What matters is the difficulty
that prevent us from moving 1341 - 1466 into SetVideo. So I think TODO on 1339
should be there. But good suggestion to add comments on not moving it now.

https://codereview.webrtc.org/2321463002/diff/120001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:1321: // Create video renders.
On 2016/10/31 09:06:56, stefan-webrtc (holmer) wrote:
> video renderers

Done.

[stefan-webrtc, 2016-11-01 12:20:15]

lgtm

[minyue-webrtc, 2016-11-01 12:21:29]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2016-11-01 12:21:35]

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-11-01 12:43:13]

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

[commit-bot: I haz the power, 2016-11-01 12:43:14]

Try jobs failed on following builders:
  win_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/2684)

[minyue-webrtc, 2016-11-01 12:53:43]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2016-11-01 12:53:54]

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-11-01 12:59:31]

Description was changed from

==========
Adding audio only mode to video loopback test.

BUG=webrtc:6609
==========

to

==========
Adding audio only mode to video loopback test.

BUG=webrtc:6609
==========

[commit-bot: I haz the power, 2016-11-01 12:59:33]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2016-11-01 12:59:43]

Description was changed from

==========
Adding audio only mode to video loopback test.

BUG=webrtc:6609
==========

to

==========
Adding audio only mode to video loopback test.

BUG=webrtc:6609

Committed: https://crrev.com/a27172d683e1cd7272dfb4197c979af4220b6bc6
Cr-Commit-Position: refs/heads/master@{#14875}
==========

[commit-bot: I haz the power, 2016-11-01 12:59:44]

Patchset 6 (id:??) landed as
https://crrev.com/a27172d683e1cd7272dfb4197c979af4220b6bc6
Cr-Commit-Position: refs/heads/master@{#14875}