Style fixes in FecReceiver and ProducerFec unit tests. (pt. 7)

Style fixes in FecReceiver and ProducerFec unit tests.

- Change some types to size_t.
- Update parameter ordering.
- Run 'git cl format'
- Moved 'using declarations' into unnamed namespaces.
- Removed "::webrtc::" prefix from some using declarations.

BUG=webrtc:5654
Committed: https://crrev.com/71ca74798af8fd33a55ddd462945289b920103f2
Cr-Commit-Position: refs/heads/master@{#14475}

[brandtr, 2016-08-25 14:40:11]

Description was changed from

==========
Style fixes in FecReceiver and ProducerFec.

BUG=webrtc:5654
==========

to

==========
Style fixes in FecReceiver and ProducerFec.

- Change some types to size_t.
- Use constexpr.
- Run 'git cl format'

BUG=webrtc:5654
==========

[brandtr, 2016-08-25 14:47:18]

Description was changed from

==========
Style fixes in FecReceiver and ProducerFec.

- Change some types to size_t.
- Use constexpr.
- Run 'git cl format'

BUG=webrtc:5654
==========

to

==========
Style fixes in FecReceiver and ProducerFec.

- Change some types to size_t.
- Use constexpr.
- Parameter ordering.
- Run 'git cl format'

BUG=webrtc:5654
==========

[brandtr, 2016-08-25 14:48:00]

Patchset #1 (id:1) has been deleted

[brandtr, 2016-08-30 07:15:30]

Description was changed from

==========
Style fixes in FecReceiver and ProducerFec.

- Change some types to size_t.
- Use constexpr.
- Parameter ordering.
- Run 'git cl format'

BUG=webrtc:5654
==========

to

==========
Style fixes in FecReceiver and ProducerFec unit tests.

- Change some types to size_t.
- Use constexpr.
- Parameter ordering.
- Run 'git cl format'

BUG=webrtc:5654
==========

[brandtr, 2016-08-30 07:15:36]

Rebase.

[brandtr, 2016-08-30 14:35:32]

Description was changed from

==========
Style fixes in FecReceiver and ProducerFec unit tests.

- Change some types to size_t.
- Use constexpr.
- Parameter ordering.
- Run 'git cl format'

BUG=webrtc:5654
==========

to

==========
Style fixes in FecReceiver and ProducerFec unit tests.

- Change some types to size_t.
- Use constexpr where applicable.
- Update parameter ordering.
- Run 'git cl format'

BUG=webrtc:5654
==========

[brandtr, 2016-09-02 09:43:06]

Rebase.

[brandtr, 2016-09-05 12:20:42]

brandtr@webrtc.org changed reviewers:
+ danilchap@webrtc.org

[brandtr, 2016-09-05 12:20:43]

PTAL at this minor cleanup.

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right):

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:107: constexpr size_t
kNumFecPackets = 1u;
Do you think these should be constexpr or const...?

[danilchap, 2016-09-05 13:03:33]

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right):

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:68: void
VerifyReconstructedMediaPacket(const RawRtpPacket* packet,
may be const& since it can't be nullptr

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:107: constexpr size_t
kNumFecPackets = 1u;
On 2016/09/05 12:20:43, brandtr wrote:
> Do you think these should be constexpr or const...?

I would go for const.
constexpr is more for giving a name to some global constant.
Or when you need the value of the constant at compile time.
Here it is more like a parameter to the test.

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:127: EXPECT_EQ(2u,
counter.num_packets);
prefer Uppercase suffix.
Thought there is no rule about it, examples in the style guide use uppercase
suffix.

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:136: constexpr size_t
kNumFecPackets = 1u;
No need for suffix in this case, so better not to use it.
i.e. = 1; instead of = 1u;
The type already clearly states it is unsigned.

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right):

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:34: uint8_t
red_payload_type,
this is a questionable change.
I wouldn't push either direction in this case.
seq_num and timestamp can use full range of uint16_t/uint32_t, also they rely on
wrapping,
Correct range of values for payload type can be represented by uint8_t, but less
than full range (it is even less than (0,127], there are some reserved values.)
and doesn't rely on any special properties of uint8_t.
So generic 'int' (some integer) might be a better choice.

On the other side this function is very close to byte manipulation.

[brandtr, 2016-09-05 14:11:18]

Patchset #4 (id:80001) has been deleted

[brandtr, 2016-09-05 14:17:19]

Rebase

[brandtr, 2016-09-05 14:18:44]

Patchset #4 (id:100001) has been deleted

[brandtr, 2016-09-05 14:20:49]

Thanks for quick feedback!

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right):

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:68: void
VerifyReconstructedMediaPacket(const RawRtpPacket* packet,
On 2016/09/05 13:03:32, danilchap wrote:
> may be const& since it can't be nullptr

Good call, done.

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:107: constexpr size_t
kNumFecPackets = 1u;
On 2016/09/05 13:03:32, danilchap wrote:
> On 2016/09/05 12:20:43, brandtr wrote:
> > Do you think these should be constexpr or const...?
> 
> I would go for const.
> constexpr is more for giving a name to some global constant.
> Or when you need the value of the constant at compile time.
> Here it is more like a parameter to the test.

Done. I'm keeping the kCapitalization though, since it's optional for variables
with automatic storage duration.

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:127: EXPECT_EQ(2u,
counter.num_packets);
On 2016/09/05 13:03:32, danilchap wrote:
> prefer Uppercase suffix.
> Thought there is no rule about it, examples in the style guide use uppercase
> suffix.

Most of the FEC code uses lowercase for this, but I'll start using upper case if
the style suggests so :)

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:136: constexpr size_t
kNumFecPackets = 1u;
On 2016/09/05 13:03:32, danilchap wrote:
> No need for suffix in this case, so better not to use it.
> i.e. = 1; instead of = 1u;
> The type already clearly states it is unsigned.

Done.

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right):

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:34: uint8_t
red_payload_type,
On 2016/09/05 13:03:33, danilchap wrote:
> this is a questionable change.
> I wouldn't push either direction in this case.
> seq_num and timestamp can use full range of uint16_t/uint32_t, also they rely
on
> wrapping,
> Correct range of values for payload type can be represented by uint8_t, but
less
> than full range (it is even less than (0,127], there are some reserved
values.)
> and doesn't rely on any special properties of uint8_t.
> So generic 'int' (some integer) might be a better choice.
> 
> On the other side this function is very close to byte manipulation.

Yep, I'm aware of these subtleties (except that there are some reserved payload
types!), but I changed here because I've seen uint8_t being used for the payload
type in other places in the FEC code. But now I can only find places where it is
used as an int, so that's probably the better choice.

[brandtr, 2016-09-06 08:09:36]

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right):

https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:34: uint8_t
red_payload_type,
On 2016/09/05 14:20:49, brandtr wrote:
> On 2016/09/05 13:03:33, danilchap wrote:
> > this is a questionable change.
> > I wouldn't push either direction in this case.
> > seq_num and timestamp can use full range of uint16_t/uint32_t, also they
rely
> on
> > wrapping,
> > Correct range of values for payload type can be represented by uint8_t, but
> less
> > than full range (it is even less than (0,127], there are some reserved
> values.)
> > and doesn't rely on any special properties of uint8_t.
> > So generic 'int' (some integer) might be a better choice.
> > 
> > On the other side this function is very close to byte manipulation.
> 
> Yep, I'm aware of these subtleties (except that there are some reserved
payload
> types!), but I changed here because I've seen uint8_t being used for the
payload
> type in other places in the FEC code. But now I can only find places where it
is
> used as an int, so that's probably the better choice.

Found it!

In the chain of "SetGenericFECStatus" functions, the type is an uint8_t:
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour...

But in the top-level |FecConfig|, it's an int:
https://cs.chromium.org/chromium/src/third_party/webrtc/config.h?q=fecconfig+...

[danilchap, 2016-09-06 08:13:53]

On 2016/09/06 08:09:36, brandtr wrote:
>
https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
> File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2273353002/diff/60001/webrtc/modules/rtp_rtcp/s...
> webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:34: uint8_t
> red_payload_type,
> On 2016/09/05 14:20:49, brandtr wrote:
> > On 2016/09/05 13:03:33, danilchap wrote:
> > > this is a questionable change.
> > > I wouldn't push either direction in this case.
> > > seq_num and timestamp can use full range of uint16_t/uint32_t, also they
> rely
> > on
> > > wrapping,
> > > Correct range of values for payload type can be represented by uint8_t,
but
> > less
> > > than full range (it is even less than (0,127], there are some reserved
> > values.)
> > > and doesn't rely on any special properties of uint8_t.
> > > So generic 'int' (some integer) might be a better choice.
> > > 
> > > On the other side this function is very close to byte manipulation.
> > 
> > Yep, I'm aware of these subtleties (except that there are some reserved
> payload
> > types!), but I changed here because I've seen uint8_t being used for the
> payload
> > type in other places in the FEC code. But now I can only find places where
it
> is
> > used as an int, so that's probably the better choice.
> 
> Found it!
> 
> In the chain of "SetGenericFECStatus" functions, the type is an uint8_t:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour...
> 
> But in the top-level |FecConfig|, it's an int:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/config.h?q=fecconfig+...

There I would say RtpSenderVideo's SetGenericFECStatus should switch to int
eventually. That function is not close to byte manipulations.
No rush.

[brandtr, 2016-09-06 08:18:32]

Agree.

I will anyway change (or at least rename) that function when I hook up the
config "setters" for FlexFEC.

[brandtr, 2016-09-06 14:36:45]

Rebase.

[brandtr, 2016-09-12 10:37:35]

Description was changed from

==========
Style fixes in FecReceiver and ProducerFec unit tests.

- Change some types to size_t.
- Use constexpr where applicable.
- Update parameter ordering.
- Run 'git cl format'

BUG=webrtc:5654
==========

to

==========
Style fixes in FecReceiver and ProducerFec unit tests.

- Change some types to size_t.
- Update parameter ordering.
- Run 'git cl format'

BUG=webrtc:5654
==========

[brandtr, 2016-09-21 10:52:17]

Rebase.

[brandtr, 2016-09-21 12:09:29]

Description was changed from

==========
Style fixes in FecReceiver and ProducerFec unit tests.

- Change some types to size_t.
- Update parameter ordering.
- Run 'git cl format'

BUG=webrtc:5654
==========

to

==========
Style fixes in FecReceiver and ProducerFec unit tests.

- Change some types to size_t.
- Update parameter ordering.
- Run 'git cl format'
- Moved 'using declarations' into unnamed namespaces.
- Removed "::webrtc::" prefix from some using declarations.

BUG=webrtc:5654
==========

[danilchap, 2016-09-21 16:06:27]

lgtm

[brandtr, 2016-09-27 08:20:58]

Rebase.

[brandtr, 2016-09-30 13:12:56]

Rebase.

[brandtr, 2016-10-03 12:01:26]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2016-10-03 12:01:27]

The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2273353002/#ps220001 (title:
"Rebase.")

[commit-bot: I haz the power, 2016-10-03 12:01:35]

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-10-03 12:11:28]

Description was changed from

==========
Style fixes in FecReceiver and ProducerFec unit tests.

- Change some types to size_t.
- Update parameter ordering.
- Run 'git cl format'
- Moved 'using declarations' into unnamed namespaces.
- Removed "::webrtc::" prefix from some using declarations.

BUG=webrtc:5654
==========

to

==========
Style fixes in FecReceiver and ProducerFec unit tests.

- Change some types to size_t.
- Update parameter ordering.
- Run 'git cl format'
- Moved 'using declarations' into unnamed namespaces.
- Removed "::webrtc::" prefix from some using declarations.

BUG=webrtc:5654
==========

[commit-bot: I haz the power, 2016-10-03 12:11:29]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2016-10-03 12:11:39]

Description was changed from

==========
Style fixes in FecReceiver and ProducerFec unit tests.

- Change some types to size_t.
- Update parameter ordering.
- Run 'git cl format'
- Moved 'using declarations' into unnamed namespaces.
- Removed "::webrtc::" prefix from some using declarations.

BUG=webrtc:5654
==========

to

==========
Style fixes in FecReceiver and ProducerFec unit tests.

- Change some types to size_t.
- Update parameter ordering.
- Run 'git cl format'
- Moved 'using declarations' into unnamed namespaces.
- Removed "::webrtc::" prefix from some using declarations.

BUG=webrtc:5654

Committed: https://crrev.com/71ca74798af8fd33a55ddd462945289b920103f2
Cr-Commit-Position: refs/heads/master@{#14475}
==========

[commit-bot: I haz the power, 2016-10-03 12:11:40]

Patchset 9 (id:??) landed as
https://crrev.com/71ca74798af8fd33a55ddd462945289b920103f2
Cr-Commit-Position: refs/heads/master@{#14475}