Adding debug dump to audio network adaptor.

Adding debug dump to audio network adaptor.

BUG=webrtc:6303
Committed: https://crrev.com/7e4f8928062afc8d571bb69f3223711701cbaad6
Cr-Commit-Position: refs/heads/master@{#14361}

[minyue-webrtc, 2016-09-20 14:26:17]

Description was changed from

==========
on Karl's comments


revising


reformat


adding missing files


Adding BitrateController to audio network adaptor.


using -> of Optional


on Henrik's comments


removing unnecessary code for abstract class Controller


revising


removing unused api


adding include


adding include


adding include


formatting


ready to submit


roughly


Revert "cleaning up for commit 1"

This reverts commit e85ac75a7bde1be14753ec2d4dc1fbd32d0b66ba.

dtor inline


removing non-abstract class methods.


resolving constructor


adding include


correction


after review


adding copy ctor


cleaning up for commit 1


ready to commit


adding more


half way


Adding interface for AudioNetworkAdaptor.

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

to

==========
Adding debug dump to audio network adaptor.

BUG=webrtc:6303
==========

[minyue-webrtc, 2016-09-20 14:26:17]

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

[minyue-webrtc, 2016-09-20 14:38:48]

Patchset #1 (id:1) has been deleted

[minyue-webrtc, 2016-09-20 14:47:09]

Patchset #1 (id:20001) has been deleted

[minyue-webrtc, 2016-09-20 14:47:34]

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

[commit-bot: I haz the power, 2016-09-20 14:47:44]

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-20 14:48:03]

Hi Michael,

Could you take a look at this CL?

[commit-bot: I haz the power, 2016-09-20 14:49:14]

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

[commit-bot: I haz the power, 2016-09-20 14:49:15]

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

[minyue-webrtc, 2016-09-21 09:15:18]

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

[minyue-webrtc, 2016-09-21 09:16:30]

Hi Karl,

Michael has read the CL and had no major concerns. Now I add you to give another
look. Thanks!

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc
(right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:84:
debug_dump_writer_.reset(new DebugDumpWriterImpl(file_handle));
Michael suggested to add

#ifndef

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc
(right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc:46:
dump_file_->OpenFile(ana_dump_file_name.c_str(), false);
Per offline discussion, Michael suggested adding 

#ifndef WEBRTC_AUDIO_NETWORK_ADAPTOR_DEBUG_DUMP
  RTC_DCHECK(false);
#endif

That is, no DebugDumpWriterImpl should be created if protobuf is not allowed.

I agree. This will be added together with other comments, if any.

[kwiberg-webrtc, 2016-09-21 10:54:09]

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc
(right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:34:
}
You can make just one constructor with the debug dumper argument defaulting to
nullptr. (This is safe because the constructor isn't a virtual function; see
https://google.github.io/styleguide/cppguide.html#Default_Arguments .)

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:84:
debug_dump_writer_.reset(new DebugDumpWriterImpl(file_handle));
On 2016/09/21 09:16:30, minyue-webrtc wrote:
> Michael suggested to add
> 
> #ifndef 

Sounds reasonable. (A good way to ensure you put that in all the right places is
to #ifndef away the member variable.)

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h
(right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:28:
explicit Config(const Clock* clock);
I'd argue that this constructor isn't necessary; it kind of defeats the purpose
of having a config struct if you're going to populate it by a single constructor
call anyway...

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/controller.h (right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/controller.h:27:
rtc::Optional<int> rtt_ms;
Is there a good reason to add rtt in the same CL that adds debug dumping?

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc
(right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc:46:
dump_file_->OpenFile(ana_dump_file_name.c_str(), false);
On 2016/09/21 09:16:30, minyue-webrtc wrote:
> Per offline discussion, Michael suggested adding 
> 
> #ifndef WEBRTC_AUDIO_NETWORK_ADAPTOR_DEBUG_DUMP
>   RTC_DCHECK(false);
> #endif
> 
> That is, no DebugDumpWriterImpl should be created if protobuf is not allowed.
> 
> I agree. This will be added together with other comments, if any.

Not sure I understand how that code would give the effect you say. But I'll
review the code when you upload a new patch set.

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.h
(right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.h:34: };
Excellent with an interface class that's *just* an interface, and a separate
class for the implementation.

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.h:53: };
Unless you have a specific reason for wanting to expose the Impl class, I
suggest moving it to the .cc file, and exposing only a Create function here.
That makes the code easier to read, because you know right away that the Impl
class communicates with the rest of the world *only* through the DebugDumpWriter
interface.

[minyue-webrtc, 2016-09-22 15:11:49]

Hi Karl,

Thanks for the comments! Please take a look at the new patch.

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc
(right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:34:
}
On 2016/09/21 10:54:09, kwiberg-webrtc wrote:
> You can make just one constructor with the debug dumper argument defaulting to
> nullptr. (This is safe because the constructor isn't a virtual function; see
> https://google.github.io/styleguide/cppguide.html#Default_Arguments .)

Done.

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:84:
debug_dump_writer_.reset(new DebugDumpWriterImpl(file_handle));
On 2016/09/21 10:54:09, kwiberg-webrtc wrote:
> On 2016/09/21 09:16:30, minyue-webrtc wrote:
> > Michael suggested to add
> > 
> > #ifndef 
> 
> Sounds reasonable. (A good way to ensure you put that in all the right places
is
> to #ifndef away the member variable.)

After I added this comment I preferred adding this #ifndef in DebugDumpWriter.
see the new patch.

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h
(right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:28:
explicit Config(const Clock* clock);
On 2016/09/21 10:54:09, kwiberg-webrtc wrote:
> I'd argue that this constructor isn't necessary; it kind of defeats the
purpose
> of having a config struct if you're going to populate it by a single
constructor
> call anyway...

Ok. I can ask you about this offline.

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/controller.h (right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/controller.h:27:
rtc::Optional<int> rtt_ms;
On 2016/09/21 10:54:09, kwiberg-webrtc wrote:
> Is there a good reason to add rtt in the same CL that adds debug dumping?

This is not obvious. I should have explained it earlier. My apologies. This is
because it is (only) needed by the debug dump. It does not affect Controller's
behaviors under current implementation, but may be used by controllers in the
future.

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc
(right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc:46:
dump_file_->OpenFile(ana_dump_file_name.c_str(), false);
On 2016/09/21 10:54:09, kwiberg-webrtc wrote:
> On 2016/09/21 09:16:30, minyue-webrtc wrote:
> > Per offline discussion, Michael suggested adding 
> > 
> > #ifndef WEBRTC_AUDIO_NETWORK_ADAPTOR_DEBUG_DUMP
> >   RTC_DCHECK(false);
> > #endif
> > 
> > That is, no DebugDumpWriterImpl should be created if protobuf is not
allowed.
> > 
> > I agree. This will be added together with other comments, if any.
> 
> Not sure I understand how that code would give the effect you say. But I'll
> review the code when you upload a new patch set.

Done.

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.h
(right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.h:34: };
On 2016/09/21 10:54:09, kwiberg-webrtc wrote:
> Excellent with an interface class that's *just* an interface, and a separate
> class for the implementation.

:)

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.h:53: };
On 2016/09/21 10:54:09, kwiberg-webrtc wrote:
> Unless you have a specific reason for wanting to expose the Impl class, I
> suggest moving it to the .cc file, and exposing only a Create function here.
> That makes the code easier to read, because you know right away that the Impl
> class communicates with the rest of the world *only* through the
DebugDumpWriter
> interface.

Done.

[minyue-webrtc, 2016-09-22 15:20:49]

two additional self-comments on the new patch set.

https://codereview.webrtc.org/2356763002/diff/60001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h
(right):

https://codereview.webrtc.org/2356763002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:28:
explicit Config(const Clock* clock);
will follow your suggestion.

https://codereview.webrtc.org/2356763002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:40:
std::unique_ptr<ControllerManager>&& controller_manager,
will change it back.

[minyue-webrtc, 2016-09-22 15:31:35]

PS2 has some errors, if you have not started reviewing it, take PS3 instead.
Thanks!

[kwiberg-webrtc, 2016-09-22 15:54:16]

lgtm

Comments are optional.

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/controller.h (right):

https://codereview.webrtc.org/2356763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/controller.h:27:
rtc::Optional<int> rtt_ms;
On 2016/09/22 15:11:49, minyue-webrtc wrote:
> On 2016/09/21 10:54:09, kwiberg-webrtc wrote:
> > Is there a good reason to add rtt in the same CL that adds debug dumping?
> 
> This is not obvious. I should have explained it earlier. My apologies. This is
> because it is (only) needed by the debug dump. It does not affect Controller's
> behaviors under current implementation, but may be used by controllers in the
> future.

Acknowledged.

https://codereview.webrtc.org/2356763002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc
(right):

https://codereview.webrtc.org/2356763002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc:59:
RTC_DISALLOW_COPY_AND_ASSIGN(DebugDumpWriterImpl);
You can skip this line. Everyone will be handling your class with base class
pointers, so this line isn't needed (which is good, because it also doesn't do
anything, since it isn't in the .h file where the users can see it).

https://codereview.webrtc.org/2356763002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc:71:
DebugDumpWriterImpl::~DebugDumpWriterImpl() = default;
Now that this entire class is in the .cc file, there's no longer any reason to
avoid virtual inline methods. You may still find it more readable to have the
long methods non-inlined, but tiny ones like this don't have to be.

https://codereview.webrtc.org/2356763002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc:137:
return writer;
return std::unique_ptr<DebugDumpWriter>(new DebugDumpWriterImpl(file_handle));

https://codereview.webrtc.org/2356763002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.h
(right):

https://codereview.webrtc.org/2356763002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.h:26: //
Factory method. Constructor disabled.
This comment isn't really necessary. It's obvious that this is a factory method,
and it's also obvious that you can't call the DebugDumpWriter constructor
directly (because it has pure virtual methods).

[michaelt, 2016-09-22 15:56:09]

lgtm

[minyue-webrtc, 2016-09-22 17:50:36]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2016-09-22 17:50:36]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org,
michaelt@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2356763002/#ps100001 (title:
"rebasing")

[commit-bot: I haz the power, 2016-09-22 17:50:46]

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-22 17:52:18]

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

[commit-bot: I haz the power, 2016-09-22 17:52:19]

Try jobs failed on following builders:
  mac_gyp_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gyp_dbg/builds/886)
  mac_gyp_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gyp_rel/builds/889)

[minyue-webrtc, 2016-09-22 18:33:18]

Patchset #5 (id:120001) has been deleted

[minyue-webrtc, 2016-09-22 18:45:25]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2016-09-22 18:45:26]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org,
michaelt@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2356763002/#ps140001 (title:
"some fixing")

[commit-bot: I haz the power, 2016-09-22 18:45:28]

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-22 20:39:16]

Description was changed from

==========
Adding debug dump to audio network adaptor.

BUG=webrtc:6303
==========

to

==========
Adding debug dump to audio network adaptor.

BUG=webrtc:6303
==========

[commit-bot: I haz the power, 2016-09-22 20:39:17]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-09-22 20:39:23]

Description was changed from

==========
Adding debug dump to audio network adaptor.

BUG=webrtc:6303
==========

to

==========
Adding debug dump to audio network adaptor.

BUG=webrtc:6303

Committed: https://crrev.com/7e4f8928062afc8d571bb69f3223711701cbaad6
Cr-Commit-Position: refs/heads/master@{#14361}
==========

[commit-bot: I haz the power, 2016-09-22 20:39:25]

Patchset 5 (id:??) landed as
https://crrev.com/7e4f8928062afc8d571bb69f3223711701cbaad6
Cr-Commit-Position: refs/heads/master@{#14361}

[minyue-webrtc, 2016-09-22 20:40:32]

https://codereview.webrtc.org/2356763002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc
(right):

https://codereview.webrtc.org/2356763002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc:59:
RTC_DISALLOW_COPY_AND_ASSIGN(DebugDumpWriterImpl);
On 2016/09/22 15:54:16, kwiberg-webrtc wrote:
> You can skip this line. Everyone will be handling your class with base class
> pointers, so this line isn't needed (which is good, because it also doesn't do
> anything, since it isn't in the .h file where the users can see it).

Done.

https://codereview.webrtc.org/2356763002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc:71:
DebugDumpWriterImpl::~DebugDumpWriterImpl() = default;
On 2016/09/22 15:54:15, kwiberg-webrtc wrote:
> Now that this entire class is in the .cc file, there's no longer any reason to
> avoid virtual inline methods. You may still find it more readable to have the
> long methods non-inlined, but tiny ones like this don't have to be.

Done.

https://codereview.webrtc.org/2356763002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.cc:137:
return writer;
On 2016/09/22 15:54:15, kwiberg-webrtc wrote:
> return std::unique_ptr<DebugDumpWriter>(new DebugDumpWriterImpl(file_handle));

Done.

https://codereview.webrtc.org/2356763002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.h
(right):

https://codereview.webrtc.org/2356763002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/debug_dump_writer.h:26: //
Factory method. Constructor disabled.
On 2016/09/22 15:54:16, kwiberg-webrtc wrote:
> This comment isn't really necessary. It's obvious that this is a factory
method,
> and it's also obvious that you can't call the DebugDumpWriter constructor
> directly (because it has pure virtual methods).

Done.

[minyue-webrtc, 2016-09-22 21:16:44]

A revert of this CL (patchset #5 id:140001) has been created in
https://codereview.webrtc.org/2362003002/ by minyue@webrtc.org.

The reason for reverting is: Chromium bot fails.