Add SetReportBlocks to rtcp Sender/Receive Report classes.

Add SetReportBlocks to rtcp Sender/Receive Report classes.

BUG=None

Review-Url: https://codereview.webrtc.org/2991623002
Cr-Commit-Position: refs/heads/master@{#19136}
Committed: https://chromium.googlesource.com/external/webrtc/+/6209dcdeb13558eac8dc4e2a785adbf19606b7ee

[danilchap, 2017-07-25 09:51:39]

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

[danilchap, 2017-07-25 09:51:40]

To make review steps smaller, I split out adding helpers for
https://codereview.webrtc.org/2979413002/ into own CL, ptal

[eladalon, 2017-07-25 11:46:01]

lgtm

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
(right):

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:140:
blocks[0].SetJitter(2001);
nit: Maybe use 2001u, here and elsewhere, to prevent the reader from a momentary
confusion over the value here appearing different than in the EXPECT_EQ part
(where I know the u suffix is necessary for the compiler).

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:150:
TEST(RtcpPacketReceiverReportTest, CanSetMax31ReportBlock) {
nit: I would have used something like MaxReportBlockLimitObserved, or anything
else that wouldn't use the value as part of the name.

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:158:
EXPECT_FALSE(rr.SetReportBlocks(blocks));
nit: This is not important enough to block (no pun intended), and might even be
subjective, but I'd just like to mention that I think the testing guidelines
would discourage this, with the rationale being that there could be two
different bugs that would still trigger the same test failing:
1. Unable to set 31 blocks (the limit somehow became lower).
2. Able to set 32 blocks (the limit somehow became higher).
By breaking this into two tests, we know immediately what happened as soon as
the failure occurs.

[eladalon, 2017-07-25 11:48:47]

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
(right):

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:152:
const size_t kMaxReportBlocks = (1 << 5) - 1;
Oh, by the way - I would understand if it were:
(1 << some_constant) - 1
But I'm not sure we get extra clarity from
(1 << 5) - 1
Maybe reconsider this? Perhaps we can give the 5 a name? (I see it's also used
in other tests in this file, btw.)

[danilchap, 2017-07-25 12:45:20]

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
(right):

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:140:
blocks[0].SetJitter(2001);
On 2017/07/25 11:46:01, eladalon wrote:
> nit: Maybe use 2001u, here and elsewhere, to prevent the reader from a
momentary
> confusion over the value here appearing different than in the EXPECT_EQ part
> (where I know the u suffix is necessary for the compiler).

Done. Less confusion is better.

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:150:
TEST(RtcpPacketReceiverReportTest, CanSetMax31ReportBlock) {
On 2017/07/25 11:46:01, eladalon wrote:
> nit: I would have used something like MaxReportBlockLimitObserved, or anything
> else that wouldn't use the value as part of the name.

Since exact value moved away from test, moved it away from test name too.

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:152:
const size_t kMaxReportBlocks = (1 << 5) - 1;
On 2017/07/25 11:48:47, eladalon wrote:
> Oh, by the way - I would understand if it were:
> (1 << some_constant) - 1
> But I'm not sure we get extra clarity from
> (1 << 5) - 1
> Maybe reconsider this? Perhaps we can give the 5 a name? (I see it's also used
> in other tests in this file, btw.)

Not sure why I hide the constant in the private sections of the Sender/Receiver
Report classes in the first place: it is fixed by the spec and shouldn't change.
Made it public and use that one in tests.

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:158:
EXPECT_FALSE(rr.SetReportBlocks(blocks));
On 2017/07/25 11:46:01, eladalon wrote:
> nit: This is not important enough to block (no pun intended), and might even
be
> subjective, but I'd just like to mention that I think the testing guidelines
> would discourage this, with the rationale being that there could be two
> different bugs that would still trigger the same test failing:
> 1. Unable to set 31 blocks (the limit somehow became lower).
> 2. Able to set 32 blocks (the limit somehow became higher).
> By breaking this into two tests, we know immediately what happened as soon as
> the failure occurs.

test guide suggest to test distinct behavior in different tests.
I consider this single behavior even if it can break in different ways.

Changed expectations to make them look different.

[danilchap, 2017-07-25 12:45:32]

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

[commit-bot: I haz the power, 2017-07-25 12:45:41]

Dry run: CQ is trying da patch.

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

[eladalon, 2017-07-25 13:26:10]

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
(right):

https://codereview.webrtc.org/2991623002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:158:
EXPECT_FALSE(rr.SetReportBlocks(blocks));
On 2017/07/25 12:45:20, danilchap wrote:
> On 2017/07/25 11:46:01, eladalon wrote:
> > nit: This is not important enough to block (no pun intended), and might even
> be
> > subjective, but I'd just like to mention that I think the testing guidelines
> > would discourage this, with the rationale being that there could be two
> > different bugs that would still trigger the same test failing:
> > 1. Unable to set 31 blocks (the limit somehow became lower).
> > 2. Able to set 32 blocks (the limit somehow became higher).
> > By breaking this into two tests, we know immediately what happened as soon
as
> > the failure occurs.
> 
> test guide suggest to test distinct behavior in different tests.
> I consider this single behavior even if it can break in different ways.
> 
> Changed expectations to make them look different.

The guide discourages performing actions on the object under test, after a test
has been performed. But I agree that this case is very subjective, and it can be
very strongly argued that having two tests for the observation of a limit might
actually be less clear, rather than more. I think I'll check this like you in
future tests I'll write.

https://codereview.webrtc.org/2991623002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report.h (right):

https://codereview.webrtc.org/2991623002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report.h:27: static
constexpr size_t kMaxNumberOfReportBlocks = 0x1f;
(Not actionable.)
Does this correspond to a field of 5 bits? I wonder why the limit isn't 32, btw.
Is it every possible to have 0 blocks? If not, they could have made 0 mean 1
report, 1 mean 2 report, etc., up to 31 meaning 32 reports.

https://codereview.webrtc.org/2991623002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
(right):

https://codereview.webrtc.org/2991623002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:127:
rb.SetMediaSsrc(kRemoteSsrc - 1);
IMHO, as a second pair of eyes, it was clearer with (kRemoteSsrc  +
ReceiverReport::kMaxNumberOfReportBlocks). If you want to make it even clearer,
while not spilling the line over, maybe consider the following version?

  size_t i;
  for (i = 0; i < kMaxReportBlocks; ++i) {
    rb.SetMediaSsrc(kRemoteSsrc + i);
    EXPECT_TRUE(rr.AddReportBlock(rb));
  }
  rb.SetMediaSsrc(kRemoteSsrc + i);
  EXPECT_FALSE(rr.AddReportBlock(rb));

(But either is fine, really.)

https://codereview.webrtc.org/2991623002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc (right):

https://codereview.webrtc.org/2991623002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc:23: constexpr size_t
SenderReport::kMaxNumberOfReportBlocks;
I don't think these two are necessary. Maybe only for certain compilers?
Locally, at least, I don't need this.

[commit-bot: I haz the power, 2017-07-25 13:32:13]

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

[commit-bot: I haz the power, 2017-07-25 13:32:14]

Dry run: This issue passed the CQ dry run.

[danilchap, 2017-07-25 13:39:00]

https://codereview.webrtc.org/2991623002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report.h (right):

https://codereview.webrtc.org/2991623002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report.h:27: static
constexpr size_t kMaxNumberOfReportBlocks = 0x1f;
On 2017/07/25 13:26:09, eladalon wrote:
> (Not actionable.)
> Does this correspond to a field of 5 bits? I wonder why the limit isn't 32,
btw.
> Is it every possible to have 0 blocks? If not, they could have made 0 mean 1
> report, 1 mean 2 report, etc., up to 31 meaning 32 reports.

yes, there are 5 bits to store number of report blocks.
0 report blocks is valid and common (specially for sender reports)
See rfc3550, section 6.4.1/6.4.2

https://codereview.webrtc.org/2991623002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
(right):

https://codereview.webrtc.org/2991623002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:127:
rb.SetMediaSsrc(kRemoteSsrc - 1);
On 2017/07/25 13:26:09, eladalon wrote:
> IMHO, as a second pair of eyes, it was clearer with (kRemoteSsrc  +
> ReceiverReport::kMaxNumberOfReportBlocks). If you want to make it even
clearer,
> while not spilling the line over, maybe consider the following version?
> 
>   size_t i;
>   for (i = 0; i < kMaxReportBlocks; ++i) {
>     rb.SetMediaSsrc(kRemoteSsrc + i);
>     EXPECT_TRUE(rr.AddReportBlock(rb));
>   }
>   rb.SetMediaSsrc(kRemoteSsrc + i);
>   EXPECT_FALSE(rr.AddReportBlock(rb));
> 
> (But either is fine, really.)

I'll add + kMax back since it is clearer.

https://codereview.webrtc.org/2991623002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc (right):

https://codereview.webrtc.org/2991623002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc:23: constexpr size_t
SenderReport::kMaxNumberOfReportBlocks;
On 2017/07/25 13:26:09, eladalon wrote:
> I don't think these two are necessary. Maybe only for certain compilers?
> Locally, at least, I don't need this.

it might not be used right now, but when it is needed and absent, error message
might be surprising.

https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#the-c...

[danilchap, 2017-07-25 14:30:49]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2017-07-25 14:30:49]

The patchset sent to the CQ was uploaded after l-g-t-m from eladalon@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2991623002/#ps40001 (title:
"one more comment")

[commit-bot: I haz the power, 2017-07-25 14:30:52]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-25 14:33:42]

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

[commit-bot: I haz the power, 2017-07-25 14:33:43]

Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/22270)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/23404)

[danilchap, 2017-07-25 14:36:39]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2017-07-25 14:36:40]

The patchset sent to the CQ was uploaded after l-g-t-m from eladalon@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2991623002/#ps60001 (title:
"fix typo")

[commit-bot: I haz the power, 2017-07-25 14:36:42]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-25 15:07:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1500993399642470,
"parent_rev": "fb143127d753d71a9775b2d4fd888f0f5f43ce1c", "commit_rev":
"6209dcdeb13558eac8dc4e2a785adbf19606b7ee"}

[commit-bot: I haz the power, 2017-07-25 15:07:18]

Description was changed from

==========
Add SetReportBlocks to rtcp Sender/Receive Report classes.

BUG=None
==========

to

==========
Add SetReportBlocks to rtcp Sender/Receive Report classes.

BUG=None

Review-Url: https://codereview.webrtc.org/2991623002
Cr-Commit-Position: refs/heads/master@{#19136}
Committed:
https://chromium.googlesource.com/external/webrtc/+/6209dcdeb13558eac8dc4e2a7...
==========

[commit-bot: I haz the power, 2017-07-25 15:07:20]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/6209dcdeb13558eac8dc4e2a7...