Always append the BYE packet type at the end

Always append the BYE packet type at the end

When composing a RTCP packet, if there is a BYE
to be appended, preserve it and append it at the
end after all other packet types are added.

BUG=webrtc:5498
NOTRY=true

Committed: https://crrev.com/0e2e50ca1c83c9ad9ba61149fbcd704b514b3e13
Cr-Commit-Position: refs/heads/master@{#11672}

[pbos-webrtc, 2016-02-06 12:48:36]

pbos@webrtc.org changed reviewers:
+ danilchap@webrtc.org, pbos@webrtc.org, stefan@webrtc.org

[pbos-webrtc, 2016-02-06 12:48:37]

danilchap@ can you help review this + suggest tests to be added?

[danilchap, 2016-02-08 10:44:29]

Nice patch.
A test would be nice to ensure later changes will not put bye packet back in the
middle of a compound rtcp packet.
Here is one of the ways to add it to rtcp_sender_unittest.cc :
test::RtcpPacketParser doesn't worry about the order, so need a different
parser. Something like that:
const uint8_t* next_packet = data;
while (next_packet < data+len) {
  RtcpCommonHeader header;
  ParseHeader(next_packet, &header);
  next_packet = next_packet + header.packet_size_bytes;
  bool is_last_packet = (data + len == next_packet);
  if (header.packet_type == kByeType) {
    EXPECT_TRUE(is_last_packet) << "Bye packet should be last in a compound RTCP
packet.";
  }
}

Create a MockTransport with pseudocode above in the mock of SendRtcp (check
gmock). In your test recreate rtcp_sender_ with this MockTransport instead of
TestTransport. Ensure test breaks before your change.

Other than missing test just some style comments.

https://codereview.webrtc.org/1674963004/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/1674963004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:838:
rtc::scoped_ptr<rtcp::RtcpPacket> packetWithBye = nullptr;
Local variables use snake_case.
This variable doesn't hold packet with bye, but instead bye packet itself, may
be call it 'packet_bye' or 'bye_packet' or just 'bye'.
Smart pointers do not require nullptr initialization.

https://codereview.webrtc.org/1674963004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:854: // if there is a BYE, don't
append now - save it and append it
Comments better be started with an Upper letter and end with a dot.

https://codereview.webrtc.org/1674963004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:857:
packetWithBye.reset(packet.release());
packetWithBye = std::move(packet); is a standard way to move a smart pointer

https://codereview.webrtc.org/1674963004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:864: if (packetWithBye != nullptr)
{
"if (packetWithBye)" is preferred way to check if pointer holds something

[aleung, 2016-02-10 04:52:58]

On 2016/02/08 10:44:29, danilchap wrote:
> Nice patch.
> A test would be nice to ensure later changes will not put bye packet back in
the
> middle of a compound rtcp packet.
> Here is one of the ways to add it to rtcp_sender_unittest.cc :
> test::RtcpPacketParser doesn't worry about the order, so need a different
> parser. Something like that:
> const uint8_t* next_packet = data;
> while (next_packet < data+len) {
>   RtcpCommonHeader header;
>   ParseHeader(next_packet, &header);
>   next_packet = next_packet + header.packet_size_bytes;
>   bool is_last_packet = (data + len == next_packet);
>   if (header.packet_type == kByeType) {
>     EXPECT_TRUE(is_last_packet) << "Bye packet should be last in a compound
RTCP
> packet.";
>   }
> }
> 
> Create a MockTransport with pseudocode above in the mock of SendRtcp (check
> gmock). In your test recreate rtcp_sender_ with this MockTransport instead of
> TestTransport. Ensure test breaks before your change.
> 
> Other than missing test just some style comments.
> 
>
https://codereview.webrtc.org/1674963004/diff/1/webrtc/modules/rtp_rtcp/sourc...
> File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):
> 
>
https://codereview.webrtc.org/1674963004/diff/1/webrtc/modules/rtp_rtcp/sourc...
> webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:838:
> rtc::scoped_ptr<rtcp::RtcpPacket> packetWithBye = nullptr;
> Local variables use snake_case.
> This variable doesn't hold packet with bye, but instead bye packet itself, may
> be call it 'packet_bye' or 'bye_packet' or just 'bye'.
> Smart pointers do not require nullptr initialization.
> 
>
https://codereview.webrtc.org/1674963004/diff/1/webrtc/modules/rtp_rtcp/sourc...
> webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:854: // if there is a BYE, don't
> append now - save it and append it
> Comments better be started with an Upper letter and end with a dot.
> 
>
https://codereview.webrtc.org/1674963004/diff/1/webrtc/modules/rtp_rtcp/sourc...
> webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:857:
> packetWithBye.reset(packet.release());
> packetWithBye = std::move(packet); is a standard way to move a smart pointer
> 
>
https://codereview.webrtc.org/1674963004/diff/1/webrtc/modules/rtp_rtcp/sourc...
> webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:864: if (packetWithBye !=
nullptr)
> {
> "if (packetWithBye)" is preferred way to check if pointer holds something

Thanks for the review.  I updated the source change per the review.  I will add
unit test next.

[danilchap, 2016-02-11 13:35:30]

https://codereview.webrtc.org/1674963004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc (right):

https://codereview.webrtc.org/1674963004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:22: #include
"webrtc/modules/rtp_rtcp/source/rtcp_utility.h"
Alphabetical order.

https://codereview.webrtc.org/1674963004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:221: // This is a mock
transport class which is solely used
This mock is used in one test only, may be move it there.
This way the main check (EXPECT_TRUE(is_last_packet)) would be inside the test.
i.e.
TEST_F(RtcpSenderTest, ByeMustBeLast) {
  class MockTransportExpectsByeIsLastInCompoundRtcpPacket : public Transport {
    ...
  } mock_transport;
  // Re-configure rtcp_sender_ with mock_transport
  rtcp_sender_.reset(new RTCPSender(..., &mock_transport));
  ...
  
or you can use gmock:
#include "webrtc/test/mock_transport.h"
using ::testing::_;
using ::testing::ElementsAre;
using ::testing::Invoke;
...
TEST_F(RtcpSenderTest, ByeMustBeLast) {
  MockTransport mock_transport;
  EXPECT_CALL(mock_transport, SendRtcp(_, _))
      .WillOnce(Invoke([](const uint8_t* data, size_t len) {
        const uint8_t* next_packet = data;
        ...
        return true;
      }));
  // Re-configure rtcp_sender_ with mock_transport
  rtcp_sender_.reset(new RTCPSender(..., &mock_transport));

https://codereview.webrtc.org/1674963004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:225: public NullRtpData {
any reason to derive it from NullRtpData?

https://codereview.webrtc.org/1674963004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:231: const PacketOptions&
options) override {
for consistency comment /*options*/ parameter too.

https://codereview.webrtc.org/1674963004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:821: metric.lossRate = 1;
Are VoIPMetric values have any meaning for this test? may be can remove them to
make test shorter and concentrated on the bye packet.
i.e.
RTCPVoIPMetric metric;
rtcp_sender_->SetRTCPVoIPMetrics(&metric);

[danilchap, 2016-02-12 10:13:21]

lgtm

[danilchap, 2016-02-12 10:13:47]

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

[commit-bot: I haz the power, 2016-02-12 10:13:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674963004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674963004/60001

[commit-bot: I haz the power, 2016-02-12 11:49:16]

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

[commit-bot: I haz the power, 2016-02-12 11:49:17]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2016-02-17 09:47:04]

lgtm with comment removed

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

https://codereview.webrtc.org/1674963004/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:775: // details.
I think we can remove the link to the bug, as this CL actually fixes the bug.

[danilchap, 2016-02-18 09:01:26]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2016-02-18 09:01:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1674963004/#ps80001 (title:
"Removed Comment")

[commit-bot: I haz the power, 2016-02-18 09:02:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674963004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674963004/80001

[commit-bot: I haz the power, 2016-02-18 09:12:53]

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

[commit-bot: I haz the power, 2016-02-18 09:12:54]

Try jobs failed on following builders:
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/11005)

[pbos-webrtc, 2016-02-18 10:39:43]

The CQ bit was checked by pbos@webrtc.org

[commit-bot: I haz the power, 2016-02-18 10:39:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674963004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674963004/80001

[commit-bot: I haz the power, 2016-02-18 10:48:04]

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

[commit-bot: I haz the power, 2016-02-18 10:48:05]

Try jobs failed on following builders:
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/11008)

[danilchap, 2016-02-18 16:31:33]

Description was changed from

==========
Always append the BYE packet type at the end

When composing a RTCP packet, if there is a BYE
to be appended, preserve it and append it at the
end after all other packet types are added.

BUG=webrtc:5498
==========

to

==========
Always append the BYE packet type at the end

When composing a RTCP packet, if there is a BYE
to be appended, preserve it and append it at the
end after all other packet types are added.

BUG=webrtc:5498
NOTRY=true
==========

[danilchap, 2016-02-18 16:31:54]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-02-18 16:32:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674963004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674963004/80001

[commit-bot: I haz the power, 2016-02-18 16:33:29]

Description was changed from

==========
Always append the BYE packet type at the end

When composing a RTCP packet, if there is a BYE
to be appended, preserve it and append it at the
end after all other packet types are added.

BUG=webrtc:5498
NOTRY=true
==========

to

==========
Always append the BYE packet type at the end

When composing a RTCP packet, if there is a BYE
to be appended, preserve it and append it at the
end after all other packet types are added.

BUG=webrtc:5498
NOTRY=true
==========

[commit-bot: I haz the power, 2016-02-18 16:33:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-18 16:33:38]

Description was changed from

==========
Always append the BYE packet type at the end

When composing a RTCP packet, if there is a BYE
to be appended, preserve it and append it at the
end after all other packet types are added.

BUG=webrtc:5498
NOTRY=true
==========

to

==========
Always append the BYE packet type at the end

When composing a RTCP packet, if there is a BYE
to be appended, preserve it and append it at the
end after all other packet types are added.

BUG=webrtc:5498
NOTRY=true

Committed: https://crrev.com/0e2e50ca1c83c9ad9ba61149fbcd704b514b3e13
Cr-Commit-Position: refs/heads/master@{#11672}
==========

[commit-bot: I haz the power, 2016-02-18 16:33:39]

Patchset 5 (id:??) landed as
https://crrev.com/0e2e50ca1c83c9ad9ba61149fbcd704b514b3e13
Cr-Commit-Position: refs/heads/master@{#11672}