Moved Opus-specific payload splitting into AudioDecoderOpus.

Moved Opus-specific payload splitting into AudioDecoderOpus.

The biggest change to NetEq is the move from a primary flag, to a
Priority with two separate levels: one set by RED splitting and one
set by the codec itself. This allows us to unambigously prioritize
"fallback" packets from these two sources. I've chosen what I believe
is the sensible ordering: packets that the codec prioritizes are
chosen first, regardless of if they are secondary RED packets or
not. So if we were to use Opus w/ FEC in RED, we'd only do Opus FEC
decoding if there was no RED packet that could cover the time slot.

With this change, PayloadSplitter now only deals with RED
packets. Maybe it should be renamed RedPayloadSplitter?

BUG=webrtc:5805
Committed: https://crrev.com/a70695a3e1e7ead375d4bbc7997cd630d1ac1731
Cr-Commit-Position: refs/heads/master@{#14347}

[ossu, 2016-09-14 16:21:07]

ossu@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, kwiberg@webrtc.org

[ossu, 2016-09-14 16:21:08]

This is the last of my three CLs that changes payload splitting in NetEq.
Although it touches quite a few files, most of them are only simple interface
updates.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc:347:
TEST(FecPayloadSplitter, MixedPayload) {
Since splitting is now done on a per-codec basis, I can't really find a place
for this test as it is. There's nothing we can hand both a PCMU payload and an
Opus payload and make sure one of them is split while the other is not. There
should maybe be a test on the Opus codec that FEC payloads are identified and
split properly, though.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc:398:
TEST(FecPayloadSplitter, EmbedFecInRed) {
As RED and FEC is now indicated separately, rather than sharing the primary
flag, there's not much point in testing them together. To retain this test, it
would have to test that NetEq does the right assignments (i.e. the SplitRed
value goes into red_level and the codec's priority goes into codec_level).
That's a lot of testing for two lines of very simple code. Perhaps this test
could be turned into one testing that Packet::Priority orders things properly?

[hlundin-webrtc, 2016-09-15 09:33:30]

Nice changes!

I'm in favor of renaming PayloadSplitter to RedPayloadSplitter. Please, do this
as a last step (to avoid diff-mess between patch sets), and remove the question
from the CL description.

See more comments inline.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:64: uint8_t priority,
"You should not use the unsigned integer types such as uint32_t, unless there is
a valid reason such as representing a bit pattern rather than a number, or you
need defined overflow modulo 2^N. In particular, do not use unsigned types to
say a number will never be negative. Instead, use assertions for this."
https://google.github.io/styleguide/cppguide.html#Integer_Types

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:27:
swap(this->payload_, payload);
std::move?

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:61:
AudioDecoderOpus* decoder_;
const, const, const

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:85: const int
duration =
What if PacketDurationRedundant returns an error (-1)?

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:50:
memset(decoded.data(), 0,
ASSERT_GE(decoded.size(), sizeof(int16_t) * kPacketDuration * num_channels_);

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:57: size_t
num_channels_;
const

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:84:
RTC_DCHECK(false);
FAIL();

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:104:
RTC_DCHECK(false);
FAIL();

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:272: // TODO(ossu): Check
that this is really want we want to filter on.
I would argue that also the old code was wrong. This is a legacy from when
duplicates were purged on extracting packets (i.e., as part of GetAudio). So,
when checking the size of the buffer, we wanted to avoid counting two packets
covering the same samples twice.

Since some time back, we're purging duplicates on InsertPacket. So, the right
thing to do should be to count all packets. You can put a TODO on me for that
here. It's probably going to mess up unit tests. And, keep the new code for now.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:561: a->priority =
{1, 0};
Add tests for the other cases too.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter_unittest.cc:347:
TEST(FecPayloadSplitter, MixedPayload) {
On 2016/09/14 16:21:07, ossu wrote:
> Since splitting is now done on a per-codec basis, I can't really find a place
> for this test as it is. There's nothing we can hand both a PCMU payload and an
> Opus payload and make sure one of them is split while the other is not. There
> should maybe be a test on the Opus codec that FEC payloads are identified and
> split properly, though.

Acknowledged.

[ossu, 2016-09-15 12:22:53]

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:64: uint8_t priority,
On 2016/09/15 09:33:29, hlundin-webrtc wrote:
> "You should not use the unsigned integer types such as uint32_t, unless there
is
> a valid reason such as representing a bit pattern rather than a number, or you
> need defined overflow modulo 2^N. In particular, do not use unsigned types to
> say a number will never be negative. Instead, use assertions for this."
> https://google.github.io/styleguide/cppguide.html#Integer_Types

You are technically correct - the best kind of correct! Perhaps these should
just be ints after all. Seems like a waste of bytes, though. Not that they are
very expensive...

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:27:
swap(this->payload_, payload);
On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> std::move?

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:61:
AudioDecoderOpus* decoder_;
On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> const, const, const

Acknowledged. Acknowledged. Acknowledged.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:85: const int
duration =
On 2016/09/15 09:33:29, hlundin-webrtc wrote:
> What if PacketDurationRedundant returns an error (-1)?

I can't see that it's able to: it will call WebRtcOpus_FecDurationEst which in
turn calls opus_packet_get_samples_per_frame, whose return value is either >=
480 && <= 5760 or it returns 0.
Could to an RTC_DCHECK just to be safe, I guess.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:50:
memset(decoded.data(), 0,
On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> ASSERT_GE(decoded.size(), sizeof(int16_t) * kPacketDuration * num_channels_);

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:57: size_t
num_channels_;
On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> const

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:84:
RTC_DCHECK(false);
On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> FAIL();

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:104:
RTC_DCHECK(false);
On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> FAIL();

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet.h:28: uint8_t codec_level;
I'm not super psyched about the naming here, but it was the best I could come up
with at the time. Any suggestions to change codec_level and red_level to?

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:272: // TODO(ossu): Check
that this is really want we want to filter on.
On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> I would argue that also the old code was wrong. This is a legacy from when
> duplicates were purged on extracting packets (i.e., as part of GetAudio). So,
> when checking the size of the buffer, we wanted to avoid counting two packets
> covering the same samples twice.
> 
> Since some time back, we're purging duplicates on InsertPacket. So, the right
> thing to do should be to count all packets. You can put a TODO on me for that
> here. It's probably going to mess up unit tests. And, keep the new code for
now.

Acknowledged.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:561: a->priority =
{1, 0};
On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> Add tests for the other cases too.

Acknowledged.

[hlundin-webrtc, 2016-09-16 07:51:27]

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:64: uint8_t priority,
On 2016/09/15 12:22:53, ossu wrote:
> On 2016/09/15 09:33:29, hlundin-webrtc wrote:
> > "You should not use the unsigned integer types such as uint32_t, unless
there
> is
> > a valid reason such as representing a bit pattern rather than a number, or
you
> > need defined overflow modulo 2^N. In particular, do not use unsigned types
to
> > say a number will never be negative. Instead, use assertions for this."
> > https://google.github.io/styleguide/cppguide.html#Integer_Types
> 
> You are technically correct - the best kind of correct! Perhaps these should
> just be ints after all. Seems like a waste of bytes, though. Not that they are
> very expensive...

Once upon a time, in an office building (not so) far far away, we used to spend
a lot of time tweaking the size of member variables. This code is more int than
uint8_t these days...

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:85: const int
duration =
On 2016/09/15 12:22:53, ossu wrote:
> On 2016/09/15 09:33:29, hlundin-webrtc wrote:
> > What if PacketDurationRedundant returns an error (-1)?
> 
> I can't see that it's able to: it will call WebRtcOpus_FecDurationEst which in
> turn calls opus_packet_get_samples_per_frame, whose return value is either >=
> 480 && <= 5760 or it returns 0.
> Could to an RTC_DCHECK just to be safe, I guess.

But the method comment for AudioDecoder::PacketDuration(Redundant) says "... -1
in case of an error".

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet.h:28: uint8_t codec_level;
On 2016/09/15 12:22:53, ossu wrote:
> I'm not super psyched about the naming here, but it was the best I could come
up
> with at the time. Any suggestions to change codec_level and red_level to?

Nope.

[kwiberg-webrtc, 2016-09-19 11:07:49]

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:64: uint8_t priority,
On 2016/09/15 12:22:53, ossu wrote:
> On 2016/09/15 09:33:29, hlundin-webrtc wrote:
> > "You should not use the unsigned integer types such as uint32_t, unless
there
> is
> > a valid reason such as representing a bit pattern rather than a number, or
you
> > need defined overflow modulo 2^N. In particular, do not use unsigned types
to
> > say a number will never be negative. Instead, use assertions for this."
> > https://google.github.io/styleguide/cppguide.html#Integer_Types
> 
> You are technically correct - the best kind of correct! Perhaps these should
> just be ints after all. Seems like a waste of bytes, though. Not that they are
> very expensive...

Function arguments and local variables are stored in registers most of the time
unless they're live when you call a subroutine, and then (almost) any sort of
integer will occupy a full register.

+1 for making this an int.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:33: int ret =
decoder_->Decode(
const

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:36: }
const:

const int ret = (is_primary_payload_ ? decoder_->PacketDuration
                                     : decoder_->PacketDurationRedundant)(
    payload_.data(), payload_.size());

or however clang-format wants to lay that out...

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:52: }
The same ?: construction can be used to make ret const here (and there's a much
longer argument list to dedupe).

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:63: bool
is_primary_payload_;
Not sure if this is what Henrik was pointing out, but can these two be const
too?

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:692: int ret =
packet_buffer_->InsertPacketList(
const?

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:46: size_t
Duration() const /* override */ { return kPacketDuration; }
Do you need to comment out the overrides when you're not using MOCK_METHOD in
this class?

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet.h:52: static constexpr Priority
kHighestPriority = {0, 0};
I'm a bit suspicious about this constant. Why is it ever a good idea to compare
an existing priority with the highest priority, as opposed to comparing two
existing priorities?

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.h:25: class PayloadSplitter {
+1 for making this RedPayloadSplitter. Or PayloadSplitterRed, for consistency
with Youtube Red...

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.h:30: };
:-)

[ossu, 2016-09-19 11:41:01]

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:64: uint8_t priority,
On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> On 2016/09/15 12:22:53, ossu wrote:
> > On 2016/09/15 09:33:29, hlundin-webrtc wrote:
> > > "You should not use the unsigned integer types such as uint32_t, unless
> there
> > is
> > > a valid reason such as representing a bit pattern rather than a number, or
> you
> > > need defined overflow modulo 2^N. In particular, do not use unsigned types
> to
> > > say a number will never be negative. Instead, use assertions for this."
> > > https://google.github.io/styleguide/cppguide.html#Integer_Types
> > 
> > You are technically correct - the best kind of correct! Perhaps these should
> > just be ints after all. Seems like a waste of bytes, though. Not that they
are
> > very expensive...
> 
> Function arguments and local variables are stored in registers most of the
time
> unless they're live when you call a subroutine, and then (almost) any sort of
> integer will occupy a full register.
> 
> +1 for making this an int.

I've thought about this change as also applying to Packet::Priority, which is
where the size "actually" matters. There having two ints where none can be < 0
and both will be really small is more of a waste, and a hassle. I can have them
like that, though, with CHECKs for < 0, but I think we're throwing away some
expressiveness in the API.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/legacy_encoded_audio_frame.cc:33: int ret =
decoder_->Decode(
On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> const

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:36: }
On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> const:
> 
> const int ret = (is_primary_payload_ ? decoder_->PacketDuration
>                                      : decoder_->PacketDurationRedundant)(
>     payload_.data(), payload_.size());
> 
> or however clang-format wants to lay that out...

Oh, god, no! I'll be surprised if it works but even if it does, I don't like it.
Trinary isn't always better than if/else. :)
I could _possibly_ imagine splitting it into two, first getting an auto
duration_function, then calling it on the line below, but we'll see...

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:61:
AudioDecoderOpus* decoder_;
On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> const, const, const

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:85: const int
duration =
On 2016/09/16 07:51:27, hlundin-webrtc wrote:
> On 2016/09/15 12:22:53, ossu wrote:
> > On 2016/09/15 09:33:29, hlundin-webrtc wrote:
> > > What if PacketDurationRedundant returns an error (-1)?
> > 
> > I can't see that it's able to: it will call WebRtcOpus_FecDurationEst which
in
> > turn calls opus_packet_get_samples_per_frame, whose return value is either
>=
> > 480 && <= 5760 or it returns 0.
> > Could to an RTC_DCHECK just to be safe, I guess.
> 
> But the method comment for AudioDecoder::PacketDuration(Redundant) says "...
-1
> in case of an error".

But it is only used within this implementation, i.e. there's no chance
PacketDurationRedundant _will_ return -1, even though the contract says it
could. Would you find it acceptable if I were to replace the call to
PacketdurationRedundant with a call to WebRtcOpus_FecDurationEst directly?
Eventually, I think we'll want to get rid of most of the "classic"
Decode*/PacketDuration* calls, if possible.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:692: int ret =
packet_buffer_->InsertPacketList(
On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> const?

Acknowledged.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:46: size_t
Duration() const /* override */ { return kPacketDuration; }
On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> Do you need to comment out the overrides when you're not using MOCK_METHOD in
> this class?

No, I don't think I should have to. Wonder why they're like that.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet.h:52: static constexpr Priority
kHighestPriority = {0, 0};
On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> I'm a bit suspicious about this constant. Why is it ever a good idea to
compare
> an existing priority with the highest priority, as opposed to comparing two
> existing priorities?

I was thinking it would be clearer to set something to "the highest priority"
rather than to {0, 0} or a default-instantiated Priority. At a couple of places,
we're also comparing against what was previously "primary flag set", which is
more-or-less the same as "the highest priority".

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.h:25: class PayloadSplitter {
On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> +1 for making this RedPayloadSplitter. Or PayloadSplitterRed, for consistency
> with Youtube Red...

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.h:30: };
On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> :-)

Yeah, maybe just have RedPayloadSplitter return a bool at this point.

[kwiberg-webrtc, 2016-09-19 11:55:17]

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:64: uint8_t priority,
On 2016/09/19 11:41:01, ossu wrote:
> On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> > On 2016/09/15 12:22:53, ossu wrote:
> > > On 2016/09/15 09:33:29, hlundin-webrtc wrote:
> > > > "You should not use the unsigned integer types such as uint32_t, unless
> > there
> > > is
> > > > a valid reason such as representing a bit pattern rather than a number,
or
> > you
> > > > need defined overflow modulo 2^N. In particular, do not use unsigned
types
> > to
> > > > say a number will never be negative. Instead, use assertions for this."
> > > > https://google.github.io/styleguide/cppguide.html#Integer_Types
> > > 
> > > You are technically correct - the best kind of correct! Perhaps these
should
> > > just be ints after all. Seems like a waste of bytes, though. Not that they
> are
> > > very expensive...
> > 
> > Function arguments and local variables are stored in registers most of the
> time
> > unless they're live when you call a subroutine, and then (almost) any sort
of
> > integer will occupy a full register.
> > 
> > +1 for making this an int.
> 
> I've thought about this change as also applying to Packet::Priority, which is
> where the size "actually" matters. There having two ints where none can be < 0
> and both will be really small is more of a waste, and a hassle. I can have
them
> like that, though, with CHECKs for < 0, but I think we're throwing away some
> expressiveness in the API.

I also thought about this change as applying to Packet::Priority. Why would you
need to CHECK that the values are nonnegative?

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:36: }
On 2016/09/19 11:41:01, ossu wrote:
> On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> > const:
> > 
> > const int ret = (is_primary_payload_ ? decoder_->PacketDuration
> >                                      : decoder_->PacketDurationRedundant)(
> >     payload_.data(), payload_.size());
> > 
> > or however clang-format wants to lay that out...
> 
> Oh, god, no! I'll be surprised if it works but even if it does, I don't like
it.
> Trinary isn't always better than if/else. :)
> I could _possibly_ imagine splitting it into two, first getting an auto
> duration_function, then calling it on the line below, but we'll see...
> 

Hmm, you're right, I don't think it will work. And if you're not interested in
?:, rewriting it with lambdas won't be an improvement.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:52: }
On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> The same ?: construction can be used to make ret const here (and there's a
much
> longer argument list to dedupe).

No, it won't work for the same reason as above.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:61:
AudioDecoderOpus* decoder_;
On 2016/09/19 11:41:01, ossu wrote:
> On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> > const, const, const
> 
> Acknowledged.

Acknowledged.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:46: size_t
Duration() const /* override */ { return kPacketDuration; }
On 2016/09/19 11:41:01, ossu wrote:
> On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> > Do you need to comment out the overrides when you're not using MOCK_METHOD
in
> > this class?
> 
> No, I don't think I should have to. Wonder why they're like that.

Because MOCK_METHOD0 et al. expand to method declarations without the "override"
keyword, and we compile with -Werror and -Winconsistent-missing-override.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet.h:52: static constexpr Priority
kHighestPriority = {0, 0};
On 2016/09/19 11:41:01, ossu wrote:
> On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> > I'm a bit suspicious about this constant. Why is it ever a good idea to
> compare
> > an existing priority with the highest priority, as opposed to comparing two
> > existing priorities?
> 
> I was thinking it would be clearer to set something to "the highest priority"
> rather than to {0, 0} or a default-instantiated Priority.

Makes sense as a short-hand---*if* priority values are restricted to be
nonnegative for some other reason.

> At a couple of places,
> we're also comparing against what was previously "primary flag set", which is
> more-or-less the same as "the highest priority".

Yes, this is what I was suspicious about. Why are we doing that?

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/payload_splitter.h:30: };
On 2016/09/19 11:41:01, ossu wrote:
> On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> > :-)
> 
> Yeah, maybe just have RedPayloadSplitter return a bool at this point.

Yes. Or an enum class, but a bool should be enough.

[hlundin-webrtc, 2016-09-19 12:19:13]

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:36: }
On 2016/09/19 11:55:16, kwiberg-webrtc wrote:
> On 2016/09/19 11:41:01, ossu wrote:
> > On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> > > const:
> > > 
> > > const int ret = (is_primary_payload_ ? decoder_->PacketDuration
> > >                                      : decoder_->PacketDurationRedundant)(
> > >     payload_.data(), payload_.size());
> > > 
> > > or however clang-format wants to lay that out...
> > 
> > Oh, god, no! I'll be surprised if it works but even if it does, I don't like
> it.
> > Trinary isn't always better than if/else. :)
> > I could _possibly_ imagine splitting it into two, first getting an auto
> > duration_function, then calling it on the line below, but we'll see...
> > 
> 
> Hmm, you're right, I don't think it will work. And if you're not interested in
> ?:, rewriting it with lambdas won't be an improvement.

I'm with ossu: "Oh, God, no!"

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:61:
AudioDecoderOpus* decoder_;
On 2016/09/19 11:55:16, kwiberg-webrtc wrote:
> On 2016/09/19 11:41:01, ossu wrote:
> > On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> > > const, const, const
> > 
> > Acknowledged.
> 
> Acknowledged.

Roger that.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:63: bool
is_primary_payload_;
On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> Not sure if this is what Henrik was pointing out, but can these two be const
> too?

That is why I put three "const" in one comment. I was hoping to have them spread
out over the three adjacent lines, not all being applied to decoder_ (although I
guess that is possible).

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:85: const int
duration =
On 2016/09/19 11:41:01, ossu wrote:
> On 2016/09/16 07:51:27, hlundin-webrtc wrote:
> > On 2016/09/15 12:22:53, ossu wrote:
> > > On 2016/09/15 09:33:29, hlundin-webrtc wrote:
> > > > What if PacketDurationRedundant returns an error (-1)?
> > > 
> > > I can't see that it's able to: it will call WebRtcOpus_FecDurationEst
which
> in
> > > turn calls opus_packet_get_samples_per_frame, whose return value is either
> >=
> > > 480 && <= 5760 or it returns 0.
> > > Could to an RTC_DCHECK just to be safe, I guess.
> > 
> > But the method comment for AudioDecoder::PacketDuration(Redundant) says "...
> -1
> > in case of an error".
> 
> But it is only used within this implementation, i.e. there's no chance
> PacketDurationRedundant _will_ return -1, even though the contract says it
> could. Would you find it acceptable if I were to replace the call to
> PacketdurationRedundant with a call to WebRtcOpus_FecDurationEst directly?
> Eventually, I think we'll want to get rid of most of the "classic"
> Decode*/PacketDuration* calls, if possible.

You are right. Since this is an internal affair of this implementation, I'd
settle for a DCHECK here.

[ossu, 2016-09-19 14:07:34]

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:64: uint8_t priority,
On 2016/09/19 11:55:16, kwiberg-webrtc wrote:
> On 2016/09/19 11:41:01, ossu wrote:
> > On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> > > On 2016/09/15 12:22:53, ossu wrote:
> > > > On 2016/09/15 09:33:29, hlundin-webrtc wrote:
> > > > > "You should not use the unsigned integer types such as uint32_t,
unless
> > > there
> > > > is
> > > > > a valid reason such as representing a bit pattern rather than a
number,
> or
> > > you
> > > > > need defined overflow modulo 2^N. In particular, do not use unsigned
> types
> > > to
> > > > > say a number will never be negative. Instead, use assertions for
this."
> > > > > https://google.github.io/styleguide/cppguide.html#Integer_Types
> > > > 
> > > > You are technically correct - the best kind of correct! Perhaps these
> should
> > > > just be ints after all. Seems like a waste of bytes, though. Not that
they
> > are
> > > > very expensive...
> > > 
> > > Function arguments and local variables are stored in registers most of the
> > time
> > > unless they're live when you call a subroutine, and then (almost) any sort
> of
> > > integer will occupy a full register.
> > > 
> > > +1 for making this an int.
> > 
> > I've thought about this change as also applying to Packet::Priority, which
is
> > where the size "actually" matters. There having two ints where none can be <
0
> > and both will be really small is more of a waste, and a hassle. I can have
> them
> > like that, though, with CHECKs for < 0, but I think we're throwing away some
> > expressiveness in the API.
> 
> I also thought about this change as applying to Packet::Priority. Why would
you
> need to CHECK that the values are nonnegative?

If we want some priority level to correspond to "normal audio packet from a
non-RED payload or primary RED-sub-block", e.g. for stats.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:36: }
On 2016/09/19 11:55:16, kwiberg-webrtc wrote:
> On 2016/09/19 11:41:01, ossu wrote:
> > On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> > > const:
> > > 
> > > const int ret = (is_primary_payload_ ? decoder_->PacketDuration
> > >                                      : decoder_->PacketDurationRedundant)(
> > >     payload_.data(), payload_.size());
> > > 
> > > or however clang-format wants to lay that out...
> > 
> > Oh, god, no! I'll be surprised if it works but even if it does, I don't like
> it.
> > Trinary isn't always better than if/else. :)
> > I could _possibly_ imagine splitting it into two, first getting an auto
> > duration_function, then calling it on the line below, but we'll see...
> > 
> 
> Hmm, you're right, I don't think it will work. And if you're not interested in
> ?:, rewriting it with lambdas won't be an improvement.

?: is fine but it's easy to end up with a big, confusing expression. I opt for
leaving this as-is for now, with a look towards more restructuring when we
simplify the set of methods on AudioDecoder.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:63: bool
is_primary_payload_;
On 2016/09/19 12:19:13, hlundin-webrtc wrote:
> On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> > Not sure if this is what Henrik was pointing out, but can these two be const
> > too?
> 
> That is why I put three "const" in one comment. I was hoping to have them
spread
> out over the three adjacent lines, not all being applied to decoder_ (although
I
> guess that is possible).

I was only able to get two consts into the decoder_ line, and then it didn't
compile. :(

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:85: const int
duration =
On 2016/09/19 12:19:13, hlundin-webrtc wrote:
> On 2016/09/19 11:41:01, ossu wrote:
> > On 2016/09/16 07:51:27, hlundin-webrtc wrote:
> > > On 2016/09/15 12:22:53, ossu wrote:
> > > > On 2016/09/15 09:33:29, hlundin-webrtc wrote:
> > > > > What if PacketDurationRedundant returns an error (-1)?
> > > > 
> > > > I can't see that it's able to: it will call WebRtcOpus_FecDurationEst
> which
> > in
> > > > turn calls opus_packet_get_samples_per_frame, whose return value is
either
> > >=
> > > > 480 && <= 5760 or it returns 0.
> > > > Could to an RTC_DCHECK just to be safe, I guess.
> > > 
> > > But the method comment for AudioDecoder::PacketDuration(Redundant) says
"...
> > -1
> > > in case of an error".
> > 
> > But it is only used within this implementation, i.e. there's no chance
> > PacketDurationRedundant _will_ return -1, even though the contract says it
> > could. Would you find it acceptable if I were to replace the call to
> > PacketdurationRedundant with a call to WebRtcOpus_FecDurationEst directly?
> > Eventually, I think we'll want to get rid of most of the "classic"
> > Decode*/PacketDuration* calls, if possible.
> 
> You are right. Since this is an internal affair of this implementation, I'd
> settle for a DCHECK here.

Acknowledged.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:46: size_t
Duration() const /* override */ { return kPacketDuration; }
On 2016/09/19 11:55:16, kwiberg-webrtc wrote:
> On 2016/09/19 11:41:01, ossu wrote:
> > On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> > > Do you need to comment out the overrides when you're not using MOCK_METHOD
> in
> > > this class?
> > 
> > No, I don't think I should have to. Wonder why they're like that.
> 
> Because MOCK_METHOD0 et al. expand to method declarations without the
"override"
> keyword, and we compile with -Werror and -Winconsistent-missing-override.

Oh, right, now I see. I forgot why I left them in. The answer is: I don't know
but let's find out!. I'll put the overrides in and we'll see if any bots explode
on it.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet.h:52: static constexpr Priority
kHighestPriority = {0, 0};
On 2016/09/19 11:55:16, kwiberg-webrtc wrote:
> On 2016/09/19 11:41:01, ossu wrote:
> > On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> > > I'm a bit suspicious about this constant. Why is it ever a good idea to
> > compare
> > > an existing priority with the highest priority, as opposed to comparing
two
> > > existing priorities?
> > 
> > I was thinking it would be clearer to set something to "the highest
priority"
> > rather than to {0, 0} or a default-instantiated Priority.
> 
> Makes sense as a short-hand---*if* priority values are restricted to be
> nonnegative for some other reason.

I think stats is one reason. To me it makes sense to have a level for
"this-is-as-good-as-it-gets" in NetEq and std::numeric_limits<int>::min() is too
annoying to write..

> > At a couple of places,
> > we're also comparing against what was previously "primary flag set", which
is
> > more-or-less the same as "the highest priority".
> 
> Yes, this is what I was suspicious about. Why are we doing that?

Bad judgment? Poor education? A rough upbringing? I think the answer is, again,
stats. Maybe that should change, though. kHighestPriority is only compared
against in two places, and both seem slightly wrong.

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:272: // TODO(ossu): Check
that this is really want we want to filter on.
On 2016/09/15 12:22:53, ossu wrote:
> On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> > I would argue that also the old code was wrong. This is a legacy from when
> > duplicates were purged on extracting packets (i.e., as part of GetAudio).
So,
> > when checking the size of the buffer, we wanted to avoid counting two
packets
> > covering the same samples twice.
> > 
> > Since some time back, we're purging duplicates on InsertPacket. So, the
right
> > thing to do should be to count all packets. You can put a TODO on me for
that
> > here. It's probably going to mess up unit tests. And, keep the new code for
> now.
> 
> Acknowledged.

I tried putting something else here: checking only the red_level or removing the
check altogether, but the Opus bitexactness test failed. Everything else seemed
fine, though. I've kept the behavior, but gotten rid of kHighestPriority while
we figure out what to do with it.

[hlundin-webrtc, 2016-09-20 07:02:24]

Three nits, then LGTM.

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_decoder.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_decoder.cc:17: #include <utility>
Duplicate.

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_decoder.h (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_decoder.h:14: #include <memory>
Duplicates.

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:286:
packet_buffer_->DiscardPacketsWithPayloadType(rtp_payload_type);
This is very courteous...

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:265: Packet *packet = *it;
Packet* packet

[kwiberg-webrtc, 2016-09-20 09:14:45]

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_decoder.h:64: uint8_t priority,
On 2016/09/19 14:07:33, ossu wrote:
> On 2016/09/19 11:55:16, kwiberg-webrtc wrote:
> > On 2016/09/19 11:41:01, ossu wrote:
> > > On 2016/09/19 11:07:49, kwiberg-webrtc wrote:
> > > > On 2016/09/15 12:22:53, ossu wrote:
> > > > > On 2016/09/15 09:33:29, hlundin-webrtc wrote:
> > > > > > "You should not use the unsigned integer types such as uint32_t,
> unless
> > > > there
> > > > > is
> > > > > > a valid reason such as representing a bit pattern rather than a
> number,
> > or
> > > > you
> > > > > > need defined overflow modulo 2^N. In particular, do not use unsigned
> > types
> > > > to
> > > > > > say a number will never be negative. Instead, use assertions for
> this."
> > > > > > https://google.github.io/styleguide/cppguide.html#Integer_Types
> > > > > 
> > > > > You are technically correct - the best kind of correct! Perhaps these
> > should
> > > > > just be ints after all. Seems like a waste of bytes, though. Not that
> they
> > > are
> > > > > very expensive...
> > > > 
> > > > Function arguments and local variables are stored in registers most of
the
> > > time
> > > > unless they're live when you call a subroutine, and then (almost) any
sort
> > of
> > > > integer will occupy a full register.
> > > > 
> > > > +1 for making this an int.
> > > 
> > > I've thought about this change as also applying to Packet::Priority, which
> is
> > > where the size "actually" matters. There having two ints where none can be
<
> 0
> > > and both will be really small is more of a waste, and a hassle. I can have
> > them
> > > like that, though, with CHECKs for < 0, but I think we're throwing away
some
> > > expressiveness in the API.
> > 
> > I also thought about this change as applying to Packet::Priority. Why would
> you
> > need to CHECK that the values are nonnegative?
> 
> If we want some priority level to correspond to "normal audio packet from a
> non-RED payload or primary RED-sub-block", e.g. for stats.

Ah, yes, for stats I can see why we'd care about the "absolute" priority and not
just the relative priority. Thanks.

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:61:
AudioDecoderOpus* decoder_;
On 2016/09/19 12:19:13, hlundin-webrtc wrote:
> On 2016/09/19 11:55:16, kwiberg-webrtc wrote:
> > On 2016/09/19 11:41:01, ossu wrote:
> > > On 2016/09/15 09:33:30, hlundin-webrtc wrote:
> > > > const, const, const
> > > 
> > > Acknowledged.
> > 
> > Acknowledged.
> 
> Roger that.

Roger, Roger. What's our vector, Victor?

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:63: packet->priority
= Packet::kHighestPriority;
Why can you skip this?

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:91: new
LegacyEncodedAudioFrame(this, std::move(new_payload)));
Arguably, new_payload doesn't have to be a named variable anymore. Eliminate it
if you feel like it.

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:51: }
That's a lot of duplicated function arguments. Consider the simplification you
suggested to me earlier:

using DT = decltype(decoder_);
const auto dec_fun = is_primary_payload_ ? &DT::PacketDuration
                                         : &DT::PacketDurationRedundant;
const int ret = (decoder_->*dec_fun)(
    payload_.data(), payload_.size(), decoder_->SampleRateHz(),
    decoded.size() * sizeof(int16_t), decoded.data(), &speech_type);

Not having to repeat all those arguments is worth a lot.

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:58:
EXPECT_GE(decoded.size(), output_size);
Isn't it always the case that !(decoded.size() >= output_size) in this else
clause, so that this EXPECT always fails?

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet.h:40: // timeframe. The highest
priority packet will have levels {0, 0}.
It's probably a good idea to DCHECK that the levels are nonnegative in the
constructor and in all the other functions.

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:272: }
Can you use std::remove_if instead? It should be shorter and simpler, and run in
time O(n) instead of O(n^2).

[ossu, 2016-09-20 13:51:56]

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:63: packet->priority
= Packet::kHighestPriority;
On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> Why can you skip this?

Because Priority is now default constructed at the highest level
({0, 0}).

---^^ Look, I made an owl!

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.cc:91: new
LegacyEncodedAudioFrame(this, std::move(new_payload)));
On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> Arguably, new_payload doesn't have to be a named variable anymore. Eliminate
it
> if you feel like it.

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:51: }
On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> That's a lot of duplicated function arguments. Consider the simplification you
> suggested to me earlier:
> 
> using DT = decltype(decoder_);
> const auto dec_fun = is_primary_payload_ ? &DT::PacketDuration
>                                          : &DT::PacketDurationRedundant;
> const int ret = (decoder_->*dec_fun)(
>     payload_.data(), payload_.size(), decoder_->SampleRateHz(),
>     decoded.size() * sizeof(int16_t), decoded.data(), &speech_type);
> 
> Not having to repeat all those arguments is worth a lot.

I'll be leaving this as-is for now, since I expect to be reworking AudioDecoder
some more in the near future.

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:286:
packet_buffer_->DiscardPacketsWithPayloadType(rtp_payload_type);
On 2016/09/20 07:02:24, hlundin-webrtc wrote:
> This is very courteous...

Thanks?

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:58:
EXPECT_GE(decoded.size(), output_size);
On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> Isn't it always the case that !(decoded.size() >= output_size) in this else
> clause, so that this EXPECT always fails?

Yup! Putting an ASSERT in here won't work, since we don't return void.
I'll clarify that I'm using EXPECT_GE just to get the proper printouts (as
opposed to using ADD_FAILURE() like I did below).

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet.h:40: // timeframe. The highest
priority packet will have levels {0, 0}.
On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> It's probably a good idea to DCHECK that the levels are nonnegative in the
> constructor and in all the other functions.

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:265: Packet *packet = *it;
On 2016/09/20 07:02:24, hlundin-webrtc wrote:
> Packet* packet

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:272: }
On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> Can you use std::remove_if instead? It should be shorter and simpler, and run
in
> time O(n) instead of O(n^2).

Since buffer_ is a std::list, this runs in O(n) and having to do delete manually
precludes using remove_if. I might revisit this if/when I take a look at Packet
handling in NetEq again. It'd be nice if we didn't have to manually
memory-manage them.

[kwiberg-webrtc, 2016-09-20 14:56:23]

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

https://codereview.webrtc.org/2342443005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc:63: packet->priority
= Packet::kHighestPriority;
On 2016/09/20 13:51:56, ossu wrote:
> On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> > Why can you skip this?
> 
> Because Priority is now default constructed at the highest level
> ({0, 0}).
> 
> ---^^ Look, I made an owl!

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:51: }
On 2016/09/20 13:51:56, ossu wrote:
> On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> > That's a lot of duplicated function arguments. Consider the simplification
you
> > suggested to me earlier:
> > 
> > using DT = decltype(decoder_);
> > const auto dec_fun = is_primary_payload_ ? &DT::PacketDuration
> >                                          : &DT::PacketDurationRedundant;
> > const int ret = (decoder_->*dec_fun)(
> >     payload_.data(), payload_.size(), decoder_->SampleRateHz(),
> >     decoded.size() * sizeof(int16_t), decoded.data(), &speech_type);
> > 
> > Not having to repeat all those arguments is worth a lot.
> 
> I'll be leaving this as-is for now, since I expect to be reworking
AudioDecoder
> some more in the near future.

Acknowledged.

But expect me to keep badgering you about constructs like these until you've
seen the light.

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:58:
EXPECT_GE(decoded.size(), output_size);
On 2016/09/20 13:51:56, ossu wrote:
> On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> > Isn't it always the case that !(decoded.size() >= output_size) in this else
> > clause, so that this EXPECT always fails?
> 
> Yup! Putting an ASSERT in here won't work, since we don't return void.
> I'll clarify that I'm using EXPECT_GE just to get the proper printouts (as
> opposed to using ADD_FAILURE() like I did below).

Does something like this work?

  ADD_FAILURE() << "Expected " << decoded.size() << " to be >= "
      << output_size << ", but the world is a cruel place.";

Alternatively, something like this?

  EXPECT_GE(a, b);
  if (a < b) {
    // The expectation failed.
    return empty optional;
  }
  do_stuff_that_requires_that(a >= b);

Just trying my best to turn that comment into something the compiler can read.
:-)

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:272: }
On 2016/09/20 13:51:56, ossu wrote:
> On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> > Can you use std::remove_if instead? It should be shorter and simpler, and
run
> in
> > time O(n) instead of O(n^2).
> 
> Since buffer_ is a std::list, this runs in O(n) and having to do delete
manually
> precludes using remove_if. I might revisit this if/when I take a look at
Packet
> handling in NetEq again. It'd be nice if we didn't have to manually
> memory-manage them.

Aha, an std::list. And yes, the manual memory management. OK.

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet.h:47: RTC_DCHECK_GE(red_level, 0);
It's probably just as important to check the levels of the right-hand side. At
which point you'll probably save a few lines by defining a private
CheckInvariants() function.

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:80:
RTC_DCHECK_GE(packet->priority.red_level, 0);
...or maybe make the invariant-checking function public...

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/payload_splitter.h (left):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/payload_splitter.h:1: /*
It looks like you've emptied the three payload_splitter files without deleting
them. Is that the case, or is it just me who can't read the diff properly?

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/red_payload_splitter.cc (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/red_payload_splitter.cc:30: // This could be
as high as 256 (to match the number of priority levels) but
You allow more than 256 priority levels now, right?

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/red_payload_splitter.cc:115:
static_cast<uint8_t>((new_headers.size() - 1) - i);
Cast to int? And use checked_cast?

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/red_payload_splitter.h (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/red_payload_splitter.h:25: class
RedPayloadSplitter {
Could you split this class into an interface and a private implementation? That
hides the implementation better and lets the compiler catch more mistakes for
us, for no runtime cost.

[ossu, 2016-09-20 15:45:22]

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.cc:51: }
On 2016/09/20 14:56:22, kwiberg-webrtc wrote:
> On 2016/09/20 13:51:56, ossu wrote:
> > On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> > > That's a lot of duplicated function arguments. Consider the simplification
> you
> > > suggested to me earlier:
> > > 
> > > using DT = decltype(decoder_);
> > > const auto dec_fun = is_primary_payload_ ? &DT::PacketDuration
> > >                                          : &DT::PacketDurationRedundant;
> > > const int ret = (decoder_->*dec_fun)(
> > >     payload_.data(), payload_.size(), decoder_->SampleRateHz(),
> > >     decoded.size() * sizeof(int16_t), decoded.data(), &speech_type);
> > > 
> > > Not having to repeat all those arguments is worth a lot.
> > 
> > I'll be leaving this as-is for now, since I expect to be reworking
> AudioDecoder
> > some more in the near future.
> 
> Acknowledged.
> 
> But expect me to keep badgering you about constructs like these until you've
> seen the light.

Badger? Badger, badger, badger!

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:58:
EXPECT_GE(decoded.size(), output_size);
On 2016/09/20 14:56:22, kwiberg-webrtc wrote:
> On 2016/09/20 13:51:56, ossu wrote:
> > On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> > > Isn't it always the case that !(decoded.size() >= output_size) in this
else
> > > clause, so that this EXPECT always fails?
> > 
> > Yup! Putting an ASSERT in here won't work, since we don't return void.
> > I'll clarify that I'm using EXPECT_GE just to get the proper printouts (as
> > opposed to using ADD_FAILURE() like I did below).
> 
> Does something like this work?
> 
>   ADD_FAILURE() << "Expected " << decoded.size() << " to be >= "
>       << output_size << ", but the world is a cruel place.";
> 
> Alternatively, something like this?
> 
>   EXPECT_GE(a, b);
>   if (a < b) {
>     // The expectation failed.
>     return empty optional;
>   }
>   do_stuff_that_requires_that(a >= b);
> 
> Just trying my best to turn that comment into something the compiler can read.
> :-)

1) Yes, but it's more to write.
2) Isn't that more-or-less what's already being done, except with the check
being >= and the executing code is in the if-clause. Is there a benefit to
having the "correct" code running outside of the if-statement?

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet.h:47: RTC_DCHECK_GE(red_level, 0);
On 2016/09/20 14:56:23, kwiberg-webrtc wrote:
> It's probably just as important to check the levels of the right-hand side. At
> which point you'll probably save a few lines by defining a private
> CheckInvariants() function.

Alright!

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:80:
RTC_DCHECK_GE(packet->priority.red_level, 0);
On 2016/09/20 14:56:23, kwiberg-webrtc wrote:
> ...or maybe make the invariant-checking function public...

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/payload_splitter.h (left):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/payload_splitter.h:1: /*
On 2016/09/20 14:56:23, kwiberg-webrtc wrote:
> It looks like you've emptied the three payload_splitter files without deleting
> them. Is that the case, or is it just me who can't read the diff properly?

It's just the codereview tool going bananas. Git properly identified them all as
moves.

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/red_payload_splitter.cc (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/red_payload_splitter.cc:30: // This could be
as high as 256 (to match the number of priority levels) but
On 2016/09/20 14:56:23, kwiberg-webrtc wrote:
> You allow more than 256 priority levels now, right?

I do. I'll change this to 2147483648. :)
I'll change the wording.

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/red_payload_splitter.cc:115:
static_cast<uint8_t>((new_headers.size() - 1) - i);
On 2016/09/20 14:56:23, kwiberg-webrtc wrote:
> Cast to int? And use checked_cast?

D'oh!

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/red_payload_splitter.h (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/red_payload_splitter.h:25: class
RedPayloadSplitter {
On 2016/09/20 14:56:23, kwiberg-webrtc wrote:
> Could you split this class into an interface and a private implementation?
That
> hides the implementation better and lets the compiler catch more mistakes for
> us, for no runtime cost.

I think this goes outside the remit of this CL, IMHO. Also not sure why we'd
want to hide (or for that matter mock, as we do) this functionality. It's got no
state and should probably be moved into free functions somewhere.

[kwiberg-webrtc, 2016-09-20 16:19:15]

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc (right):

https://codereview.webrtc.org/2342443005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_network_stats_unittest.cc:58:
EXPECT_GE(decoded.size(), output_size);
On 2016/09/20 15:45:21, ossu wrote:
> On 2016/09/20 14:56:22, kwiberg-webrtc wrote:
> > On 2016/09/20 13:51:56, ossu wrote:
> > > On 2016/09/20 09:14:45, kwiberg-webrtc wrote:
> > > > Isn't it always the case that !(decoded.size() >= output_size) in this
> else
> > > > clause, so that this EXPECT always fails?
> > > 
> > > Yup! Putting an ASSERT in here won't work, since we don't return void.
> > > I'll clarify that I'm using EXPECT_GE just to get the proper printouts (as
> > > opposed to using ADD_FAILURE() like I did below).
> > 
> > Does something like this work?
> > 
> >   ADD_FAILURE() << "Expected " << decoded.size() << " to be >= "
> >       << output_size << ", but the world is a cruel place.";
> > 
> > Alternatively, something like this?
> > 
> >   EXPECT_GE(a, b);
> >   if (a < b) {
> >     // The expectation failed.
> >     return empty optional;
> >   }
> >   do_stuff_that_requires_that(a >= b);
> > 
> > Just trying my best to turn that comment into something the compiler can
read.
> > :-)
> 
> 1) Yes, but it's more to write.
> 2) Isn't that more-or-less what's already being done, except with the check
> being >= and the executing code is in the if-clause. Is there a benefit to
> having the "correct" code running outside of the if-statement?

1) It's more code, but you don't need that multiline comment that explains what
the code is doing. You'll be replacing 1 line of code + 3 lines of comment with
2 lines of code.

2) Yes, it's doing the same thing, but again structured in a way that should let
you eliminate the comment that explains what's going on. (I think I like the
first suggestion better, though.)

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/red_payload_splitter.h (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/red_payload_splitter.h:25: class
RedPayloadSplitter {
On 2016/09/20 15:45:21, ossu wrote:
> On 2016/09/20 14:56:23, kwiberg-webrtc wrote:
> > Could you split this class into an interface and a private implementation?
> That
> > hides the implementation better and lets the compiler catch more mistakes
for
> > us, for no runtime cost.
> 
> I think this goes outside the remit of this CL, IMHO.

Fair enough. (This is a pervasive problem in NetEq. I thought we could chip away
at it one class at a time.)

> Also not sure why we'd
> want to hide (or for that matter mock, as we do) this functionality. It's got
no
> state and should probably be moved into free functions somewhere.

There are unit tests that test the main NetEq object with its innards replaced
with mocks. Not sure if that's a good enough reason in this particular case,
though---I haven't checked if the tests in question are useful.

[ossu, 2016-09-21 10:23:13]

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet.h:47: RTC_DCHECK_GE(red_level, 0);
On 2016/09/20 15:45:21, ossu wrote:
> On 2016/09/20 14:56:23, kwiberg-webrtc wrote:
> > It's probably just as important to check the levels of the right-hand side.
At
> > which point you'll probably save a few lines by defining a private
> > CheckInvariants() function.
> 
> Alright!

Called it CheckInvariant to mimic ArrayView. Maybe it can become a standard name
for that sort of thing in WebRTC?

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:80:
RTC_DCHECK_GE(packet->priority.red_level, 0);
On 2016/09/20 15:45:21, ossu wrote:
> On 2016/09/20 14:56:23, kwiberg-webrtc wrote:
> > ...or maybe make the invariant-checking function public...
> 
> Acknowledged.

Considered it but decided to keep the check like this here, so that we get a
better error message when this happens, i.e. it'll point to one of these lines,
rather than inside Priority::CheckInvariant(). I bet that if priority levels are
set wrong, that will be caught upon insertion into the PacketBuffer (since
they're ferried more-or-less directly from ParsePayload to the PacketBuffer).

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/red_payload_splitter.h (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/red_payload_splitter.h:25: class
RedPayloadSplitter {
On 2016/09/20 16:19:15, kwiberg-webrtc wrote:
> On 2016/09/20 15:45:21, ossu wrote:
> > On 2016/09/20 14:56:23, kwiberg-webrtc wrote:
> > > Could you split this class into an interface and a private implementation?
> > That
> > > hides the implementation better and lets the compiler catch more mistakes
> for
> > > us, for no runtime cost.
> > 
> > I think this goes outside the remit of this CL, IMHO.
> 
> Fair enough. (This is a pervasive problem in NetEq. I thought we could chip
away
> at it one class at a time.)
> 
> > Also not sure why we'd
> > want to hide (or for that matter mock, as we do) this functionality. It's
got
> no
> > state and should probably be moved into free functions somewhere.
> 
> There are unit tests that test the main NetEq object with its innards replaced
> with mocks. Not sure if that's a good enough reason in this particular case,
> though---I haven't checked if the tests in question are useful.

After a quick check, I'm not convinced that Mock(Red)PayloadSplitter is ever
used in the neteq_impl_unittests.c. Nothing seems to be setting up the mock
through EXPECT_CALL, so if it were used (and called by NetEqImpl), we should be
getting warnings from gmock about calling uninteresting mocks.

[kwiberg-webrtc, 2016-09-21 10:59:14]

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet.h:47: RTC_DCHECK_GE(red_level, 0);
On 2016/09/21 10:23:12, ossu wrote:
> On 2016/09/20 15:45:21, ossu wrote:
> > On 2016/09/20 14:56:23, kwiberg-webrtc wrote:
> > > It's probably just as important to check the levels of the right-hand
side.
> At
> > > which point you'll probably save a few lines by defining a private
> > > CheckInvariants() function.
> > 
> > Alright!
> 
> Called it CheckInvariant to mimic ArrayView. Maybe it can become a standard
name
> for that sort of thing in WebRTC?

Yes. Although if intended to be generic, it should probably be plural.

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet_buffer.cc (right):

https://codereview.webrtc.org/2342443005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet_buffer.cc:80:
RTC_DCHECK_GE(packet->priority.red_level, 0);
On 2016/09/21 10:23:13, ossu wrote:
> On 2016/09/20 15:45:21, ossu wrote:
> > On 2016/09/20 14:56:23, kwiberg-webrtc wrote:
> > > ...or maybe make the invariant-checking function public...
> > 
> > Acknowledged.
> 
> Considered it but decided to keep the check like this here, so that we get a
> better error message when this happens, i.e. it'll point to one of these
lines,
> rather than inside Priority::CheckInvariant(). I bet that if priority levels
are
> set wrong, that will be caught upon insertion into the PacketBuffer (since
> they're ferried more-or-less directly from ParsePayload to the PacketBuffer).

Acknowledged.

https://codereview.webrtc.org/2342443005/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet.h:66: }
This works, but you get better error messages if you have this function return a
bool (in which case it probably deserves a better name...) and then [D]CHECK
that at each call site.

[ossu, 2016-09-21 15:28:12]

So, all good?

https://codereview.webrtc.org/2342443005/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet.h:66: }
On 2016/09/21 10:59:13, kwiberg-webrtc wrote:
> This works, but you get better error messages if you have this function return
a
> bool (in which case it probably deserves a better name...) and then [D]CHECK
> that at each call site.

I'll get more precise location information, but less information about what went
wrong (i.e. which of the levels was bad). It's the Heisenberg uncertainty
priciple of RTC_DCHECK macros. :(

[ossu, 2016-09-21 15:28:22]

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

[commit-bot: I haz the power, 2016-09-21 15:28: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, 2016-09-21 17:28:56]

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

[commit-bot: I haz the power, 2016-09-21 17:28:57]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2016-09-21 17:44:13]

lgtm

https://codereview.webrtc.org/2342443005/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/packet.h (right):

https://codereview.webrtc.org/2342443005/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/packet.h:66: }
On 2016/09/21 15:28:12, ossu wrote:
> On 2016/09/21 10:59:13, kwiberg-webrtc wrote:
> > This works, but you get better error messages if you have this function
return
> a
> > bool (in which case it probably deserves a better name...) and then [D]CHECK
> > that at each call site.
> 
> I'll get more precise location information, but less information about what
went
> wrong (i.e. which of the levels was bad). It's the Heisenberg uncertainty
> priciple of RTC_DCHECK macros. :(

Acknowledged. Keeping it like this is OK.

[ossu, 2016-09-22 08:57:15]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-09-22 08:57:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2342443005/#ps60001 (title:
"Some small fixes.")

[commit-bot: I haz the power, 2016-09-22 08:57:23]

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

[commit-bot: I haz the power, 2016-09-22 09:06:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-22 09:07:09]

Description was changed from

==========
Moved Opus-specific payload splitting into AudioDecoderOpus.

The biggest change to NetEq is the move from a primary flag, to a
Priority with two separate levels: one set by RED splitting and one
set by the codec itself. This allows us to unambigously prioritize
"fallback" packets from these two sources. I've chosen what I believe
is the sensible ordering: packets that the codec prioritizes are
chosen first, regardless of if they are secondary RED packets or
not. So if we were to use Opus w/ FEC in RED, we'd only do Opus FEC
decoding if there was no RED packet that could cover the time slot.

With this change, PayloadSplitter now only deals with RED
packets. Maybe it should be renamed RedPayloadSplitter?

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

to

==========
Moved Opus-specific payload splitting into AudioDecoderOpus.

The biggest change to NetEq is the move from a primary flag, to a
Priority with two separate levels: one set by RED splitting and one
set by the codec itself. This allows us to unambigously prioritize
"fallback" packets from these two sources. I've chosen what I believe
is the sensible ordering: packets that the codec prioritizes are
chosen first, regardless of if they are secondary RED packets or
not. So if we were to use Opus w/ FEC in RED, we'd only do Opus FEC
decoding if there was no RED packet that could cover the time slot.

With this change, PayloadSplitter now only deals with RED
packets. Maybe it should be renamed RedPayloadSplitter?

BUG=webrtc:5805

Committed: https://crrev.com/a70695a3e1e7ead375d4bbc7997cd630d1ac1731
Cr-Commit-Position: refs/heads/master@{#14347}
==========

[commit-bot: I haz the power, 2016-09-22 09:07:10]

Patchset 4 (id:??) landed as
https://crrev.com/a70695a3e1e7ead375d4bbc7997cd630d1ac1731
Cr-Commit-Position: refs/heads/master@{#14347}