Add rtpdump and rtc log functionality to screenshare_loopback and video_loopback

Add rtpdump and rtc log functionality to screenshare_loopback and video_loopback

BUG=none

Review-Url: https://codereview.webrtc.org/2974903002
Cr-Commit-Position: refs/heads/master@{#18996}
Committed: https://chromium.googlesource.com/external/webrtc/+/984369516641507af3b3c352999f2147e784e124

[ilnik, 2017-07-11 09:24:01]

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

[commit-bot: I haz the power, 2017-07-11 09:24:05]

Dry run: CQ is trying da patch.

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

[ilnik, 2017-07-11 09:24:12]

ilnik@webrtc.org changed reviewers:
+ sprang@webrtc.org

[commit-bot: I haz the power, 2017-07-11 10:24:15]

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

[commit-bot: I haz the power, 2017-07-11 10:24:16]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-07-12 13:27:06]

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

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:223: if (rtp_dump_name.length() > 0) {
nit: if (!rtp_dump_name.empty()) ...

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:282: std::unique_ptr<test::RtpPacket> p(new
test::RtpPacket());
Just stack allocate it instead.

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1085: Clock* clock_;
Clock* const clock_;

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1742: bool event_log_started =  false;
nit: git cl format?

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1743: if (params.rtc_event_log_name.length()
> 0) {
.empty()

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1745: event_log_started =
event_log_->StartLogging(params.rtc_event_log_name, -1);
Why would StartLogging() fail?
Can you just DCHECK instead?

Also, I don't think you need event_log_started, just call StopLogging() in any
case below. The member will by default be populated by a null-impl that does
nothing in that case.

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.h (right):

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.h:90: std::string rtp_dump_name;
Could you perhaps create a new substruct here called Logging or something like
that? Then we can move these two, the |logs| boolean and encoded_frame_base_path
there.

Also, shouldn't num_thumbnails be in the Video section?

[ilnik, 2017-07-12 13:58:53]

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

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:223: if (rtp_dump_name.length() > 0) {
On 2017/07/12 13:27:05, sprang_webrtc wrote:
> nit: if (!rtp_dump_name.empty()) ...

Done.

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:282: std::unique_ptr<test::RtpPacket> p(new
test::RtpPacket());
On 2017/07/12 13:27:05, sprang_webrtc wrote:
> Just stack allocate it instead.

Done.

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1085: Clock* clock_;
On 2017/07/12 13:27:05, sprang_webrtc wrote:
> Clock* const clock_;

Done.

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1742: bool event_log_started =  false;
On 2017/07/12 13:27:05, sprang_webrtc wrote:
> nit: git cl format?

Done.

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1743: if (params.rtc_event_log_name.length()
> 0) {
On 2017/07/12 13:27:05, sprang_webrtc wrote:
> .empty()

Done.

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1745: event_log_started =
event_log_->StartLogging(params.rtc_event_log_name, -1);
On 2017/07/12 13:27:05, sprang_webrtc wrote:
> Why would StartLogging() fail?
> Can you just DCHECK instead?
> 
> Also, I don't think you need event_log_started, just call StopLogging() in any
> case below. The member will by default be populated by a null-impl that does
> nothing in that case.

Done.

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.h (right):

https://codereview.webrtc.org/2974903002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.h:90: std::string rtp_dump_name;
On 2017/07/12 13:27:06, sprang_webrtc wrote:
> Could you perhaps create a new substruct here called Logging or something like
> that? Then we can move these two, the |logs| boolean and
encoded_frame_base_path
> there.
> 
> Also, shouldn't num_thumbnails be in the Video section?

Done. 
num_thumbnails is more of a call property, not a video. Moved it there.

[ilnik, 2017-07-12 13:59:03]

Description was changed from

==========
Add rtpdump and rtc log functionality to screenshare_loopback

BUG=none
==========

to

==========
Add rtpdump and rtc log functionality to screenshare_loopback

BUG=none
==========

[ilnik, 2017-07-12 13:59:16]

Description was changed from

==========
Add rtpdump and rtc log functionality to screenshare_loopback

BUG=none
==========

to

==========
Add rtpdump and rtc log functionality to screenshare_loopback and video_loopback

BUG=none
==========

[sprang_webrtc, 2017-07-12 14:09:13]

lgtm
thanks!

[ilnik, 2017-07-12 14:49:10]

The CQ bit was checked by ilnik@webrtc.org

[commit-bot: I haz the power, 2017-07-12 14:49:14]

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, 2017-07-12 14:50:43]

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

[commit-bot: I haz the power, 2017-07-12 14:50:44]

Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/21942)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/21727)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/27253)

[ilnik, 2017-07-12 14:55:32]

The CQ bit was checked by ilnik@webrtc.org

[ilnik, 2017-07-12 14:55:33]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2974903002/#ps100001 (title:
"rebase")

[commit-bot: I haz the power, 2017-07-12 14:55:36]

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, 2017-07-12 16:56:05]

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

[commit-bot: I haz the power, 2017-07-12 16:56:07]

Try jobs failed on following builders:
  win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started
yet; builder either lacks capacity or does not exist (misspelled?))
  win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started
yet; builder either lacks capacity or does not exist (misspelled?))
  win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not
started yet; builder either lacks capacity or does not exist (misspelled?))
  win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started
yet; builder either lacks capacity or does not exist (misspelled?))
  win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started
yet; builder either lacks capacity or does not exist (misspelled?))

[ilnik, 2017-07-13 07:44:04]

The CQ bit was checked by ilnik@webrtc.org

[commit-bot: I haz the power, 2017-07-13 07:44:11]

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, 2017-07-13 07:47:03]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1499931844947250,
"parent_rev": "168baeda6f2561a0e048df398dcb9cbea6e02720", "commit_rev":
"984369516641507af3b3c352999f2147e784e124"}

[commit-bot: I haz the power, 2017-07-13 07:47:07]

Description was changed from

==========
Add rtpdump and rtc log functionality to screenshare_loopback and video_loopback

BUG=none
==========

to

==========
Add rtpdump and rtc log functionality to screenshare_loopback and video_loopback

BUG=none

Review-Url: https://codereview.webrtc.org/2974903002
Cr-Commit-Position: refs/heads/master@{#18996}
Committed:
https://chromium.googlesource.com/external/webrtc/+/984369516641507af3b3c3529...
==========

[commit-bot: I haz the power, 2017-07-13 07:47:08]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/984369516641507af3b3c3529...