Resolve cyclic dependency between audio network adaptor and event log api

Resolve cyclic dependency between audio network adaptor and event log api

BUG=webrtc:7257
Review-Url: https://codereview.webrtc.org/2745473003
Cr-Commit-Position: refs/heads/master@{#17565}
Committed: https://chromium.googlesource.com/external/webrtc/+/cde46b72788f5e37cfae33824b54d9c7d8a14ef8

[michaelt, 2017-03-09 14:12:00]

Description was changed from

==========
Resolve cyclic dependency between audio network adaptor and event log api

BUG=webrtc:7257
==========

to

==========
Resolve cyclic dependency between audio network adaptor and event log api

BUG=webrtc:7257
==========

[michaelt, 2017-03-09 14:12:00]

michaelt@webrtc.org changed reviewers:
+ terelius@webrtc.org

[michaelt, 2017-03-09 14:14:02]

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

[michaelt, 2017-03-09 14:14:03]

It seams there is a problem with the bwe_defines.h include as well. 
So i did not remove the "not check fix".

[terelius, 2017-03-09 14:28:59]

https://codereview.webrtc.org/2745473003/diff/1/webrtc/logging/BUILD.gn
File webrtc/logging/BUILD.gn (right):

https://codereview.webrtc.org/2745473003/diff/1/webrtc/logging/BUILD.gn#newco...
webrtc/logging/BUILD.gn:56: "../modules/audio_coding:audio_network_adaptor",
This should be removed, right?

https://codereview.webrtc.org/2745473003/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h (right):

https://codereview.webrtc.org/2745473003/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h:17: #include
"webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h"
Can't you just forward declare the AudioEncoderRuntimeConfig?

[michaelt, 2017-03-09 14:42:54]

https://codereview.webrtc.org/2745473003/diff/1/webrtc/logging/BUILD.gn
File webrtc/logging/BUILD.gn (right):

https://codereview.webrtc.org/2745473003/diff/1/webrtc/logging/BUILD.gn#newco...
webrtc/logging/BUILD.gn:56: "../modules/audio_coding:audio_network_adaptor",
No, since its really used in the rtc_event_log_impl.

https://codereview.webrtc.org/2745473003/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h (right):

https://codereview.webrtc.org/2745473003/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h:17: #include
"webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h"
Gmock seams not to like forward declaration. :)

[terelius, 2017-03-10 10:26:57]

lgtm

https://codereview.webrtc.org/2745473003/diff/1/webrtc/logging/BUILD.gn
File webrtc/logging/BUILD.gn (right):

https://codereview.webrtc.org/2745473003/diff/1/webrtc/logging/BUILD.gn#newco...
webrtc/logging/BUILD.gn:56: "../modules/audio_coding:audio_network_adaptor",
On 2017/03/09 14:42:54, michaelt wrote:
> No, since its really used in the rtc_event_log_impl.
> 

Sorry. I mixed up the build targets.

[the sun, 2017-03-10 12:09:26]

solenberg@webrtc.org changed reviewers:
+ ossu@webrtc.org, solenberg@webrtc.org

[the sun, 2017-03-10 12:09:27]

Drive-by lgtm for voice_engine (I bet you don't want to wait for Minyue to be
back).

Also adding ossu@ since there may be points of contact with his work on
injectable encoders: https://codereview.webrtc.org/2695243005/

[michaelt, 2017-03-22 09:25:32]

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

[commit-bot: I haz the power, 2017-03-22 09:25:44]

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, 2017-03-22 09:52:38]

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

[commit-bot: I haz the power, 2017-03-22 09:52:39]

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

[minyue-webrtc, 2017-03-23 09:19:09]

lgtm

[michaelt, 2017-03-23 09:51:33]

The CQ bit was checked by michaelt@webrtc.org

[commit-bot: I haz the power, 2017-03-23 09:51:44]

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-03-23 09:56:51]

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

[commit-bot: I haz the power, 2017-03-23 09:56:52]

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

[michaelt, 2017-03-27 15:04:09]

The CQ bit was checked by michaelt@webrtc.org

[commit-bot: I haz the power, 2017-03-27 15:04:13]

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-03-27 15:05:42]

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

[commit-bot: I haz the power, 2017-03-27 15:05:43]

Try jobs failed on following builders:
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24244)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23058)

[michaelt, 2017-03-28 10:40:34]

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

[commit-bot: I haz the power, 2017-03-28 10:40: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, 2017-03-28 11:26:01]

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

[commit-bot: I haz the power, 2017-03-28 11:26:02]

Dry run: This issue passed the CQ dry run.

[michaelt, 2017-03-28 12:11:12]

The CQ bit was checked by michaelt@webrtc.org

[michaelt, 2017-03-28 12:11:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, terelius@webrtc.org, minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2745473003/#ps20001 (title:
"Rebased")

[commit-bot: I haz the power, 2017-03-28 12:11:23]

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-03-28 12:15:55]

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

[commit-bot: I haz the power, 2017-03-28 12:15:57]

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

[michaelt, 2017-04-04 12:02:46]

michaelt@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[michaelt, 2017-04-06 08:18:36]

@kjellander_webrtc
Can you pleas take a look, I need approval from tools for this.

[kjellander_webrtc, 2017-04-06 08:41:38]

This is awesome, thanks for taking the time to clean this up.

https://codereview.webrtc.org/2745473003/diff/20001/webrtc/logging/BUILD.gn
File webrtc/logging/BUILD.gn (right):

https://codereview.webrtc.org/2745473003/diff/20001/webrtc/logging/BUILD.gn#n...
webrtc/logging/BUILD.gn:31: check_includes = false
Does this mean that this can be removed now?

Try running gn check after doing so (trybots also always pass --check to gn gen
so they will tell you if it's OK).

[michaelt, 2017-04-06 09:16:37]

https://codereview.webrtc.org/2745473003/diff/20001/webrtc/logging/BUILD.gn
File webrtc/logging/BUILD.gn (right):

https://codereview.webrtc.org/2745473003/diff/20001/webrtc/logging/BUILD.gn#n...
webrtc/logging/BUILD.gn:31: check_includes = false
When I activate the check some unrelated problems pop up:
ERROR at //webrtc/logging/rtc_event_log/rtc_event_log.h:20:11: Include not
allowed.
#include "webrtc/modules/remote_bitrate_estimator/include/bwe_defines.h"
          ^------------------------------------------------------------
It is not in any dependency of
  //webrtc/logging:rtc_event_log_api
The include file is in the target(s):
  //webrtc/modules/remote_bitrate_estimator:remote_bitrate_estimator
which should somehow be reachable. 

Should I try to solve them in this CL as well ?

[kjellander_webrtc, 2017-04-06 09:24:17]

On 2017/04/06 09:16:37, michaelt wrote:
> https://codereview.webrtc.org/2745473003/diff/20001/webrtc/logging/BUILD.gn
> File webrtc/logging/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2745473003/diff/20001/webrtc/logging/BUILD.gn#n...
> webrtc/logging/BUILD.gn:31: check_includes = false
> When I activate the check some unrelated problems pop up:
> ERROR at //webrtc/logging/rtc_event_log/rtc_event_log.h:20:11: Include not
> allowed.
> #include "webrtc/modules/remote_bitrate_estimator/include/bwe_defines.h"
>           ^------------------------------------------------------------
> It is not in any dependency of
>   //webrtc/logging:rtc_event_log_api
> The include file is in the target(s):
>   //webrtc/modules/remote_bitrate_estimator:remote_bitrate_estimator
> which should somehow be reachable. 
> 
> Should I try to solve them in this CL as well ?

That would be great. If it's just a matter of a few missing dependencies, it
would make sense to bake them into this one. If it grows - it could just as well
be landed as a follow-up.

[michaelt, 2017-04-06 09:31:48]

On 2017/04/06 09:24:17, kjellander_webrtc wrote:
> On 2017/04/06 09:16:37, michaelt wrote:
> > https://codereview.webrtc.org/2745473003/diff/20001/webrtc/logging/BUILD.gn
> > File webrtc/logging/BUILD.gn (right):
> > 
> >
>
https://codereview.webrtc.org/2745473003/diff/20001/webrtc/logging/BUILD.gn#n...
> > webrtc/logging/BUILD.gn:31: check_includes = false
> > When I activate the check some unrelated problems pop up:
> > ERROR at //webrtc/logging/rtc_event_log/rtc_event_log.h:20:11: Include not
> > allowed.
> > #include "webrtc/modules/remote_bitrate_estimator/include/bwe_defines.h"
> >           ^------------------------------------------------------------
> > It is not in any dependency of
> >   //webrtc/logging:rtc_event_log_api
> > The include file is in the target(s):
> >   //webrtc/modules/remote_bitrate_estimator:remote_bitrate_estimator
> > which should somehow be reachable. 
> > 
> > Should I try to solve them in this CL as well ?
> 
> That would be great. If it's just a matter of a few missing dependencies, it
> would make sense to bake them into this one. If it grows - it could just as
well
> be landed as a follow-up.

Done

[kjellander_webrtc, 2017-04-06 09:35:09]

On 2017/04/06 09:31:48, michaelt wrote:
> On 2017/04/06 09:24:17, kjellander_webrtc wrote:
> > On 2017/04/06 09:16:37, michaelt wrote:
> > >
https://codereview.webrtc.org/2745473003/diff/20001/webrtc/logging/BUILD.gn
> > > File webrtc/logging/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2745473003/diff/20001/webrtc/logging/BUILD.gn#n...
> > > webrtc/logging/BUILD.gn:31: check_includes = false
> > > When I activate the check some unrelated problems pop up:
> > > ERROR at //webrtc/logging/rtc_event_log/rtc_event_log.h:20:11: Include not
> > > allowed.
> > > #include "webrtc/modules/remote_bitrate_estimator/include/bwe_defines.h"
> > >           ^------------------------------------------------------------
> > > It is not in any dependency of
> > >   //webrtc/logging:rtc_event_log_api
> > > The include file is in the target(s):
> > >   //webrtc/modules/remote_bitrate_estimator:remote_bitrate_estimator
> > > which should somehow be reachable. 
> > > 
> > > Should I try to solve them in this CL as well ?
> > 
> > That would be great. If it's just a matter of a few missing dependencies, it
> > would make sense to bake them into this one. If it grows - it could just as
> well
> > be landed as a follow-up.
> 
> Done

lgtm! Make sure to run at least a couple of compile trybots for PS#3, to be
sure.

[michaelt, 2017-04-06 09:35:34]

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

[commit-bot: I haz the power, 2017-04-06 09:35:37]

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

[terelius, 2017-04-06 09:39:37]

https://codereview.webrtc.org/2745473003/diff/40001/webrtc/logging/BUILD.gn
File webrtc/logging/BUILD.gn (right):

https://codereview.webrtc.org/2745473003/diff/40001/webrtc/logging/BUILD.gn#n...
webrtc/logging/BUILD.gn:32:
"../modules/remote_bitrate_estimator:remote_bitrate_estimator",
I don't like this. The event log API should not link with
remote_bitrate_estimator. Maybe change BandwidthUsage to an enum class and
forward declare it in the event log?

[michaelt, 2017-04-06 09:45:06]

Ok i think i will revert the last change and clean up BandwidthUsage problem in
a separate CL.
WDYT ?

[terelius, 2017-04-06 09:55:42]

On 2017/04/06 09:45:06, michaelt wrote:
> Ok i think i will revert the last change and clean up BandwidthUsage problem
in
> a separate CL.
> WDYT ?

I'm fine with that.

[kjellander_webrtc, 2017-04-06 09:56:15]

On 2017/04/06 09:55:42, terelius wrote:
> On 2017/04/06 09:45:06, michaelt wrote:
> > Ok i think i will revert the last change and clean up BandwidthUsage problem
> in
> > a separate CL.
> > WDYT ?
> 
> I'm fine with that.

sgtm, as long as it gets done

[michaelt, 2017-04-06 09:58:54]

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

[commit-bot: I haz the power, 2017-04-06 09:58:57]

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, 2017-04-06 10:30:42]

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

[commit-bot: I haz the power, 2017-04-06 10:30:44]

Dry run: This issue passed the CQ dry run.

[michaelt, 2017-04-06 12:54:53]

The CQ bit was checked by michaelt@webrtc.org

[michaelt, 2017-04-06 12:54:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, terelius@webrtc.org, minyue@webrtc.org,
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2745473003/#ps60001 (title:
"Revert "Activated checks for rtc_event_log_api"")

[commit-bot: I haz the power, 2017-04-06 12:55:15]

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-04-06 12:59:11]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491483293253970,
"parent_rev": "28dc285f2208ecbb3b79213ca8ccb608d46af654", "commit_rev":
"cde46b72788f5e37cfae33824b54d9c7d8a14ef8"}

[commit-bot: I haz the power, 2017-04-06 12:59:16]

Description was changed from

==========
Resolve cyclic dependency between audio network adaptor and event log api

BUG=webrtc:7257
==========

to

==========
Resolve cyclic dependency between audio network adaptor and event log api

BUG=webrtc:7257

Review-Url: https://codereview.webrtc.org/2745473003
Cr-Commit-Position: refs/heads/master@{#17565}
Committed:
https://chromium.googlesource.com/external/webrtc/+/cde46b72788f5e37cfae33824...
==========

[commit-bot: I haz the power, 2017-04-06 12:59:18]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/cde46b72788f5e37cfae33824...