Moved codec-specific audio packet splitting into decoders.

Moved codec-specific audio packet splitting into decoders.

There's still some code run specifically for Opus w/ FEC. Will
investigate on how to move that as well.

Closed in favor of https://codereview.webrtc.org/2326003002, which solves the same problem more neatly.

BUG=webrtc:5805

[ossu, 2016-08-25 10:49:14]

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

[ossu, 2016-08-25 10:49:15]

This is the work I've done so far in moving audio packet splitting from NetEq
internals into AudioDecoder. We can either work on getting this CL to cover FEC
as well, or we could put this in as-is and do FEC in a separate CL, together
with making the packet splitting semantics clearer. Explanation on how and when
to split packets is quite hand-wavy in this CL.

[kwiberg-webrtc, 2016-08-26 12:39:25]

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.cc:107: uint32_t
timestamps_per_ms) {
This is the sort of function that should have a unit test.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.cc:114: size_t min_chunk_size =
bytes_per_ms * 20;
const

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.cc:119: split_size_bytes >>= 1;
Less cleverly written as split_size_bytes /= 2;

I'm sure there's a way to do this without looping, but we can leave that
optimization for another day...

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.cc:121: uint32_t
timestamps_per_chunk = static_cast<uint32_t>(
const

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_decoder.h (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:39: typedef
std::vector<PacketSplit> PacketSplits;
I'm not sure it's worth having a typedef for this. It's not that much shorter,
and it obscures what the type is.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:89: // possible to return an
empty PacketSplits vector.
Should we note that this (and the other static function below) are experimental
and shouldn't be used?

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:91: size_t payload_len)
const;
Consider using a single rtc::ArrayView<const uint8_t> argument instead.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:136: // Utility function for
use by sample-based codecs to implement SplitPacket.
Hmm. If things are expected to change further, maybe this utility should live
somewhere else than in our public API.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:140: uint32_t
timestamps_per_ms);
Consider replacing the first two arguments with an rtc::ArrayView<const
uint8_t>.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/g722/audio_decoder_g722.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/g722/audio_decoder_g722.cc:53: return
SplitBySamples(payload, payload_len, 8, 16);
You could probably make the SplitBySamples calls easier to read by making named
locals for the third and fourth arguments.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/g722/audio_decoder_g722.cc:133: //           
 fine.
I agree. (And I don't think this comment needs to be in the code.)

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:59: if
(payload_len >= 950) {
It's probably best to name the constants:

  constexpr int bytes_in_20ms_frame = 38;
  constexpr int bytes_in_30ms_frame = 50;
  constexpr int frame_bytes_lcm = 950;  // LCM(38, 50) = 950.
  if (payload_len >= frame_bytes_lcm) {

Hmm. Discarding everything that's longer than the lcm is excessive. It should be
enough to discard only those packets that are actually multiples of both 38 and
50.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:79: }
Is this special case really needed? It looks like the loop would handle it just
fine (and if it doesn't, wouldn't it be ending every sequence with a zero-size
PacketSplit?).

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.cc:52: //        
    CheckedDivExact to catch that happening.
Could you handle that by letting SplitBySamples work on 10ms instead of 1ms
chunks?

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/payload_splitter.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.cc:251: // No splitting
necessary; move on to the next packet
Hmm. Wouldn't it be better to test just the first thing, and DCHECK the
remaining three?

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.cc:256: Packet* new_packet =
new Packet;
Ew. Consider using unique_ptr in this block, and using .release() when you put
it in |new_packets| at the end.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc:158:
ASSERT_EQ(payload_value, packet->payload[i]);
The caller of VerifyPacket doesn't call ASSERT/EXPECT_NO_FATAL_FAILURES, so it's
probably better to just EXPECT. See
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid....

[ossu, 2016-08-26 13:05:30]

I'll do another pass over this CL. Specifically, I think SplitBySamples should
be moved out and made a utility. Since the whole SplitAudio API should change
pretty soon in favor of a bigger change to the AudioDecoder interface, there's
really no reason to provide utilities for users since they shouldn't implement
it anyways.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.cc:107: uint32_t
timestamps_per_ms) {
On 2016/08/26 12:39:24, kwiberg-webrtc wrote:
> This is the sort of function that should have a unit test.

It does. Several of them even.
Though, granted, it's not tested by calling SplitBySamples directly, but the
same way it was tested before: by setting up codecs with known splits and
testing the splitting on them.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.cc:114: size_t min_chunk_size =
bytes_per_ms * 20;
On 2016/08/26 12:39:24, kwiberg-webrtc wrote:
> const

Hey, don't blame me, I just clean up around here! :)

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.cc:119: split_size_bytes >>= 1;
On 2016/08/26 12:39:24, kwiberg-webrtc wrote:
> Less cleverly written as split_size_bytes /= 2;
> 
> I'm sure there's a way to do this without looping, but we can leave that
> optimization for another day...

Yeah, I mean the code can be improved but I've aimed to change as little as
possible during the restructuring. I could simplify it in a separate CL and if
something breaks, we know it's not due to moving stuff around, but due to the
cleanup.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_decoder.h (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:91: size_t payload_len)
const;
On 2016/08/26 12:39:24, kwiberg-webrtc wrote:
> Consider using a single rtc::ArrayView<const uint8_t> argument instead.

I've used this since it's how the rest of the API looks. An ArrayView is better,
but not at the cost of conformance, IMO.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:136: // Utility function for
use by sample-based codecs to implement SplitPacket.
On 2016/08/26 12:39:24, kwiberg-webrtc wrote:
> Hmm. If things are expected to change further, maybe this utility should live
> somewhere else than in our public API.

Yeah, I don't love it here... It could be useful to provide this as a utility
for others to use as well, however they could just copy the functionality.
Anyway, it should maybe be in some separate utility header, possibly an internal
one.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/g722/audio_decoder_g722.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/g722/audio_decoder_g722.cc:133: //           
 fine.
On 2016/08/26 12:39:24, kwiberg-webrtc wrote:
> I agree. (And I don't think this comment needs to be in the code.)

Cool. I've put this TODO here so that it doesn't get glossed over in review.
I'll remove it before landing.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:79: }
On 2016/08/26 12:39:25, kwiberg-webrtc wrote:
> Is this special case really needed? It looks like the loop would handle it
just
> fine (and if it doesn't, wouldn't it be ending every sequence with a zero-size
> PacketSplit?).

You're right. This shouldn't be necessary. I'll remove it.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/payload_splitter.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.cc:251: // No splitting
necessary; move on to the next packet
On 2016/08/26 12:39:25, kwiberg-webrtc wrote:
> Hmm. Wouldn't it be better to test just the first thing, and DCHECK the
> remaining three?

Nah, I think it's fine if you can figure out that some data could be discarded
already in SplitPacket. Though the extra copying probably isn't worth it.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.cc:256: Packet* new_packet =
new Packet;
On 2016/08/26 12:39:25, kwiberg-webrtc wrote:
> Ew. Consider using unique_ptr in this block, and using .release() when you put
> it in |new_packets| at the end.

Hello. I'm the messenger. Please don't shoot me! :)

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc:158:
ASSERT_EQ(payload_value, packet->payload[i]);
On 2016/08/26 12:39:25, kwiberg-webrtc wrote:
> The caller of VerifyPacket doesn't call ASSERT/EXPECT_NO_FATAL_FAILURES, so
it's
> probably better to just EXPECT. See
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid....

Well you'll get one roughly one trillion errors during a failing test, rather
than only the first one. The line just before the for-loop does ASSERT_FALSE, so
an assert could already happen.

[kwiberg-webrtc, 2016-08-26 22:05:13]

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.cc:107: uint32_t
timestamps_per_ms) {
On 2016/08/26 13:05:29, ossu wrote:
> On 2016/08/26 12:39:24, kwiberg-webrtc wrote:
> > This is the sort of function that should have a unit test.
> 
> It does. Several of them even.
> Though, granted, it's not tested by calling SplitBySamples directly, but the
> same way it was tested before: by setting up codecs with known splits and
> testing the splitting on them.

It's arguably not a unit test of this function in that case. :-) But that's
certainly much better than nothing.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.cc:114: size_t min_chunk_size =
bytes_per_ms * 20;
On 2016/08/26 13:05:29, ossu wrote:
> On 2016/08/26 12:39:24, kwiberg-webrtc wrote:
> > const
> 
> Hey, don't blame me, I just clean up around here! :)

Yeah, I think I wrote this comment before I realized that you'd just moved this
code from somewhere else.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.cc:119: split_size_bytes >>= 1;
On 2016/08/26 13:05:29, ossu wrote:
> On 2016/08/26 12:39:24, kwiberg-webrtc wrote:
> > Less cleverly written as split_size_bytes /= 2;
> > 
> > I'm sure there's a way to do this without looping, but we can leave that
> > optimization for another day...
> 
> Yeah, I mean the code can be improved but I've aimed to change as little as
> possible during the restructuring. I could simplify it in a separate CL and if
> something breaks, we know it's not due to moving stuff around, but due to the
> cleanup.

sgtm.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_decoder.h (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:91: size_t payload_len)
const;
On 2016/08/26 13:05:29, ossu wrote:
> On 2016/08/26 12:39:24, kwiberg-webrtc wrote:
> > Consider using a single rtc::ArrayView<const uint8_t> argument instead.
> 
> I've used this since it's how the rest of the API looks. An ArrayView is
better,
> but not at the cost of conformance, IMO.

That's unavoidable if you want a gradual shift from old to new style. But
switching everything at once is problematic in other ways, as is not switching.

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc (right):

https://codereview.webrtc.org/2281453002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc:158:
ASSERT_EQ(payload_value, packet->payload[i]);
On 2016/08/26 13:05:30, ossu wrote:
> On 2016/08/26 12:39:25, kwiberg-webrtc wrote:
> > The caller of VerifyPacket doesn't call ASSERT/EXPECT_NO_FATAL_FAILURES, so
> it's
> > probably better to just EXPECT. See
> >
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid....
> 
> Well you'll get one roughly one trillion errors during a failing test, rather
> than only the first one. The line just before the for-loop does ASSERT_FALSE,
so
> an assert could already happen.

OK. But then the callers ought to use ASSERT_NO_FATAL_FAILURES.

[ossu, 2016-09-19 15:29:59]

Description was changed from

==========
Moved codec-specific audio packet splitting into decoders.

There's still some code run specifically for Opus w/ FEC. Will
investigate on how to move that as well.

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

to

==========
Moved codec-specific audio packet splitting into decoders.

There's still some code run specifically for Opus w/ FEC. Will
investigate on how to move that as well.

Closed in favor of https://codereview.webrtc.org/2326003002, which solves the
same problem more neatly.

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