Replace spatial layers with padding packets in LayerFilteringTransport

Replace spatial layers with padding packets in LayerFilteringTransport

The old code (https://codereview.webrtc.org/1287643002/) completely removed the
packets containing the higher spatial layers, which affected the bitrate estimator
and gave unexpected results. Here we replace those packets with padding packets.

The padding packet has at most 255 + (header length) bytes, so the original
packet has to be split into multiple pieces.

EDIT: This works better, but still the loopback test seems to behave somewhat differently
depending on which layer is analyzed. (i.e. whether the higher layer is discarded or not)

[ivica, 2015-09-10 08:07:30]

ivica@webrtc.org changed reviewers:
+ mflodman@webrtc.org, sprang@webrtc.org

[ivica, 2015-09-10 08:07:30]

Improving LayerFilteringTransport.

[sprang_webrtc, 2015-09-10 09:21:54]

https://codereview.webrtc.org/1327153002/diff/1/webrtc/test/layer_filtering_t...
File webrtc/test/layer_filtering_transport.cc (right):

https://codereview.webrtc.org/1327153002/diff/1/webrtc/test/layer_filtering_t...
webrtc/test/layer_filtering_transport.cc:98: for (size_t left = length; left;) {
I'd rather have
size_t bytes_left = length;
while (bytes_left > 0) {

https://codereview.webrtc.org/1327153002/diff/1/webrtc/test/layer_filtering_t...
webrtc/test/layer_filtering_transport.cc:103: // Let the next packet have at
least 5 padding bytes.
Why 5?

https://codereview.webrtc.org/1327153002/diff/1/webrtc/test/layer_filtering_t...
webrtc/test/layer_filtering_transport.cc:111:
ByteWriter<uint16_t>::WriteBigEndian(&temp_buffer[2], seq_num);
Not sure I you will also have to add AbsSendTime extension header to get BWE to
work well... See RTPSender::UpdateAbsoluteSendTime
Since we're sending back-to-back here, might be enough to copy entire header of
original packet, only nulling out the payload and changing it to padding.

https://codereview.webrtc.org/1327153002/diff/1/webrtc/test/layer_filtering_t...
webrtc/test/layer_filtering_transport.cc:115: DCHECK(result);
Perhaps also DCHECK_GE(left, packet_length);

[ivica, 2015-09-10 09:44:46]

https://codereview.webrtc.org/1327153002/diff/1/webrtc/test/layer_filtering_t...
File webrtc/test/layer_filtering_transport.cc (right):

https://codereview.webrtc.org/1327153002/diff/1/webrtc/test/layer_filtering_t...
webrtc/test/layer_filtering_transport.cc:98: for (size_t left = length; left;) {
On 2015/09/10 09:21:54, språng wrote:
> I'd rather have
> size_t bytes_left = length;
> while (bytes_left > 0) {

Done.

https://codereview.webrtc.org/1327153002/diff/1/webrtc/test/layer_filtering_t...
webrtc/test/layer_filtering_transport.cc:103: // Let the next packet have at
least 5 padding bytes.
On 2015/09/10 09:21:54, språng wrote:
> Why 5?

Changed to at least 1. It is because the last byte of the packet is changed to
be equal to the padding_length. If there are no padding bytes, that would be a
special case...

https://codereview.webrtc.org/1327153002/diff/1/webrtc/test/layer_filtering_t...
webrtc/test/layer_filtering_transport.cc:111:
ByteWriter<uint16_t>::WriteBigEndian(&temp_buffer[2], seq_num);
On 2015/09/10 09:21:54, språng wrote:
> Not sure I you will also have to add AbsSendTime extension header to get BWE
to
> work well... See RTPSender::UpdateAbsoluteSendTime
> Since we're sending back-to-back here, might be enough to copy entire header
of
> original packet, only nulling out the payload and changing it to padding.

temp_buffer is already a copy of the old packet. (there is a memcpy on line 85)

https://codereview.webrtc.org/1327153002/diff/1/webrtc/test/layer_filtering_t...
webrtc/test/layer_filtering_transport.cc:115: DCHECK(result);
On 2015/09/10 09:21:54, språng wrote:
> Perhaps also DCHECK_GE(left, packet_length);

Done.

[sprang_webrtc, 2015-09-10 12:36:59]

Looks good, just some minor comments.

https://codereview.webrtc.org/1327153002/diff/1/webrtc/test/layer_filtering_t...
File webrtc/test/layer_filtering_transport.cc (right):

https://codereview.webrtc.org/1327153002/diff/1/webrtc/test/layer_filtering_t...
webrtc/test/layer_filtering_transport.cc:111:
ByteWriter<uint16_t>::WriteBigEndian(&temp_buffer[2], seq_num);
On 2015/09/10 09:44:46, ivica wrote:
> On 2015/09/10 09:21:54, språng wrote:
> > Not sure I you will also have to add AbsSendTime extension header to get BWE
> to
> > work well... See RTPSender::UpdateAbsoluteSendTime
> > Since we're sending back-to-back here, might be enough to copy entire header
> of
> > original packet, only nulling out the payload and changing it to padding.
> 
> temp_buffer is already a copy of the old packet. (there is a memcpy on line
85)

Acknowledged.

https://codereview.webrtc.org/1327153002/diff/20001/webrtc/test/layer_filteri...
File webrtc/test/layer_filtering_transport.cc (right):

https://codereview.webrtc.org/1327153002/diff/20001/webrtc/test/layer_filteri...
webrtc/test/layer_filtering_transport.cc:92: }
remove {}

https://codereview.webrtc.org/1327153002/diff/20001/webrtc/test/layer_filteri...
webrtc/test/layer_filtering_transport.cc:97: temp_buffer[1] &=
~kRtpMarkerBitMask;
Just to make the code easier to follow how about some named constants:
const size_t kMaxPaddingBytes = 255;
const size_t kMinPaddingBytes = 1;
Then replace below where applicable.

https://codereview.webrtc.org/1327153002/diff/20001/webrtc/test/layer_filteri...
webrtc/test/layer_filtering_transport.cc:101: if (bytes_left > 255 +
header.headerLength * 2) {
// Room left for another padding packet even after emitting one of maximum size.

https://codereview.webrtc.org/1327153002/diff/20001/webrtc/test/layer_filteri...
webrtc/test/layer_filtering_transport.cc:103: } else if (bytes_left >= 255 +
header.headerLength) {
If we make this padding packet the maximum size, the remained of the buffer
won't fit a packet of minimum size.

[ivica, 2015-09-10 14:21:36]

https://codereview.webrtc.org/1327153002/diff/20001/webrtc/test/layer_filteri...
File webrtc/test/layer_filtering_transport.cc (right):

https://codereview.webrtc.org/1327153002/diff/20001/webrtc/test/layer_filteri...
webrtc/test/layer_filtering_transport.cc:92: }
On 2015/09/10 12:36:58, språng wrote:
> remove {}

Done.

https://codereview.webrtc.org/1327153002/diff/20001/webrtc/test/layer_filteri...
webrtc/test/layer_filtering_transport.cc:97: temp_buffer[1] &=
~kRtpMarkerBitMask;
On 2015/09/10 12:36:58, språng wrote:
> Just to make the code easier to follow how about some named constants:
> const size_t kMaxPaddingBytes = 255;
> const size_t kMinPaddingBytes = 1;
> Then replace below where applicable.

Done.

https://codereview.webrtc.org/1327153002/diff/20001/webrtc/test/layer_filteri...
webrtc/test/layer_filtering_transport.cc:101: if (bytes_left > 255 +
header.headerLength * 2) {
On 2015/09/10 12:36:59, språng wrote:
> // Room left for another padding packet even after emitting one of maximum
size.

Done.

https://codereview.webrtc.org/1327153002/diff/20001/webrtc/test/layer_filteri...
webrtc/test/layer_filtering_transport.cc:103: } else if (bytes_left >= 255 +
header.headerLength) {
On 2015/09/10 12:36:58, språng wrote:
> If we make this padding packet the maximum size, the remained of the buffer
> won't fit a packet of minimum size.

Done.

[sprang_webrtc, 2015-09-11 08:02:52]

lgtm

[ivica, 2015-09-22 12:54:15]

It seems like the problem was in the sequence number all the time. When they are
properly set, replacing original packets with padding packets seems unnecessary.
I am going to close this CL after the following changes get committed:
https://codereview.webrtc.org/1350383004/

[ivica, 2015-10-08 06:49:52]

Closing this CL, because it has been replaced with a simpler solution:
https://codereview.webrtc.org/1350383004/