Cast: Use DSCP AF41 for all traffic if possible.

Cast: Use DSCP AF41 for all traffic if possible.

Turns on DSCP AF41 to give cast traffic a higher priority.

BUG=385768
TBR=miu

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278683

[Alpha Left Google, 2014-06-19 21:37:15]

+mfoltz for review

[mark a. foltz, 2014-06-19 21:52:28]

Minor comments.

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
File media/cast/transport/transport/udp_transport.cc (right):

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
media/cast/transport/transport/udp_transport.cc:53: net::NetLog::Source())),
Will the DSCP value potentially change over the lifetime of the socket?  If not
perhaps set it in the ctor or after binding the socket to an address.

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
media/cast/transport/transport/udp_transport.cc:172: if (result != net::OK) {
This appears to be unimplemented on Windows and will always return an error.

https://code.google.com/p/chromium/codesearch#chromium/src/net/udp/udp_socket...

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
media/cast/transport/transport/udp_transport.cc:176: // Don't change DSCP in
next send.
Would there be any benefit to retrying SetDiffServCodePoint again if it fails
the first time?

[Alpha Left Google, 2014-06-19 21:59:46]

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
File media/cast/transport/transport/udp_transport.cc (right):

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
media/cast/transport/transport/udp_transport.cc:53: net::NetLog::Source())),
On 2014/06/19 21:52:28, mark a. foltz wrote:
> Will the DSCP value potentially change over the lifetime of the socket?  If
not
> perhaps set it in the ctor or after binding the socket to an address.

It could change over the lifetime of a socket. For example the use case in
webrtc is that it can change for every packet.

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
media/cast/transport/transport/udp_transport.cc:172: if (result != net::OK) {
On 2014/06/19 21:52:28, mark a. foltz wrote:
> This appears to be unimplemented on Windows and will always return an error.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/udp/udp_socket...

That's right. Hubbe said it is difficult to implement on Windows and we don't
expect to have it on windows.

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
media/cast/transport/transport/udp_transport.cc:176: // Don't change DSCP in
next send.
On 2014/06/19 21:52:28, mark a. foltz wrote:
> Would there be any benefit to retrying SetDiffServCodePoint again if it fails
> the first time?

Based on the existing implementation in p2p socket (used by webrtc) after the
first failure there will be no retry. It is unlikely to work if the first try
fails.

[miu, 2014-06-19 22:41:42]

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
File media/cast/transport/transport/udp_transport.cc (right):

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
media/cast/transport/transport/udp_transport.cc:93: next_dscp_value_ = dscp;
Please add a DCHECK(IO thread), or do you need a mutex to make this thread-safe?

[Alpha Left Google, 2014-06-19 22:43:21]

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
File media/cast/transport/transport/udp_transport.cc (right):

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
media/cast/transport/transport/udp_transport.cc:93: next_dscp_value_ = dscp;
On 2014/06/19 22:41:42, miu wrote:
> Please add a DCHECK(IO thread), or do you need a mutex to make this
thread-safe?

Done.

[mark a. foltz, 2014-06-19 22:57:32]

LGTM

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
File media/cast/transport/transport/udp_transport.cc (right):

https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
media/cast/transport/transport/udp_transport.cc:172: if (result != net::OK) {
On 2014/06/19 21:59:46, Alpha wrote:
> On 2014/06/19 21:52:28, mark a. foltz wrote:
> > This appears to be unimplemented on Windows and will always return an error.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/udp/udp_socket...
> 
> That's right. Hubbe said it is difficult to implement on Windows and we don't
> expect to have it on windows.

Maybe don't LOG(ERROR) on Windows, since the error won't mean anything on
Windows.

[Alpha Left Google, 2014-06-19 23:02:31]

On 2014/06/19 22:57:32, mark a. foltz wrote:
> LGTM
> 
>
https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
> File media/cast/transport/transport/udp_transport.cc (right):
> 
>
https://codereview.chromium.org/344953003/diff/1/media/cast/transport/transpo...
> media/cast/transport/transport/udp_transport.cc:172: if (result != net::OK) {
> On 2014/06/19 21:59:46, Alpha wrote:
> > On 2014/06/19 21:52:28, mark a. foltz wrote:
> > > This appears to be unimplemented on Windows and will always return an
error.
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/udp/udp_socket...
> > 
> > That's right. Hubbe said it is difficult to implement on Windows and we
don't
> > expect to have it on windows.
> 
> Maybe don't LOG(ERROR) on Windows, since the error won't mean anything on
> Windows.

On windows it's less spamy because a tool is needed to see the log. I prefer to
log it here regardlessly instead of doing a ifdef. We should really pull some
expertise to enable this anyway.

[Alpha Left Google, 2014-06-19 23:02:35]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-06-19 23:03:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/344953003/20001

[commit-bot: I haz the power, 2014-06-19 23:03:59]

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

[commit-bot: I haz the power, 2014-06-19 23:04:00]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Alpha Left Google, 2014-06-20 00:18:18]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-06-20 00:21:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/344953003/20001

[miu, 2014-06-20 03:09:13]

lgtm

[commit-bot: I haz the power, 2014-06-20 12:53:46]

Change committed as 278683