Cast: Increase UDP socket send buffer size

Cast: set UDP socket send buffer size via options

On Windows the default UDP send buffer size is 8192. OSX default is
9216. On Linux default is 212992. The values are too small on Windows
and OSX. This cannot make good use of the A-MPDU of 802.11n (64KB),
which is the most common network for Cast Streaming.

We have also seen strong evidence that the default buffer size is
limiting, e.g. long packet queuing delay on Windows, p2p socket seeing
a lot of EWOULDBLOCKs.

This code uses pacer maximum burst size (in bytes) as the send buffer
size. Also added an option "send_buffer_min_size" to allow application
to choose an even larger value for send buffer size.

BUG=423545
Committed: https://crrev.com/482d3eb1b6b8da044304ca4bc89d9c0b0764e218
Cr-Commit-Position: refs/heads/master@{#300183}

[Alpha Left Google, 2014-10-15 00:29:41]

hclam@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2014-10-15 21:20:21]

Overall, I'm in total agreement with what you're doing here.  However, a few
concerns:

https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:39: const int
k802dot11nMaxAMPDUSize = 65536;
This will lower the default setting on Linux.  Are you sure that's a good idea? 
What if all platforms were "maxed out" to 212992 instead?  It's possible
kernels/drivers would prefer a size larger than one data unit to help schedule
data transmission.  I'm just worried we're trying to outsmart the operating
system here.

https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:125: int send_buffer_size =
std::max<int32>(
int, not int32 here.

https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:129: if
(transport_->SetSendBufferSize(send_buffer_size) != net::OK) {
Please make this a ctor argument to UdpTransport (line 85ish above), rather than
a separate method.  This issue/concept is specific to UdpTransport, and not any
other PacedSender implementation.

https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:140: wifi_options_autoreset_ =
net::SetWifiOptions(wifi_options);
Do we restore the old wifi options when Cast streaming is shut down?  Why/why
not?  Seems like this could break or inhibit other Chromium features (especially
on ChromeOS).

[hubbe, 2014-10-15 21:56:03]

hubbe@chromium.org changed reviewers:
+ hubbe@chromium.org

[hubbe, 2014-10-15 21:56:04]

Smaller buffers could be a more effective way to implement pacing though, larger
buffers isn't an automatic win.

Have you done any measurements that shows that larger buffers actually help in
some cases?

[Alpha Left Google, 2014-10-15 22:28:27]

On 2014/10/15 21:20:21, miu wrote:
> Overall, I'm in total agreement with what you're doing here.  However, a few
> concerns:
> 
>
https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
> File media/cast/net/cast_transport_sender_impl.cc (right):
> 
>
https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
> media/cast/net/cast_transport_sender_impl.cc:39: const int
> k802dot11nMaxAMPDUSize = 65536;
> This will lower the default setting on Linux.  Are you sure that's a good
idea? 
> What if all platforms were "maxed out" to 212992 instead?  It's possible
> kernels/drivers would prefer a size larger than one data unit to help schedule
> data transmission.  I'm just worried we're trying to outsmart the operating
> system here.

I double checked that on Windows it is 8192, OSX 9216 and Linux 212992. OS like
Linux just won't go lower than 131072 even if we set it.

I think this value is something the application should control instead of
letting OS to choose. The ideal case is that we control the buffer
size dynamically based on current bitrate to ensure we don't buffer to many
frames in OS socket buffer. This allows us to flush our pacer buffer.

> 
>
https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
> media/cast/net/cast_transport_sender_impl.cc:125: int send_buffer_size =
> std::max<int32>(
> int, not int32 here.
> 
>
https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
> media/cast/net/cast_transport_sender_impl.cc:129: if
> (transport_->SetSendBufferSize(send_buffer_size) != net::OK) {
> Please make this a ctor argument to UdpTransport (line 85ish above), rather
than
> a separate method.  This issue/concept is specific to UdpTransport, and not
any
> other PacedSender implementation.
> 
>
https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
> media/cast/net/cast_transport_sender_impl.cc:140: wifi_options_autoreset_ =
> net::SetWifiOptions(wifi_options);
> Do we restore the old wifi options when Cast streaming is shut down?  Why/why
> not?  Seems like this could break or inhibit other Chromium features
(especially
> on ChromeOS).

I think we do reset already. It's a ScopedWifiOption that will be reset during
destruction.

[Alpha Left Google, 2014-10-15 22:37:11]

On 2014/10/15 21:56:04, hubbe wrote:
> Smaller buffers could be a more effective way to implement pacing though,
larger
> buffers isn't an automatic win.
> 
> Have you done any measurements that shows that larger buffers actually help in
> some cases?

I think the buffer size needs to be at least the size of the maximum burst size,
i.e. 30KB (20 x 1.5KB). This code sets the minimum of 64KB which is not far off.
Also on Linux this value is 131072 minimum, only on Windows and OSX we get very
small values (8192 and 9216). 

I tried this locally and didn't see a regression. But then the issue of
EWOULDBLOCK happens mostly on Windows which I have no machine to test. We'll
have to try this on users. What about I add a "send_buffer_min_size" to control
this in the extension? Again I think the max burst size should control the
minimum send buffer size anyway.

[Alpha Left Google, 2014-10-15 23:07:26]

Fixed comments by miu@.

Also changed to code to allow application to override send buffer size with
"send_buffer_min_size". The minimum value based on maximum pacer burst size is
still in place.

https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:125: int send_buffer_size =
std::max<int32>(
On 2014/10/15 21:20:21, miu wrote:
> int, not int32 here.

Done.

https://codereview.chromium.org/654843007/diff/40001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:129: if
(transport_->SetSendBufferSize(send_buffer_size) != net::OK) {
On 2014/10/15 21:20:21, miu wrote:
> Please make this a ctor argument to UdpTransport (line 85ish above), rather
than
> a separate method.  This issue/concept is specific to UdpTransport, and not
any
> other PacedSender implementation.

Done.

[miu, 2014-10-16 18:31:57]

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/cast_tran...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:39: int32
GetTransportSendBufferSize(const base::DictionaryValue& options) {
s/int32/int/ and throughout.  There's no good reason to require a specific
number of bits.

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:97:
GetTransportSendBufferSize(*options.get()),
Don't need the .get().  Just *options will work.

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/udp_trans...
File media/cast/net/udp_transport.cc (right):

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/udp_trans...
media/cast/net/udp_transport.cc:45: int32 send_buffer_size,
s/int32/int/

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/udp_trans...
media/cast/net/udp_transport.cc:91: VLOG(1) << "Failed to set socket send buffer
size.";
Perhaps this should be LOG(WARNING) or, if we believe we are setting a value
within the allowed range for the operating system: LOG(ERROR).  Seems like with
VLOG(1) it would be too easy to accidentally conceal this error.

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/udp_trans...
File media/cast/net/udp_transport.h (right):

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/udp_trans...
media/cast/net/udp_transport.h:44: int32 send_buffer_size,
s/int32/int/

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/udp_trans...
media/cast/net/udp_transport.h:87: int32 send_buffer_size_;
s/int32/const int/

[Alpha Left Google, 2014-10-16 18:41:27]

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/cast_tran...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:39: int32
GetTransportSendBufferSize(const base::DictionaryValue& options) {
On 2014/10/16 18:31:57, miu wrote:
> s/int32/int/ and throughout.  There's no good reason to require a specific
> number of bits.

Reason I chose int32 is because udp socket's SetSendBufferSize(int32 size)
already takes a int32. I wanted to make the API consistent throughout.

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/cast_tran...
media/cast/net/cast_transport_sender_impl.cc:97:
GetTransportSendBufferSize(*options.get()),
On 2014/10/16 18:31:57, miu wrote:
> Don't need the .get().  Just *options will work.

Done.

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/udp_trans...
File media/cast/net/udp_transport.cc (right):

https://codereview.chromium.org/654843007/diff/80001/media/cast/net/udp_trans...
media/cast/net/udp_transport.cc:91: VLOG(1) << "Failed to set socket send buffer
size.";
On 2014/10/16 18:31:57, miu wrote:
> Perhaps this should be LOG(WARNING) or, if we believe we are setting a value
> within the allowed range for the operating system: LOG(ERROR).  Seems like
with
> VLOG(1) it would be too easy to accidentally conceal this error.

Done.

[Alpha Left Google, 2014-10-17 19:26:37]

Ping again about int32 vs int. The reason I chose int32 is because the
underlying API of UDP socket takes int32 for buffer size, same for
media::cast::UdpTransport to avoid extra conversion.

[miu, 2014-10-17 20:20:43]

lgtm % one thing you will want to change:

https://codereview.chromium.org/654843007/diff/100001/media/cast/net/udp_tran...
File media/cast/net/udp_transport.cc (right):

https://codereview.chromium.org/654843007/diff/100001/media/cast/net/udp_tran...
media/cast/net/udp_transport.cc:237: LOG(WARNING) << "Failed to send packet: "
<< result << ".";
Not here!  ;-)

You meant to change line 91 instead, right?

[Alpha Left Google, 2014-10-17 20:24:57]

https://codereview.chromium.org/654843007/diff/100001/media/cast/net/udp_tran...
File media/cast/net/udp_transport.cc (right):

https://codereview.chromium.org/654843007/diff/100001/media/cast/net/udp_tran...
media/cast/net/udp_transport.cc:237: LOG(WARNING) << "Failed to send packet: "
<< result << ".";
On 2014/10/17 20:20:42, miu wrote:
> Not here!  ;-)
> 
> You meant to change line 91 instead, right?

Doh I must be too sleepy.

[Alpha Left Google, 2014-10-17 20:25:01]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-10-17 20:26:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/654843007/120001

[commit-bot: I haz the power, 2014-10-17 22:51:21]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-17 22:51:52]

Patchset 7 (id:??) landed as
https://crrev.com/482d3eb1b6b8da044304ca4bc89d9c0b0764e218
Cr-Commit-Position: refs/heads/master@{#300183}