RtpPacketizer::NextPacket fills RtpPacket instead of payload.

RtpPacketizer::NextPacket fills RtpPacket instead of just payload.
This push decision if Marker bit should be set into packetizers fixing
issue where returned last_packet flag was ambiguous for some VP9 packets.

Added test for VP9 where last_packet != marker_bit

BUG=webrtc:6723
Committed: https://crrev.com/e545e5d062b050a3797fcf281dad4a92ece87602
Cr-Commit-Position: refs/heads/master@{#15415}

[danilchap, 2016-11-21 17:00:25]

Description was changed from

==========
RtpPacketizer::NextPacket take pointe to RtpPacket instead of payload.
This push decision if Marker bit should be set into packetizer fixing
issue where last_packet wasn't set to true for some VP9 packets.

BUG=webrtc:6723
==========

to

==========
RtpPacketizer::NextPacket take pointer to RtpPacket instead of payload.
This push decision if Marker bit should be set into packetizer fixing
issue where last_packet wasn't set to true for some VP9 packets.

Added test for VP9 where last_packet != marker_bit

BUG=webrtc:6723
==========

[danilchap, 2016-11-21 17:20:48]

Description was changed from

==========
RtpPacketizer::NextPacket take pointer to RtpPacket instead of payload.
This push decision if Marker bit should be set into packetizer fixing
issue where last_packet wasn't set to true for some VP9 packets.

Added test for VP9 where last_packet != marker_bit

BUG=webrtc:6723
==========

to

==========
RtpPacketizer::NextPacket fills RtpPacket instead of just payload.
This push decision if Marker bit should be set into packetizers fixing
issue where last_packet was ambiguous for some VP9 packets.

Added test for VP9 where last_packet != marker_bit

BUG=webrtc:6723
==========

[danilchap, 2016-11-21 17:21:49]

Patchset #1 (id:1) has been deleted

[danilchap, 2016-11-21 17:24:29]

Description was changed from

==========
RtpPacketizer::NextPacket fills RtpPacket instead of just payload.
This push decision if Marker bit should be set into packetizers fixing
issue where last_packet was ambiguous for some VP9 packets.

Added test for VP9 where last_packet != marker_bit

BUG=webrtc:6723
==========

to

==========
RtpPacketizer::NextPacket fills RtpPacket instead of just payload.
This push decision if Marker bit should be set into packetizers fixing
issue where returned last_packet flag was ambiguous for some VP9 packets.

Added test for VP9 where last_packet != marker_bit

BUG=webrtc:6723
==========

[danilchap, 2016-11-21 17:47:50]

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

[danilchap, 2016-11-21 17:47:51]

https://codereview.webrtc.org/2522553002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_format.h (right):

https://codereview.webrtc.org/2522553002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_format.h:41: virtual bool
NextPacket(RtpPacketToSend* packet, bool* last_packet) = 0;
this line is the main change of the CL.

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

https://codereview.webrtc.org/2522553002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:561: *last_packet =
packets_.empty();
these lines are the main reason for the CL.

[stefan-webrtc, 2016-12-01 09:22:37]

stefan@webrtc.org changed reviewers:
+ philipel@webrtc.org

[stefan-webrtc, 2016-12-01 09:22:38]

Philip, could you review this too since you've worked on vp9 packetization?

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

https://codereview.webrtc.org/2522553002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.cc:23: :
packet_(kNoExtensions, 1512),
Name this constant or add a comment.

https://codereview.webrtc.org/2522553002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.cc:126: auto buffer =
packet_.payload();
Is this a complicated type, or could we use the full type name instead of auto?

[stefan-webrtc, 2016-12-01 09:23:07]

lgtm % my comments. but please let philip take a look before committing.

[danilchap, 2016-12-01 09:42:31]

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

https://codereview.webrtc.org/2522553002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.cc:23: :
packet_(kNoExtensions, 1512),
On 2016/12/01 09:22:37, stefan-webrtc (holmer) wrote:
> Name this constant or add a comment.
Thank you, the reason for this odd constants is odd, better fix the reason:

I've updated tests to use more reasonable size (1000 instead of 1500) for
maximum rtp payload size and used default packet size here.

https://codereview.webrtc.org/2522553002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.cc:126: auto buffer =
packet_.payload();
On 2016/12/01 09:22:37, stefan-webrtc (holmer) wrote:
> Is this a complicated type, or could we use the full type name instead of
auto?

not that complicated (rtc::ArrayView<const uint8_t>) but I'm not sure writing it
explicitly makes the check more readable.

[philipel, 2016-12-01 14:00:07]

I haven't really worked on the packetization part of VP9 (only calculated some
info for the frame related to encoding), so I have only made comments regarding
style.

https://codereview.webrtc.org/2522553002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2522553002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:243: if (packet.first_fragment
&& packet.last_fragment) {
Maybe you want to move this block of code to a separate function (to make it
more consistent)?

Not super important though...

https://codereview.webrtc.org/2522553002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.h (right):

https://codereview.webrtc.org/2522553002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.h:45: size_t
buffer_size() const { return payload_size_ + 6; }
A functional change?
constexpr int kTheMagicSix = 6;

[danilchap, 2016-12-01 16:28:27]

https://codereview.webrtc.org/2522553002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2522553002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:243: if (packet.first_fragment
&& packet.last_fragment) {
On 2016/12/01 14:00:07, philipel wrote:
> Maybe you want to move this block of code to a separate function (to make it
> more consistent)?
> 
> Not super important though...

Sorry, not sure what block you mean here.
Can'f see a long enough repeating code fragment here.

https://codereview.webrtc.org/2522553002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.h (right):

https://codereview.webrtc.org/2522553002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.h:45: size_t
buffer_size() const { return payload_size_ + 6; }
On 2016/12/01 14:00:07, philipel wrote:
> A functional change?
> constexpr int kTheMagicSix = 6;

not a functional changed, before this constant was used in the
RtpFormatVp8TestHelper::Init function.
But it definitely deserves a name.

[philipel, 2016-12-02 09:10:26]

lgtm

https://codereview.webrtc.org/2522553002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2522553002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:243: if (packet.first_fragment
&& packet.last_fragment) {
On 2016/12/01 16:28:26, danilchap wrote:
> On 2016/12/01 14:00:07, philipel wrote:
> > Maybe you want to move this block of code to a separate function (to make it
> > more consistent)?
> > 
> > Not super important though...
> 
> Sorry, not sure what block you mean here.
> Can'f see a long enough repeating code fragment here.

I was thinking that you allocate the buffer in this if block but allocate it in
separate functions in the other cases, so for the sake of consistency you wanted
to move if block into a separate function as well. But meh... not really
important.

[danilchap, 2016-12-02 13:00:25]

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

[commit-bot: I haz the power, 2016-12-02 13:00:35]

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

[danilchap, 2016-12-02 13:05:01]

https://codereview.webrtc.org/2522553002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2522553002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:243: if (packet.first_fragment
&& packet.last_fragment) {
On 2016/12/02 09:10:26, philipel wrote:
> On 2016/12/01 16:28:26, danilchap wrote:
> > On 2016/12/01 14:00:07, philipel wrote:
> > > Maybe you want to move this block of code to a separate function (to make
it
> > > more consistent)?
> > > 
> > > Not super important though...
> > 
> > Sorry, not sure what block you mean here.
> > Can'f see a long enough repeating code fragment here.
> 
> I was thinking that you allocate the buffer in this if block but allocate it
in
> separate functions in the other cases, so for the sake of consistency you
wanted
> to move if block into a separate function as well. But meh... not really
> important.

Understood it, thank you. will leave it for another refactoring, prefer to keep
similar to original structure and add AllocatePayload call to same functions
that do matching memcpy.

[commit-bot: I haz the power, 2016-12-02 13:36:26]

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

[commit-bot: I haz the power, 2016-12-02 13:36:27]

Dry run: This issue passed the CQ dry run.

[danilchap, 2016-12-05 10:09:21]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2016-12-05 10:09:24]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2522553002/#ps120001 (title:
"Named kTheMagicSix")

[commit-bot: I haz the power, 2016-12-05 10:09:27]

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-12-05 10:26:44]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480932561999460,
"parent_rev": "71b677dbccba8b5adc06209f4b7eaf2d5fc011a7", "commit_rev":
"5e18f3e4b4811f620a161c1f052ef728c1de762e"}

[commit-bot: I haz the power, 2016-12-05 10:26:47]

Description was changed from

==========
RtpPacketizer::NextPacket fills RtpPacket instead of just payload.
This push decision if Marker bit should be set into packetizers fixing
issue where returned last_packet flag was ambiguous for some VP9 packets.

Added test for VP9 where last_packet != marker_bit

BUG=webrtc:6723
==========

to

==========
RtpPacketizer::NextPacket fills RtpPacket instead of just payload.
This push decision if Marker bit should be set into packetizers fixing
issue where returned last_packet flag was ambiguous for some VP9 packets.

Added test for VP9 where last_packet != marker_bit

BUG=webrtc:6723
==========

[commit-bot: I haz the power, 2016-12-05 10:26:48]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-12-05 10:26:59]

Description was changed from

==========
RtpPacketizer::NextPacket fills RtpPacket instead of just payload.
This push decision if Marker bit should be set into packetizers fixing
issue where returned last_packet flag was ambiguous for some VP9 packets.

Added test for VP9 where last_packet != marker_bit

BUG=webrtc:6723
==========

to

==========
RtpPacketizer::NextPacket fills RtpPacket instead of just payload.
This push decision if Marker bit should be set into packetizers fixing
issue where returned last_packet flag was ambiguous for some VP9 packets.

Added test for VP9 where last_packet != marker_bit

BUG=webrtc:6723

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

[commit-bot: I haz the power, 2016-12-05 10:27:00]

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