Move AudioDecoder and related stuff to the api/ directory

Move AudioDecoder and related stuff to the api/ directory

BUG=webrtc:5805, webrtc:6725
Review-Url: https://codereview.webrtc.org/2668523004
Cr-Commit-Position: refs/heads/master@{#16534}
Committed: https://chromium.googlesource.com/external/webrtc/+/087bd34d23dee541915aae59781bf3ef033d8082

[kwiberg-webrtc, 2017-02-01 00:27:12]

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

[commit-bot: I haz the power, 2017-02-01 00:27:18]

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-02-01 00:29:07]

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

[commit-bot: I haz the power, 2017-02-01 00:29:08]

Dry run: 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...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_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/16993)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/16826)

[kwiberg-webrtc, 2017-02-01 00:32:49]

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

[kwiberg-webrtc, 2017-02-01 00:32:50]

Still slightly rough, but please have a look.

(The #includes aren't sorted yet. I'll do that once I know I'm not going to do
any further changes that wreck the order.)

(There's no backwards compat audio_decoder.h in the old place yet. I'll add one
as soon as the WebRTC bots are all green, to flush out any remaining in-tree
users.)

[kwiberg-webrtc, 2017-02-01 00:35:05]

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

[commit-bot: I haz the power, 2017-02-01 00:35:07]

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

[kwiberg-webrtc, 2017-02-01 00:35:44]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-02-01 00:38:17]

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

[commit-bot: I haz the power, 2017-02-01 00:38:18]

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

[kwiberg-webrtc, 2017-02-01 00:44:34]

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

[commit-bot: I haz the power, 2017-02-01 00:44:37]

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

[kwiberg-webrtc, 2017-02-01 00:45:04]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-02-01 01:42:27]

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

[commit-bot: I haz the power, 2017-02-01 01:42:28]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-02-01 10:46:21]

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

[commit-bot: I haz the power, 2017-02-01 10:46:27]

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-02-01 10:48:53]

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

[commit-bot: I haz the power, 2017-02-01 10:48:55]

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

[kwiberg-webrtc, 2017-02-01 11:21:30]

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

[commit-bot: I haz the power, 2017-02-01 11:21:40]

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

[kwiberg-webrtc, 2017-02-01 11:21:59]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-02-01 11:59:06]

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

[commit-bot: I haz the power, 2017-02-01 11:59:07]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-02-01 13:30:36]

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

[commit-bot: I haz the power, 2017-02-01 13:30:38]

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

[kwiberg-webrtc, 2017-02-01 13:31:07]

Patchset #3 (id:100001) has been deleted

[kwiberg-webrtc, 2017-02-01 13:53:20]

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

[commit-bot: I haz the power, 2017-02-01 13:53:25]

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

[kwiberg-webrtc, 2017-02-01 14:08:06]

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

[kwiberg-webrtc, 2017-02-01 14:08:07]

OK, ready for final review now!

Henrik, I'd like your input on the .gn changes if you don't mind.

[kwiberg-webrtc, 2017-02-01 14:09:16]

Description was changed from

==========
Move AudioDecoder and related stuff to the api/ directory

BUG=webrtc:5805
==========

to

==========
Move AudioDecoder and related stuff to the api/ directory

BUG=webrtc:5805, webrtc:6725
==========

[kjellander_webrtc, 2017-02-01 14:54:50]

I only looked at BUILD.gn files so far.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/DEPS
File webrtc/api/DEPS (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/DEPS#newcode15
webrtc/api/DEPS:15: # We allow .cc files in webrtc/api/ to #include a bunch of
stuff
Which are the cases for this? It sounds like those .cc files should live in
webrtc/modules/audio_coding

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
File webrtc/api/audio_codec/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
webrtc/api/audio_codec/BUILD.gn:15:
rtc_source_set("audio_decoder_factory_interface") {
Can we use _api suffix instead? It's the most commonly used in webrtc/api (with
the exception for call_interfaces).

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:2101: deps = [
I thought you might need public_deps to not break GN check on existing users,
but maybe that's not always the case.

[ossu, 2017-02-01 15:05:27]

Looks pretty good. Have a few comments, though.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
File webrtc/api/audio_codec/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
webrtc/api/audio_codec/BUILD.gn:15:
rtc_source_set("audio_decoder_factory_interface") {
General stuff-in-api/ question:

Should we add _interface to the end of targets, or should it be _api or should
we leave them just without a suffix? I believe I like suffix-less best; like how
generally don't suffix class interfaces with Interface either.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
webrtc/api/audio_codec/BUILD.gn:19: deps = [
Next general question:

Where do we draw the line between deps and public_deps?
Naïvely, I'd say audio_decoder_interface and audio_format should be public_deps.
Actually, most stuff in api/ depending on other stuff in api/ should probably be
public? Or have I misunderstood?

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.cc (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.cc:26: class EncFrame final : public
AudioDecoder::EncodedAudioFrame {
Please pick a better name, even if it's just temporary; like FallbackFrame or
something. Also make a note that it should go away when we remove the old-style
API of calling Decode directly on the audio decoder.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.h (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.h:27: enum SpeechType { kSpeech = 1,
kComfortNoise = 2 };
I think these should be left as-is, i.e. one-per-line.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/b...
File webrtc/api/audio_codec/builtin_audio_decoder_factory.cc (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/b...
webrtc/api/audio_codec/builtin_audio_decoder_factory.cc:18: return
InternalCreateBuiltinAudioDecoderFactory();
Have you seen my bike-shed? In it, I put Internal at the end of the name, rather
than at the beginning.
If I were to provide any reason for it, it might make it easier to find when
using an alphabetical index: CreateBuiltinAudioDecoderFactory{,Internal} will be
right beside each-other, whereas {Internal,}CreateBuiltinAudioDecoderFactory
would be far apart.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:2101: deps = [
I think you need public_deps for these, or gn check will shout at using things
not explicitly depended on. That's what I had to do when moving peerconnection
from api/ to pc/.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:2113: deps = [
Same here.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
File
webrtc/modules/audio_coding/codecs/internal_builtin_audio_decoder_factory.cc
(right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/internal_builtin_audio_decoder_factory.cc:184:
{ "useinbandfec",
This was highlighted in my CL that clang-formatted this line:
It can be just one line and without starting and ending whitespace.

[commit-bot: I haz the power, 2017-02-01 15:08:01]

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

[commit-bot: I haz the power, 2017-02-01 15:08:02]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-02-01 20:15:03]

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/DEPS
File webrtc/api/DEPS (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/DEPS#newcode15
webrtc/api/DEPS:15: # We allow .cc files in webrtc/api/ to #include a bunch of
stuff
On 2017/02/01 14:54:50, kjellander_webrtc wrote:
> Which are the cases for this? It sounds like those .cc files should live in
> webrtc/modules/audio_coding

Files in api/ must be allowed to include files outside api/---otherwise, we
might as well delete all the non-api/ files since they're dead code.

Letting .h files in api/ include non-api/ stuff is too risky, though (see the
comment added here), so restricting that to .cc files in api/ makes sense I
think.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
File webrtc/api/audio_codec/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
webrtc/api/audio_codec/BUILD.gn:15:
rtc_source_set("audio_decoder_factory_interface") {
On 2017/02/01 15:05:26, ossu wrote:
> General stuff-in-api/ question:
> 
> Should we add _interface to the end of targets, or should it be _api or should
> we leave them just without a suffix? I believe I like suffix-less best; like
how
> generally don't suffix class interfaces with Interface either. 

The full name of an api/ build target "foo" is "webrtc/api:foo" (or
"webrtc/api/subdir:foo", if it's in a subdirectory). It seems redundant to add
an "_api" suffix or prefix on top of that.

"_interface" is at least somewhat useful, since not all api/ build targets are
interfaces. I don't really have an opinion there.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
webrtc/api/audio_codec/BUILD.gn:19: deps = [
On 2017/02/01 15:05:26, ossu wrote:
> Next general question:
> 
> Where do we draw the line between deps and public_deps?
> Naïvely, I'd say audio_decoder_interface and audio_format should be
public_deps.
> Actually, most stuff in api/ depending on other stuff in api/ should probably
be
> public? Or have I misunderstood?

Hmm. That goes against my intuition (which is that if you want to #include a
header, you should depend on its build target---which means you never use
public_deps). I'll happily listen to arguments why I'm wrong, though.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.cc (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.cc:26: class EncFrame final : public
AudioDecoder::EncodedAudioFrame {
On 2017/02/01 15:05:26, ossu wrote:
> Please pick a better name, even if it's just temporary; like FallbackFrame or
> something. Also make a note that it should go away when we remove the
old-style
> API of calling Decode directly on the audio decoder.

Will "EncodedFrame" do? That's what it is, and what EncFrame was supposed to be
short for.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.h (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.h:27: enum SpeechType { kSpeech = 1,
kComfortNoise = 2 };
On 2017/02/01 15:05:26, ossu wrote:
> I think these should be left as-is, i.e. one-per-line.

Since I moved this file, "git cl format" considered every line changed and fair
game for reformatting, and I've long since come to the conclusion that fighting
git cl format simply isn't worth it.

Do you feel strongly about this particular case?

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:2101: deps = [
On 2017/02/01 15:05:26, ossu wrote:
> I think you need public_deps for these, or gn check will shout at using things
> not explicitly depended on. That's what I had to do when moving peerconnection
> from api/ to pc/.

No... unless I've misunderstood what public_deps means, I don't think it's
needed at all ever as long as people depend on the build targets that contain
the headers they include.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
File
webrtc/modules/audio_coding/codecs/internal_builtin_audio_decoder_factory.cc
(right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/internal_builtin_audio_decoder_factory.cc:184:
{ "useinbandfec",
On 2017/02/01 15:05:26, ossu wrote:
> This was highlighted in my CL that clang-formatted this line:
> It can be just one line and without starting and ending whitespace.

I don't disagree. But again, this line only got changed in this CL in the first
place because clang-format munches every line of moved files, and I let it
because I don't want to fight.

If you feel strongly about this particular case, I can fix it up manually and
look up the magic incantation that will cause clang-format to leave it alone.

[the sun, 2017-02-02 21:02:29]

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/BUILD.gn#newc...
webrtc/api/BUILD.gn:33: "audio_codec:audio_decoder_factory_interface",
nit: is this the preferred order? I'd have guessed audio_codec: would come
before the ".." targets.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
File webrtc/api/audio_codec/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
webrtc/api/audio_codec/BUILD.gn:15:
rtc_source_set("audio_decoder_factory_interface") {
On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> On 2017/02/01 15:05:26, ossu wrote:
> > General stuff-in-api/ question:
> > 
> > Should we add _interface to the end of targets, or should it be _api or
should
> > we leave them just without a suffix? I believe I like suffix-less best; like
> how
> > generally don't suffix class interfaces with Interface either. 
> 
> The full name of an api/ build target "foo" is "webrtc/api:foo" (or
> "webrtc/api/subdir:foo", if it's in a subdirectory). It seems redundant to add
> an "_api" suffix or prefix on top of that.
> 
> "_interface" is at least somewhat useful, since not all api/ build targets are
> interfaces. I don't really have an opinion there.

All good points, but let's stick to using the _api suffix as 
1) it is proposed in the design doc
2) already in use
3) distinguishing the source_set targets (which just provide the interface) from
the default implementations (which don't have any suffix) is a good thing.
4) it is shorter than _interface

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.cc (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.cc:13: #include <assert.h>
We should use DCHECK here.

But perhaps it is better to do some due cleanups in a follow up CL? This one is
large enough as it is...

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.cc:48: AudioDecoder* const decoder_;
So does the decoder own all its frames, so they are deleted once the decoder is?

If not, we have an ownership ambiguity here. If you won't fix in this CL, add a
TODO/file an issue.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.cc:134: return 0;
Remove as many default implementations of overridable methods as possible.
Forcing implementers to consider whether they need each function is more
important than what we possibly gain by providing defaults.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.h (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.h:78: // frames. Each frame must be between
10 ms and 120 ms long. The caller must
nit: it must be from 10 ms up to 120 ms long? can you create a separate issue to
go through the docs of this class? In fact, make it an issue about tidying up
this interface, if there isn't one already.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:60: #include
"webrtc/api/audio_codec/audio_decoder_factory.h"
Here's another bike shed: audio_codecs feels like a better name, since the
reason we have a subfolder is so we can put all the codecs (plural) in one
place.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:1018:
"../../api/audio_codec:builtin_audio_decoder_factory",
1. Why does NetEq depend on builtin_audio_decoder_factory? It seems to be used
in its unit tests, but not by NetEq as such (which is as it should be).

2. Since we include the builtin factory, why do we need to explicitly pull in
the above deps? We must already have them transitively. If I'm using the lib to
build my app I want this to be easy - just add the builtin decoder factory and I
shouldn't need to worry about adding whatever other targets it needs to build.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/utility/B...
File webrtc/modules/utility/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/utility/B...
webrtc/modules/utility/BUILD.gn:34:
"../../api/audio_codec:builtin_audio_decoder_factory",
Why needed here?

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/utility/B...
webrtc/modules/utility/BUILD.gn:39: "../audio_coding",
Looks like this and the other audio_coding deps can be removed?

[kwiberg-webrtc, 2017-02-03 09:47:53]

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

[commit-bot: I haz the power, 2017-02-03 09:47:58]

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

[kwiberg-webrtc, 2017-02-03 09:50:11]

New patch set uploaded.

(Note that there are still a bunch of outstanding issues where I asked about
stuff or objected. I've made no changes for those yet.)

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/BUILD.gn#newc...
webrtc/api/BUILD.gn:33: "audio_codec:audio_decoder_factory_interface",
On 2017/02/02 21:02:28, the sun wrote:
> nit: is this the preferred order? I'd have guessed audio_codec: would come
> before the ".." targets.

"gn format" sorts them this way. (And the precommit check makes sure that you
have run it, so unlike with C++ code formatting, you don't even have the option
of ignoring the tool. I must say I kind of like that---as long as the tool is
good enough, consistency is more important than the ability to manually tweak
things to perfection.)

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
File webrtc/api/audio_codec/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
webrtc/api/audio_codec/BUILD.gn:15:
rtc_source_set("audio_decoder_factory_interface") {
On 2017/02/02 21:02:28, the sun wrote:
> On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> > On 2017/02/01 15:05:26, ossu wrote:
> > > General stuff-in-api/ question:
> > > 
> > > Should we add _interface to the end of targets, or should it be _api or
> should
> > > we leave them just without a suffix? I believe I like suffix-less best;
like
> > how
> > > generally don't suffix class interfaces with Interface either. 
> > 
> > The full name of an api/ build target "foo" is "webrtc/api:foo" (or
> > "webrtc/api/subdir:foo", if it's in a subdirectory). It seems redundant to
add
> > an "_api" suffix or prefix on top of that.
> > 
> > "_interface" is at least somewhat useful, since not all api/ build targets
are
> > interfaces. I don't really have an opinion there.
> 
> All good points, but let's stick to using the _api suffix as 
> 1) it is proposed in the design doc
> 2) already in use

Ugh. I wish I'd noticed that in time...

> 3) distinguishing the source_set targets (which just provide the interface)
from
> the default implementations (which don't have any suffix) is a good thing.

Adding an _api suffix to all build targets won't accomplish that. Are you
suggesting that only one of source_set and static_library should have the
suffix? (And in that case, for what reason? As far as the target that depends on
them is concerned, there is no visible difference.)

[Looked at the design doc.] OK, so only the source_sets should end in _api.
Makes no sense to me, but I'll live with it rather than argue.

> 4) it is shorter than _interface

Yes, but that's irrelevant since no one was suggesting that as a suffix for all
build targets in api/. On the other hand, a point *against* the "_api" suffix is
that it's longer than the "" suffix...

But I'll add the suffix, in the interest of not letting an already painted bike
shed get in the way of progress.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.cc (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.cc:13: #include <assert.h>
On 2017/02/02 21:02:29, the sun wrote:
> We should use DCHECK here.
> 
> But perhaps it is better to do some due cleanups in a follow up CL? This one
is
> large enough as it is...

Yes, exactly. I'm (mostly) just moving this file, and it's usually a good idea
to not change a bunch of unrelated things in the same CL.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.cc:48: AudioDecoder* const decoder_;
On 2017/02/02 21:02:29, the sun wrote:
> So does the decoder own all its frames, so they are deleted once the decoder
is?
> 
> If not, we have an ownership ambiguity here. If you won't fix in this CL, add
a
> TODO/file an issue.

The docs for ParsePayload, which returns these, say "The caller must ensure that
the AudioDecoder object outlives any frame objects returned by this call." I'd
say that's clear enough.

The alternative is to force every AudioDecoder implementation to have
EncodedAudioFrame instances that can survive the demise of their AudioDecoder.
This makes them harder to implement correctly, and we (i.e. NetEq) have no need
for it.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.cc:134: return 0;
On 2017/02/02 21:02:29, the sun wrote:
> Remove as many default implementations of overridable methods as possible.
> Forcing implementers to consider whether they need each function is more
> important than what we possibly gain by providing defaults.

I agree. But not in this CL.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.h (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.h:78: // frames. Each frame must be between
10 ms and 120 ms long. The caller must
On 2017/02/02 21:02:29, the sun wrote:
> nit: it must be from 10 ms up to 120 ms long?

Yes, I believe that's what NetEq et al. expect.

> can you create a separate issue to
> go through the docs of this class? In fact, make it an issue about tidying up
> this interface, if there isn't one already.

I'll see if I can sit down with Oskar on Monday and enumerate all the problems
we can think of in a bug.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/b...
File webrtc/api/audio_codec/builtin_audio_decoder_factory.cc (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/b...
webrtc/api/audio_codec/builtin_audio_decoder_factory.cc:18: return
InternalCreateBuiltinAudioDecoderFactory();
On 2017/02/01 15:05:26, ossu wrote:
> Have you seen my bike-shed? In it, I put Internal at the end of the name,
rather
> than at the beginning.
> If I were to provide any reason for it, it might make it easier to find when
> using an alphabetical index: CreateBuiltinAudioDecoderFactory{,Internal} will
be
> right beside each-other, whereas {Internal,}CreateBuiltinAudioDecoderFactory
> would be far apart.

OK, I'll repaint my shed to match yours. :-)

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:60: #include
"webrtc/api/audio_codec/audio_decoder_factory.h"
On 2017/02/02 21:02:29, the sun wrote:
> Here's another bike shed: audio_codecs feels like a better name, since the
> reason we have a subfolder is so we can put all the codecs (plural) in one
> place.

OK, fine. As long as I don't have to do the renaming dance a fourth time...

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:1018:
"../../api/audio_codec:builtin_audio_decoder_factory",
On 2017/02/02 21:02:29, the sun wrote:
> 1. Why does NetEq depend on builtin_audio_decoder_factory? It seems to be used
> in its unit tests, but not by NetEq as such (which is as it should be).

It used to create a factory if it wasn't given one. We probably just forgot to
remove this dep when getting rid of the last call (we have no tool that finds
such errors). Fixed.

> 2. Since we include the builtin factory, why do we need to explicitly pull in
> the above deps? We must already have them transitively. If I'm using the lib
to
> build my app I want this to be easy - just add the builtin decoder factory and
I
> shouldn't need to worry about adding whatever other targets it needs to build.


I thought the idea was that for each file you #include, you declare a dependency
on its build target. That's what I've done here. (We could lessen the need for
this by using public_deps, at the cost of making it much more difficult to
figure out what targets you do and do not need to declare deps on. Bad
trade-off, IMO.)

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/utility/B...
File webrtc/modules/utility/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/utility/B...
webrtc/modules/utility/BUILD.gn:34:
"../../api/audio_codec:builtin_audio_decoder_factory",
On 2017/02/02 21:02:29, the sun wrote:
> Why needed here?

Not needed anymore. Removed.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/utility/B...
webrtc/modules/utility/BUILD.gn:39: "../audio_coding",
On 2017/02/02 21:02:29, the sun wrote:
> Looks like this and the other audio_coding deps can be removed?

Yes. Done.

[commit-bot: I haz the power, 2017-02-03 10:36:38]

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

[commit-bot: I haz the power, 2017-02-03 10:36:39]

Dry run: This issue passed the CQ dry run.

[ossu, 2017-02-06 12:54:16]

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/DEPS
File webrtc/api/DEPS (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/DEPS#newcode15
webrtc/api/DEPS:15: # We allow .cc files in webrtc/api/ to #include a bunch of
stuff
On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> On 2017/02/01 14:54:50, kjellander_webrtc wrote:
> > Which are the cases for this? It sounds like those .cc files should live in
> > webrtc/modules/audio_coding
> 
> Files in api/ must be allowed to include files outside api/---otherwise, we
> might as well delete all the non-api/ files since they're dead code.
> 
> Letting .h files in api/ include non-api/ stuff is too risky, though (see the
> comment added here), so restricting that to .cc files in api/ makes sense I
> think.

Yeah, if api/ is to be the one and only external access point to webrtc,
something like this is definitely necessary. The other option would be to have
interfaces etc. in api/, but have implementations accessed outside of api/. E.g.
api:audio_decoder_factory_api gives you the interfaces you need to talk about
audio decoder factories, but to get our implementation when you need to
instantiate such a factory, you'd depend on
audio_coding:builtin_audio_decoder_factory, for example. Though I expect that
option has already been discussed and rejected during the "Slim and modular"
discussions.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
File webrtc/api/audio_codec/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
webrtc/api/audio_codec/BUILD.gn:19: deps = [
On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> On 2017/02/01 15:05:26, ossu wrote:
> > Next general question:
> > 
> > Where do we draw the line between deps and public_deps?
> > Naïvely, I'd say audio_decoder_interface and audio_format should be
> public_deps.
> > Actually, most stuff in api/ depending on other stuff in api/ should
probably
> be
> > public? Or have I misunderstood?
> 
> Hmm. That goes against my intuition (which is that if you want to #include a
> header, you should depend on its build target---which means you never use
> public_deps). I'll happily listen to arguments why I'm wrong, though.

I don't really know. I was hoping kjellander@ would have an answer to this, as
he's also reviewing this CL. I thought of as what, generally, is the difference
between deps and public_deps. Maybe I should read the friendly manual? :) Now
that you mention it, though, it's also a project-specific question.

I was under the impression that transitively included headers would require
adding deps, but if that's not the case, then you're probably right. What I was
thinking was: if we're going for many small targets, it quickly becomes a chore
to keep your deps up-to-date. In the audio_decoder_factory example,
audio_decoder and audio format are both parts of the public API of
audio_decoder_factory. You can't really use the factory without knowing about
those headers. For a big target, it might be different. If audio_coding were a
single target, then it should probably be self-contained and not pull in
anything else, possibly also large, publically.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.cc (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.cc:26: class EncFrame final : public
AudioDecoder::EncodedAudioFrame {
On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> On 2017/02/01 15:05:26, ossu wrote:
> > Please pick a better name, even if it's just temporary; like FallbackFrame
or
> > something. Also make a note that it should go away when we remove the
> old-style
> > API of calling Decode directly on the audio decoder.
> 
> Will "EncodedFrame" do? That's what it is, and what EncFrame was supposed to
be
> short for.

No, I don't think that expresses what this is, really, and it's terribly close
to the EncodedAudioFrame interface that it implements. Aagh, naming is hard!

OldStyleEncodedFrame? OldApiEncodedFrame? And a little comment above it to
explain why it's there and that we hope to get rid of it soon?

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.cc:48: AudioDecoder* const decoder_;
On 2017/02/03 09:50:11, kwiberg-webrtc wrote:
> On 2017/02/02 21:02:29, the sun wrote:
> > So does the decoder own all its frames, so they are deleted once the decoder
> is?
> > 
> > If not, we have an ownership ambiguity here. If you won't fix in this CL,
add
> a
> > TODO/file an issue.
> 
> The docs for ParsePayload, which returns these, say "The caller must ensure
that
> the AudioDecoder object outlives any frame objects returned by this call." I'd
> say that's clear enough.
> 
> The alternative is to force every AudioDecoder implementation to have
> EncodedAudioFrame instances that can survive the demise of their AudioDecoder.
> This makes them harder to implement correctly, and we (i.e. NetEq) have no
need
> for it.

I'd argue that is unreasonable to have a frame outlive its decoder. If it were
hard to enforce these lifetimes, we _could_ allow decoders to be carried in
scoped_refptrs, which would allow frames to hold on to them until they were no
longer needed. I expect most codecs would have to have some shared state that
would have to be shared like this anyways. I'd strongly prefer if we could avoid
adding more shared ownership to the code and instead insist on whatever is using
the decoder to ensure it outlives its frames. It doesn't seem obviously
difficult to me.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.h (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.h:27: enum SpeechType { kSpeech = 1,
kComfortNoise = 2 };
On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> On 2017/02/01 15:05:26, ossu wrote:
> > I think these should be left as-is, i.e. one-per-line.
> 
> Since I moved this file, "git cl format" considered every line changed and
fair
> game for reformatting, and I've long since come to the conclusion that
fighting
> git cl format simply isn't worth it.
> 
> Do you feel strongly about this particular case?

Yeah, I figured that might've been the case.
I've not yet reached that same conclusion. clang-format is a great tool, but I
don't think it should be the ultimate decider on what's more readable. In this
case, you haven't actually changed this enum, so it's especially unnecessary to
change its formatting.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.h:78: // frames. Each frame must be between
10 ms and 120 ms long. The caller must
On 2017/02/03 09:50:11, kwiberg-webrtc wrote:
> On 2017/02/02 21:02:29, the sun wrote:
> > nit: it must be from 10 ms up to 120 ms long?
> 
> Yes, I believe that's what NetEq et al. expect.
> 
> > can you create a separate issue to
> > go through the docs of this class? In fact, make it an issue about tidying
up
> > this interface, if there isn't one already.
> 
> I'll see if I can sit down with Oskar on Monday and enumerate all the problems
> we can think of in a bug.

Wait, what?

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
File
webrtc/modules/audio_coding/codecs/internal_builtin_audio_decoder_factory.cc
(right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/internal_builtin_audio_decoder_factory.cc:184:
{ "useinbandfec",
On 2017/02/01 20:15:03, kwiberg-webrtc wrote:
> On 2017/02/01 15:05:26, ossu wrote:
> > This was highlighted in my CL that clang-formatted this line:
> > It can be just one line and without starting and ending whitespace.
> 
> I don't disagree. But again, this line only got changed in this CL in the
first
> place because clang-format munches every line of moved files, and I let it
> because I don't want to fight.
> 
> If you feel strongly about this particular case, I can fix it up manually and
> look up the magic incantation that will cause clang-format to leave it alone.

I do feel quite strongly about this case. clang-format cannot decide if what's
represented here is a list or key-value pair. Since it's the latter, I think
keeping them both on the same line is more important than it would otherwise be.
Make sure the extra spaces get removed as well, I fixed this line up but only
partially and had to go back afterwards[1].

[1]
https://codereview.webrtc.org/2669583002/diff2/1:20001/webrtc/modules/audio_c...

[kwiberg-webrtc, 2017-02-06 14:19:36]

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
File webrtc/api/audio_codec/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/B...
webrtc/api/audio_codec/BUILD.gn:19: deps = [
On 2017/02/06 12:54:16, ossu wrote:
> On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> > On 2017/02/01 15:05:26, ossu wrote:
> > > Next general question:
> > > 
> > > Where do we draw the line between deps and public_deps?
> > > Naïvely, I'd say audio_decoder_interface and audio_format should be
> > public_deps.
> > > Actually, most stuff in api/ depending on other stuff in api/ should
> probably
> > be
> > > public? Or have I misunderstood?
> > 
> > Hmm. That goes against my intuition (which is that if you want to #include a
> > header, you should depend on its build target---which means you never use
> > public_deps). I'll happily listen to arguments why I'm wrong, though.
> 
> I don't really know. I was hoping kjellander@ would have an answer to this, as
> he's also reviewing this CL. I thought of as what, generally, is the
difference
> between deps and public_deps. Maybe I should read the friendly manual? :) Now
> that you mention it, though, it's also a project-specific question.
> 
> I was under the impression that transitively included headers would require
> adding deps, but if that's not the case, then you're probably right.

I'm really sure it's not---that would be madness. public_deps is used by targets
that want to export *other* targets' include files, IIUC; in other words, if you
want to take a bunch of smaller targets and logically bundle them together so
that they'll be easier to depend on, without actually merging them into one big
target, then make a new target that public_deps on all the targets whose headers
it should be re-exporting.

> What I was
> thinking was: if we're going for many small targets, it quickly becomes a
chore
> to keep your deps up-to-date. In the audio_decoder_factory example,
> audio_decoder and audio format are both parts of the public API of
> audio_decoder_factory. You can't really use the factory without knowing about
> those headers. For a big target, it might be different. If audio_coding were a
> single target, then it should probably be self-contained and not pull in
> anything else, possibly also large, publically.

I discussed this with solenberg@, who had much the same objections. We decided
that I should follow his idea of making bigger targets for "light-weight" stuff
(i.e. stuff with transitively little or no .cc content) and continue to have
separate targets for "heavy-weight" stuff (i.e. stuff with transitively lots of
.cc content).

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.cc (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.cc:26: class EncFrame final : public
AudioDecoder::EncodedAudioFrame {
On 2017/02/06 12:54:16, ossu wrote:
> On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> > On 2017/02/01 15:05:26, ossu wrote:
> > > Please pick a better name, even if it's just temporary; like FallbackFrame
> or
> > > something. Also make a note that it should go away when we remove the
> > old-style
> > > API of calling Decode directly on the audio decoder.
> > 
> > Will "EncodedFrame" do? That's what it is, and what EncFrame was supposed to
> be
> > short for.
> 
> No, I don't think that expresses what this is, really, and it's terribly close
> to the EncodedAudioFrame interface that it implements. Aagh, naming is hard!
> 
> OldStyleEncodedFrame? OldApiEncodedFrame? And a little comment above it to
> explain why it's there and that we hope to get rid of it soon?

OK, I'll pick a name like that. Maybe OldStyle* or Legacy*.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.h (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.h:27: enum SpeechType { kSpeech = 1,
kComfortNoise = 2 };
On 2017/02/06 12:54:16, ossu wrote:
> On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> > On 2017/02/01 15:05:26, ossu wrote:
> > > I think these should be left as-is, i.e. one-per-line.
> > 
> > Since I moved this file, "git cl format" considered every line changed and
> fair
> > game for reformatting, and I've long since come to the conclusion that
> fighting
> > git cl format simply isn't worth it.
> > 
> > Do you feel strongly about this particular case?
> 
> Yeah, I figured that might've been the case.
> I've not yet reached that same conclusion. clang-format is a great tool, but I
> don't think it should be the ultimate decider on what's more readable. In this
> case, you haven't actually changed this enum, so it's especially unnecessary
to
> change its formatting.

Fair enough. If a human really cares, that human should get to decide...

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.h:78: // frames. Each frame must be between
10 ms and 120 ms long. The caller must
On 2017/02/06 12:54:16, ossu wrote:
> On 2017/02/03 09:50:11, kwiberg-webrtc wrote:
> > On 2017/02/02 21:02:29, the sun wrote:
> > > nit: it must be from 10 ms up to 120 ms long?
> > 
> > Yes, I believe that's what NetEq et al. expect.
> > 
> > > can you create a separate issue to
> > > go through the docs of this class? In fact, make it an issue about tidying
> up
> > > this interface, if there isn't one already.
> > 
> > I'll see if I can sit down with Oskar on Monday and enumerate all the
problems
> > we can think of in a bug.
> 
> Wait, what?

I think we've probably already talked about most of this stuff in some detail at
least twice, so it'd probably save time if we just sat down and documented
what's still wrong with it. The worst warts are already gone, I think, but there
are some left.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
File
webrtc/modules/audio_coding/codecs/internal_builtin_audio_decoder_factory.cc
(right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/internal_builtin_audio_decoder_factory.cc:184:
{ "useinbandfec",
On 2017/02/06 12:54:16, ossu wrote:
> On 2017/02/01 20:15:03, kwiberg-webrtc wrote:
> > On 2017/02/01 15:05:26, ossu wrote:
> > > This was highlighted in my CL that clang-formatted this line:
> > > It can be just one line and without starting and ending whitespace.
> > 
> > I don't disagree. But again, this line only got changed in this CL in the
> first
> > place because clang-format munches every line of moved files, and I let it
> > because I don't want to fight.
> > 
> > If you feel strongly about this particular case, I can fix it up manually
and
> > look up the magic incantation that will cause clang-format to leave it
alone.
> 
> I do feel quite strongly about this case. clang-format cannot decide if what's
> represented here is a list or key-value pair. Since it's the latter, I think
> keeping them both on the same line is more important than it would otherwise
be.
> Make sure the extra spaces get removed as well, I fixed this line up but only
> partially and had to go back afterwards[1].
> 
> [1]
>
https://codereview.webrtc.org/2669583002/diff2/1:20001/webrtc/modules/audio_c...
> 

OK, I'll make sure this is lovingly hand-formatted...

[ossu, 2017-02-06 14:44:07]

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.cc (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.cc:26: class EncFrame final : public
AudioDecoder::EncodedAudioFrame {
On 2017/02/06 14:19:36, kwiberg-webrtc wrote:
> On 2017/02/06 12:54:16, ossu wrote:
> > On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> > > On 2017/02/01 15:05:26, ossu wrote:
> > > > Please pick a better name, even if it's just temporary; like
FallbackFrame
> > or
> > > > something. Also make a note that it should go away when we remove the
> > > old-style
> > > > API of calling Decode directly on the audio decoder.
> > > 
> > > Will "EncodedFrame" do? That's what it is, and what EncFrame was supposed
to
> > be
> > > short for.
> > 
> > No, I don't think that expresses what this is, really, and it's terribly
close
> > to the EncodedAudioFrame interface that it implements. Aagh, naming is hard!
> > 
> > OldStyleEncodedFrame? OldApiEncodedFrame? And a little comment above it to
> > explain why it's there and that we hope to get rid of it soon?
> 
> OK, I'll pick a name like that. Maybe OldStyle* or Legacy*.

Legacy might clash (at least mentally) with the other LegacyEncodedAudioFrame,
although that may be alright since they're very alike. OldStyle won't, but it
might also strange that we have such similar functionality under different
names... I'm not sure this is helping. :)

Pick one for now and we'll put this effort towards getting rid of it instead.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.h (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.h:27: enum SpeechType { kSpeech = 1,
kComfortNoise = 2 };
On 2017/02/06 14:19:36, kwiberg-webrtc wrote:
> On 2017/02/06 12:54:16, ossu wrote:
> > On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> > > On 2017/02/01 15:05:26, ossu wrote:
> > > > I think these should be left as-is, i.e. one-per-line.
> > > 
> > > Since I moved this file, "git cl format" considered every line changed and
> > fair
> > > game for reformatting, and I've long since come to the conclusion that
> > fighting
> > > git cl format simply isn't worth it.
> > > 
> > > Do you feel strongly about this particular case?
> > 
> > Yeah, I figured that might've been the case.
> > I've not yet reached that same conclusion. clang-format is a great tool, but
I
> > don't think it should be the ultimate decider on what's more readable. In
this
> > case, you haven't actually changed this enum, so it's especially unnecessary
> to
> > change its formatting.
> 
> Fair enough. If a human really cares, that human should get to decide...

Thanks! :)

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.h:78: // frames. Each frame must be between
10 ms and 120 ms long. The caller must
On 2017/02/06 14:19:36, kwiberg-webrtc wrote:
> On 2017/02/06 12:54:16, ossu wrote:
> > On 2017/02/03 09:50:11, kwiberg-webrtc wrote:
> > > On 2017/02/02 21:02:29, the sun wrote:
> > > > nit: it must be from 10 ms up to 120 ms long?
> > > 
> > > Yes, I believe that's what NetEq et al. expect.
> > > 
> > > > can you create a separate issue to
> > > > go through the docs of this class? In fact, make it an issue about
tidying
> > up
> > > > this interface, if there isn't one already.
> > > 
> > > I'll see if I can sit down with Oskar on Monday and enumerate all the
> problems
> > > we can think of in a bug.
> > 
> > Wait, what?
> 
> I think we've probably already talked about most of this stuff in some detail
at
> least twice, so it'd probably save time if we just sat down and documented
> what's still wrong with it. The worst warts are already gone, I think, but
there
> are some left.

Oh, right, yes, I agree. I was thinking this was referring to the new
EncodedAudioFrame interface, for some reason - context is sometimes difficult in
rietveld. AudioDecoder definitely needs tidying up. Let's have a proper
in-person meeting once I get back. Perhaps schedule something tentatively for
thursday?

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
File
webrtc/modules/audio_coding/codecs/internal_builtin_audio_decoder_factory.cc
(right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/internal_builtin_audio_decoder_factory.cc:184:
{ "useinbandfec",
On 2017/02/06 14:19:36, kwiberg-webrtc wrote:
> On 2017/02/06 12:54:16, ossu wrote:
> > On 2017/02/01 20:15:03, kwiberg-webrtc wrote:
> > > On 2017/02/01 15:05:26, ossu wrote:
> > > > This was highlighted in my CL that clang-formatted this line:
> > > > It can be just one line and without starting and ending whitespace.
> > > 
> > > I don't disagree. But again, this line only got changed in this CL in the
> > first
> > > place because clang-format munches every line of moved files, and I let it
> > > because I don't want to fight.
> > > 
> > > If you feel strongly about this particular case, I can fix it up manually
> and
> > > look up the magic incantation that will cause clang-format to leave it
> alone.
> > 
> > I do feel quite strongly about this case. clang-format cannot decide if
what's
> > represented here is a list or key-value pair. Since it's the latter, I think
> > keeping them both on the same line is more important than it would otherwise
> be.
> > Make sure the extra spaces get removed as well, I fixed this line up but
only
> > partially and had to go back afterwards[1].
> > 
> > [1]
> >
>
https://codereview.webrtc.org/2669583002/diff2/1:20001/webrtc/modules/audio_c...
> > 
> 
> OK, I'll make sure this is lovingly hand-formatted...

Artisanal lines of code - I like it!

[kwiberg-webrtc, 2017-02-09 14:11:37]

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

[commit-bot: I haz the power, 2017-02-09 14:11:41]

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-02-09 14:13:51]

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

[commit-bot: I haz the power, 2017-02-09 14:13:52]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17133)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21326)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/22577)

[kwiberg-webrtc, 2017-02-09 14:20:31]

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

[commit-bot: I haz the power, 2017-02-09 14:20:41]

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

[kwiberg-webrtc, 2017-02-09 14:21:02]

Patchset #8 (id:220001) has been deleted

[commit-bot: I haz the power, 2017-02-09 15:32:16]

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

[commit-bot: I haz the power, 2017-02-09 15:32:17]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-02-09 20:08:30]

I think patch set #8 fixes all issues that we've discussed.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
File webrtc/api/audio_codec/audio_decoder.h (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/audio_codec/a...
webrtc/api/audio_codec/audio_decoder.h:27: enum SpeechType { kSpeech = 1,
kComfortNoise = 2 };
On 2017/02/06 14:44:06, ossu wrote:
> On 2017/02/06 14:19:36, kwiberg-webrtc wrote:
> > On 2017/02/06 12:54:16, ossu wrote:
> > > On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> > > > On 2017/02/01 15:05:26, ossu wrote:
> > > > > I think these should be left as-is, i.e. one-per-line.
> > > > 
> > > > Since I moved this file, "git cl format" considered every line changed
and
> > > fair
> > > > game for reformatting, and I've long since come to the conclusion that
> > > fighting
> > > > git cl format simply isn't worth it.
> > > > 
> > > > Do you feel strongly about this particular case?
> > > 
> > > Yeah, I figured that might've been the case.
> > > I've not yet reached that same conclusion. clang-format is a great tool,
but
> I
> > > don't think it should be the ultimate decider on what's more readable. In
> this
> > > case, you haven't actually changed this enum, so it's especially
unnecessary
> > to
> > > change its formatting.
> > 
> > Fair enough. If a human really cares, that human should get to decide...
> 
> Thanks! :)

Aha! If I add a trailing comma just before the closing brace, clang-format
thinks it should be on separate lines, but without the trailing comma, it wants
it all on one line. Makes some sort of sense. I'll add the trailing comma.

[kjellander_webrtc, 2017-02-10 07:58:48]

lgtm if you can address the downstream breakages due to the API changes.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/DEPS
File webrtc/api/DEPS (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/DEPS#newcode15
webrtc/api/DEPS:15: # We allow .cc files in webrtc/api/ to #include a bunch of
stuff
On 2017/02/06 12:54:16, ossu wrote:
> On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> > On 2017/02/01 14:54:50, kjellander_webrtc wrote:
> > > Which are the cases for this? It sounds like those .cc files should live
in
> > > webrtc/modules/audio_coding
> > 
> > Files in api/ must be allowed to include files outside api/---otherwise, we
> > might as well delete all the non-api/ files since they're dead code.
> > 
> > Letting .h files in api/ include non-api/ stuff is too risky, though (see
the
> > comment added here), so restricting that to .cc files in api/ makes sense I
> > think.
> 
> Yeah, if api/ is to be the one and only external access point to webrtc,
> something like this is definitely necessary. The other option would be to have
> interfaces etc. in api/, but have implementations accessed outside of api/.
E.g.
> api:audio_decoder_factory_api gives you the interfaces you need to talk about
> audio decoder factories, but to get our implementation when you need to
> instantiate such a factory, you'd depend on
> audio_coding:builtin_audio_decoder_factory, for example. Though I expect that
> option has already been discussed and rejected during the "Slim and modular"
> discussions. 

Thanks for explaining. Let's do this.

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:2101: deps = [
On 2017/02/01 20:15:03, kwiberg-webrtc wrote:
> On 2017/02/01 15:05:26, ossu wrote:
> > I think you need public_deps for these, or gn check will shout at using
things
> > not explicitly depended on. That's what I had to do when moving
peerconnection
> > from api/ to pc/.
> 
> No... unless I've misunderstood what public_deps means, I don't think it's
> needed at all ever as long as people depend on the build targets that contain
> the headers they include.

That's correct, as long as they include the right targets it's fine (if GN check
is enabled for them, otherwise it doesn't really matter).

https://codereview.webrtc.org/2668523004/diff/240001/webrtc/api/audio_codecs/...
File webrtc/api/audio_codecs/builtin_audio_decoder_factory.h (right):

https://codereview.webrtc.org/2668523004/diff/240001/webrtc/api/audio_codecs/...
webrtc/api/audio_codecs/builtin_audio_decoder_factory.h:2: *  Copyright (c) 2017
The WebRTC project authors. All Rights Reserved.
Year shouldn't actually change if you're just moving a file. Only set to 2017
for new files.

[kwiberg-webrtc, 2017-02-10 09:15:52]

https://codereview.webrtc.org/2668523004/diff/240001/webrtc/api/audio_codecs/...
File webrtc/api/audio_codecs/builtin_audio_decoder_factory.h (right):

https://codereview.webrtc.org/2668523004/diff/240001/webrtc/api/audio_codecs/...
webrtc/api/audio_codecs/builtin_audio_decoder_factory.h:2: *  Copyright (c) 2017
The WebRTC project authors. All Rights Reserved.
On 2017/02/10 07:58:48, kjellander_webrtc wrote:
> Year shouldn't actually change if you're just moving a file. Only set to 2017
> for new files.

This is a new file. It just looks very similar to an existing file (because all
files have identical boilerplate, and I moved two lines of comment and one line
of code from there to here), and git noticed.

But it's an arbitrary distinction. I can change it if you want...

[kwiberg-webrtc, 2017-02-10 09:19:57]

On 2017/02/10 07:58:48, kjellander_webrtc wrote:
> lgtm if you can address the downstream breakages due to the API changes.

Yes---If I understood you correctly, I needed to manually add a dependency. I'll
prepare a CL for that, to land right after this one.

> https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/DEPS
> File webrtc/api/DEPS (right):
> 
> https://codereview.webrtc.org/2668523004/diff/140001/webrtc/api/DEPS#newcode15
> webrtc/api/DEPS:15: # We allow .cc files in webrtc/api/ to #include a bunch of
> stuff
> On 2017/02/06 12:54:16, ossu wrote:
> > On 2017/02/01 20:15:02, kwiberg-webrtc wrote:
> > > On 2017/02/01 14:54:50, kjellander_webrtc wrote:
> > > > Which are the cases for this? It sounds like those .cc files should live
> in
> > > > webrtc/modules/audio_coding
> > > 
> > > Files in api/ must be allowed to include files outside api/---otherwise,
we
> > > might as well delete all the non-api/ files since they're dead code.
> > > 
> > > Letting .h files in api/ include non-api/ stuff is too risky, though (see
> the
> > > comment added here), so restricting that to .cc files in api/ makes sense
I
> > > think.
> > 
> > Yeah, if api/ is to be the one and only external access point to webrtc,
> > something like this is definitely necessary. The other option would be to
have
> > interfaces etc. in api/, but have implementations accessed outside of api/.
> E.g.
> > api:audio_decoder_factory_api gives you the interfaces you need to talk
about
> > audio decoder factories, but to get our implementation when you need to
> > instantiate such a factory, you'd depend on
> > audio_coding:builtin_audio_decoder_factory, for example. Though I expect
that
> > option has already been discussed and rejected during the "Slim and modular"
> > discussions. 
> 
> Thanks for explaining. Let's do this.
> 
>
https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2668523004/diff/140001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/BUILD.gn:2101: deps = [
> On 2017/02/01 20:15:03, kwiberg-webrtc wrote:
> > On 2017/02/01 15:05:26, ossu wrote:
> > > I think you need public_deps for these, or gn check will shout at using
> things
> > > not explicitly depended on. That's what I had to do when moving
> peerconnection
> > > from api/ to pc/.
> > 
> > No... unless I've misunderstood what public_deps means, I don't think it's
> > needed at all ever as long as people depend on the build targets that
contain
> > the headers they include.
> 
> That's correct, as long as they include the right targets it's fine (if GN
check
> is enabled for them, otherwise it doesn't really matter).
> 
>
https://codereview.webrtc.org/2668523004/diff/240001/webrtc/api/audio_codecs/...
> File webrtc/api/audio_codecs/builtin_audio_decoder_factory.h (right):
> 
>
https://codereview.webrtc.org/2668523004/diff/240001/webrtc/api/audio_codecs/...
> webrtc/api/audio_codecs/builtin_audio_decoder_factory.h:2: *  Copyright (c)
2017
> The WebRTC project authors. All Rights Reserved.
> Year shouldn't actually change if you're just moving a file. Only set to 2017
> for new files.

[kjellander_webrtc, 2017-02-10 09:37:45]

On 2017/02/10 09:15:52, kwiberg-webrtc wrote:
>
https://codereview.webrtc.org/2668523004/diff/240001/webrtc/api/audio_codecs/...
> File webrtc/api/audio_codecs/builtin_audio_decoder_factory.h (right):
> 
>
https://codereview.webrtc.org/2668523004/diff/240001/webrtc/api/audio_codecs/...
> webrtc/api/audio_codecs/builtin_audio_decoder_factory.h:2: *  Copyright (c)
2017
> The WebRTC project authors. All Rights Reserved.
> On 2017/02/10 07:58:48, kjellander_webrtc wrote:
> > Year shouldn't actually change if you're just moving a file. Only set to
2017
> > for new files.
> 
> This is a new file. It just looks very similar to an existing file (because
all
> files have identical boilerplate, and I moved two lines of comment and one
line
> of code from there to here), and git noticed.
> 
> But it's an arbitrary distinction. I can change it if you want...

No, sorry I probably just didn't enough attention, my bad.

[kwiberg-webrtc, 2017-02-10 09:42:29]

On 2017/02/10 09:37:45, kjellander_webrtc wrote:
> On 2017/02/10 09:15:52, kwiberg-webrtc wrote:
> >
>
https://codereview.webrtc.org/2668523004/diff/240001/webrtc/api/audio_codecs/...
> > File webrtc/api/audio_codecs/builtin_audio_decoder_factory.h (right):
> > 
> >
>
https://codereview.webrtc.org/2668523004/diff/240001/webrtc/api/audio_codecs/...
> > webrtc/api/audio_codecs/builtin_audio_decoder_factory.h:2: *  Copyright (c)
> 2017
> > The WebRTC project authors. All Rights Reserved.
> > On 2017/02/10 07:58:48, kjellander_webrtc wrote:
> > > Year shouldn't actually change if you're just moving a file. Only set to
> 2017
> > > for new files.
> > 
> > This is a new file. It just looks very similar to an existing file (because
> all
> > files have identical boilerplate, and I moved two lines of comment and one
> line
> > of code from there to here), and git noticed.
> > 
> > But it's an arbitrary distinction. I can change it if you want...
> 
> No, sorry I probably just didn't enough attention, my bad.

Well, enoughing attention isn't always easy, and there *was* a point favoring
your side of the argument: All the non-boilerplate lines in the new file were
moved from the old file git found. So I changed it to 2016 like you suggested.

[the sun, 2017-02-10 10:05:08]

lgtm

https://codereview.webrtc.org/2668523004/diff/260001/webrtc/api/audio_codecs/...
File webrtc/api/audio_codecs/audio_decoder_factory.h (right):

https://codereview.webrtc.org/2668523004/diff/260001/webrtc/api/audio_codecs/...
webrtc/api/audio_codecs/audio_decoder_factory.h:19: #include
"webrtc/base/atomicops.h"
why?

https://codereview.webrtc.org/2668523004/diff/260001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc
(right):

https://codereview.webrtc.org/2668523004/diff/260001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc:2: * 
Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
This file should be renamed IMO - it is testing the building ADF, not the
generic interface

[the sun, 2017-02-10 10:05:36]

On 2017/02/10 10:05:08, the sun wrote:
> lgtm
> 
>
https://codereview.webrtc.org/2668523004/diff/260001/webrtc/api/audio_codecs/...
> File webrtc/api/audio_codecs/audio_decoder_factory.h (right):
> 
>
https://codereview.webrtc.org/2668523004/diff/260001/webrtc/api/audio_codecs/...
> webrtc/api/audio_codecs/audio_decoder_factory.h:19: #include
> "webrtc/base/atomicops.h"
> why?
> 
>
https://codereview.webrtc.org/2668523004/diff/260001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc
> (right):
> 
>
https://codereview.webrtc.org/2668523004/diff/260001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc:2: * 
> Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
> This file should be renamed IMO - it is testing the building ADF, not the
> generic interface

s/building/builtin/g :)

[kwiberg-webrtc, 2017-02-10 11:31:24]

New patch set uploaded.

https://codereview.webrtc.org/2668523004/diff/260001/webrtc/api/audio_codecs/...
File webrtc/api/audio_codecs/audio_decoder_factory.h (right):

https://codereview.webrtc.org/2668523004/diff/260001/webrtc/api/audio_codecs/...
webrtc/api/audio_codecs/audio_decoder_factory.h:19: #include
"webrtc/base/atomicops.h"
On 2017/02/10 10:05:07, the sun wrote:
> why?

I guess it was used for reference counting somehow, but not anymore obviously.
I'll remove it.

https://codereview.webrtc.org/2668523004/diff/260001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc
(right):

https://codereview.webrtc.org/2668523004/diff/260001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc:2: * 
Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
On 2017/02/10 10:05:07, the sun wrote:
> This file should be renamed IMO - it is testing the building ADF, not the
> generic interface

Good point. Done.

[the sun, 2017-02-10 11:55:25]

On 2017/02/10 11:31:24, kwiberg-webrtc wrote:
> New patch set uploaded.
> 
>
https://codereview.webrtc.org/2668523004/diff/260001/webrtc/api/audio_codecs/...
> File webrtc/api/audio_codecs/audio_decoder_factory.h (right):
> 
>
https://codereview.webrtc.org/2668523004/diff/260001/webrtc/api/audio_codecs/...
> webrtc/api/audio_codecs/audio_decoder_factory.h:19: #include
> "webrtc/base/atomicops.h"
> On 2017/02/10 10:05:07, the sun wrote:
> > why?
> 
> I guess it was used for reference counting somehow, but not anymore obviously.
> I'll remove it.
> 
>
https://codereview.webrtc.org/2668523004/diff/260001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc
> (right):
> 
>
https://codereview.webrtc.org/2668523004/diff/260001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc:2: * 
> Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
> On 2017/02/10 10:05:07, the sun wrote:
> > This file should be renamed IMO - it is testing the building ADF, not the
> > generic interface
> 
> Good point. Done.

still lgtm

[ossu, 2017-02-10 13:27:29]

LGTM!

[kwiberg-webrtc, 2017-02-10 13:28:58]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2017-02-10 13:28:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2668523004/#ps280001 (title:
"more review fixes")

[commit-bot: I haz the power, 2017-02-10 13:29:07]

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-02-10 16:15:47]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1486733338055540,
"parent_rev": "84a3759825ec3bcba104196148be15aabc258a08", "commit_rev":
"087bd34d23dee541915aae59781bf3ef033d8082"}

[commit-bot: I haz the power, 2017-02-10 16:15:51]

Description was changed from

==========
Move AudioDecoder and related stuff to the api/ directory

BUG=webrtc:5805, webrtc:6725
==========

to

==========
Move AudioDecoder and related stuff to the api/ directory

BUG=webrtc:5805, webrtc:6725

Review-Url: https://codereview.webrtc.org/2668523004
Cr-Commit-Position: refs/heads/master@{#16534}
Committed:
https://chromium.googlesource.com/external/webrtc/+/087bd34d23dee541915aae597...
==========

[commit-bot: I haz the power, 2017-02-10 16:15:53]

Committed patchset #10 (id:280001) as
https://chromium.googlesource.com/external/webrtc/+/087bd34d23dee541915aae597...