Actually pass the stereo=1 parameter to WebRTC for receiving audio

Actually pass the stereo=1 parameter to WebRTC for receiving audio

There's currently a bug in WebRTC that causes us to always decode Opus
in stereo, which explains why the missing stereo parameter didn't
cause problems. But we'd like to fix that bug...

BUG=webrtc:6986, chromium:685910
Review-Url: https://codereview.chromium.org/2710483002
Cr-Commit-Position: refs/heads/master@{#451962}
Committed: https://chromium.googlesource.com/chromium/src/+/5a5c943ccde6a22137a1b9e23a4d334a26b8633b

[kwiberg-chromium, 2017-02-20 13:07:17]

The CQ bit was checked by kwiberg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 13:07:28]

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

[commit-bot: I haz the power, 2017-02-20 13:38:32]

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

[commit-bot: I haz the power, 2017-02-20 13:38:33]

Dry run: This issue passed the CQ dry run.

[Taylor_Brandstetter, 2017-02-20 20:28:31]

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

[Taylor_Brandstetter, 2017-02-20 20:28:32]

Not a committer or owner but lgtm

[kwiberg-chromium, 2017-02-21 05:39:38]

kwiberg@chromium.org changed reviewers:
+ sergeyu@chromium.org

[kwiberg-chromium, 2017-02-21 05:39:38]

Hi Sergey,

Please take a look!

[Sergey Ulanov, 2017-02-21 18:47:04]

Thank you for fixing this issue!
LGTM

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
File remoting/protocol/webrtc_transport.cc (right):

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
remoting/protocol/webrtc_transport.cc:475: {
remove these braces, I don't think they add anything.

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
remoting/protocol/webrtc_transport.cc:476: webrtc::SdpParseError err;
s/err/error/

[kwiberg-chromium, 2017-02-21 21:32:41]

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
File remoting/protocol/webrtc_transport.cc (right):

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
remoting/protocol/webrtc_transport.cc:476: webrtc::SdpParseError err;
On 2017/02/21 18:47:03, Sergey Ulanov wrote:
> s/err/error/

There's already a function argument called error---if I rename this variable
*and* remove the braces, I shadow it without even opening up a new local scope,
which is a hard error. So I can follow either one of your suggestions, but not
both.

My choice would be to keep the braces, since we've just had a demonstration why
limiting the scope of variables can be useful :-) However, my choice would also
be to have the variable named something other than "error", because shadowing
can be confusing. Do you have a favorite name besides "error"?

[Sergey Ulanov, 2017-02-21 21:49:27]

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
File remoting/protocol/webrtc_transport.cc (right):

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
remoting/protocol/webrtc_transport.cc:476: webrtc::SdpParseError err;
On 2017/02/21 21:32:41, kwiberg-chromium wrote:
> On 2017/02/21 18:47:03, Sergey Ulanov wrote:
> > s/err/error/
> 
> There's already a function argument called error---if I rename this variable
> *and* remove the braces, I shadow it without even opening up a new local
scope,
> which is a hard error. So I can follow either one of your suggestions, but not
> both.
> 
> My choice would be to keep the braces, since we've just had a demonstration
why
> limiting the scope of variables can be useful :-) However, my choice would
also
> be to have the variable named something other than "error", because shadowing
> can be confusing. Do you have a favorite name besides "error"?

Maybe call it parse_error?
If you keep the braces it would make this code compile, but it may still be
confusing that there are two different |error| variables.

[kwiberg-chromium, 2017-02-21 21:56:54]

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
File remoting/protocol/webrtc_transport.cc (right):

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
remoting/protocol/webrtc_transport.cc:476: webrtc::SdpParseError err;
On 2017/02/21 21:49:27, Sergey Ulanov wrote:
> On 2017/02/21 21:32:41, kwiberg-chromium wrote:
> > On 2017/02/21 18:47:03, Sergey Ulanov wrote:
> > > s/err/error/
> > 
> > There's already a function argument called error---if I rename this variable
> > *and* remove the braces, I shadow it without even opening up a new local
> scope,
> > which is a hard error. So I can follow either one of your suggestions, but
not
> > both.
> > 
> > My choice would be to keep the braces, since we've just had a demonstration
> why
> > limiting the scope of variables can be useful :-) However, my choice would
> also
> > be to have the variable named something other than "error", because
shadowing
> > can be confusing. Do you have a favorite name besides "error"?
> 
> Maybe call it parse_error?
> If you keep the braces it would make this code compile, but it may still be
> confusing that there are two different |error| variables.

Yes. We *need* to have the braces *or* a variable name != "error", but for good
readability we *should* have both. I originally picked "err" because small
scopes mean variable names don't have to be so long and descriptive (because the
definition + all uses are all in those handful of lines).

OK to keep the braces, and call the variable "parse_error"?

[Sergey Ulanov, 2017-02-21 22:31:52]

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
File remoting/protocol/webrtc_transport.cc (right):

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
remoting/protocol/webrtc_transport.cc:476: webrtc::SdpParseError err;
On 2017/02/21 21:56:54, kwiberg-chromium wrote:
> On 2017/02/21 21:49:27, Sergey Ulanov wrote:
> > On 2017/02/21 21:32:41, kwiberg-chromium wrote:
> > > On 2017/02/21 18:47:03, Sergey Ulanov wrote:
> > > > s/err/error/
> > > 
> > > There's already a function argument called error---if I rename this
variable
> > > *and* remove the braces, I shadow it without even opening up a new local
> > scope,
> > > which is a hard error. So I can follow either one of your suggestions, but
> not
> > > both.
> > > 
> > > My choice would be to keep the braces, since we've just had a
demonstration
> > why
> > > limiting the scope of variables can be useful :-) However, my choice would
> > also
> > > be to have the variable named something other than "error", because
> shadowing
> > > can be confusing. Do you have a favorite name besides "error"?
> > 
> > Maybe call it parse_error?
> > If you keep the braces it would make this code compile, but it may still be
> > confusing that there are two different |error| variables.
> 
> Yes. We *need* to have the braces *or* a variable name != "error", but for
good
> readability we *should* have both. I originally picked "err" because small
> scopes mean variable names don't have to be so long and descriptive (because
the
> definition + all uses are all in those handful of lines).
> 
> OK to keep the braces, and call the variable "parse_error"?

Sorry I didn't pay full attention to what you wrote previously.
I would prefer to leave the braces out, if only for consistency with other parts
of this function as well as other functions in this file. E.g. the block that
works on |digest| and |digest_base64| below is not enclosed in braces. 
But feel free to keep the braces if you feel strongly about it.

[kwiberg-chromium, 2017-02-21 23:03:32]

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
File remoting/protocol/webrtc_transport.cc (right):

https://codereview.chromium.org/2710483002/diff/1/remoting/protocol/webrtc_tr...
remoting/protocol/webrtc_transport.cc:476: webrtc::SdpParseError err;
On 2017/02/21 22:31:52, Sergey Ulanov wrote:
> On 2017/02/21 21:56:54, kwiberg-chromium wrote:
> > On 2017/02/21 21:49:27, Sergey Ulanov wrote:
> > > On 2017/02/21 21:32:41, kwiberg-chromium wrote:
> > > > On 2017/02/21 18:47:03, Sergey Ulanov wrote:
> > > > > s/err/error/
> > > > 
> > > > There's already a function argument called error---if I rename this
> variable
> > > > *and* remove the braces, I shadow it without even opening up a new local
> > > scope,
> > > > which is a hard error. So I can follow either one of your suggestions,
but
> > not
> > > > both.
> > > > 
> > > > My choice would be to keep the braces, since we've just had a
> demonstration
> > > why
> > > > limiting the scope of variables can be useful :-) However, my choice
would
> > > also
> > > > be to have the variable named something other than "error", because
> > shadowing
> > > > can be confusing. Do you have a favorite name besides "error"?
> > > 
> > > Maybe call it parse_error?
> > > If you keep the braces it would make this code compile, but it may still
be
> > > confusing that there are two different |error| variables.
> > 
> > Yes. We *need* to have the braces *or* a variable name != "error", but for
> good
> > readability we *should* have both. I originally picked "err" because small
> > scopes mean variable names don't have to be so long and descriptive (because
> the
> > definition + all uses are all in those handful of lines).
> > 
> > OK to keep the braces, and call the variable "parse_error"?
> 
> Sorry I didn't pay full attention to what you wrote previously.
> I would prefer to leave the braces out, if only for consistency with other
parts
> of this function as well as other functions in this file. E.g. the block that
> works on |digest| and |digest_base64| below is not enclosed in braces. 
> But feel free to keep the braces if you feel strongly about it.

I'll go with your preference since this is your code.

[kwiberg-chromium, 2017-02-21 23:06:37]

The CQ bit was checked by kwiberg@chromium.org

[kwiberg-chromium, 2017-02-21 23:06:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, deadbeef@chromium.org
Link to the patchset: https://codereview.chromium.org/2710483002/#ps20001
(title: "err -> parse_error, no braces")

[commit-bot: I haz the power, 2017-02-21 23:08:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 01:13:42]

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

[commit-bot: I haz the power, 2017-02-22 01:13:43]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[kwiberg-chromium, 2017-02-22 08:27:07]

The CQ bit was checked by kwiberg@chromium.org

[commit-bot: I haz the power, 2017-02-22 08:28:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 09:54:12]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487752027977930,
"parent_rev": "7a581007693d6b482158d03cd6eac849da99bb76", "commit_rev":
"5a5c943ccde6a22137a1b9e23a4d334a26b8633b"}

[commit-bot: I haz the power, 2017-02-22 09:54:51]

Description was changed from

==========
Actually pass the stereo=1 parameter to WebRTC for receiving audio

There's currently a bug in WebRTC that causes us to always decode Opus
in stereo, which explains why the missing stereo parameter didn't
cause problems. But we'd like to fix that bug...

BUG=webrtc:6986, chromium:685910
==========

to

==========
Actually pass the stereo=1 parameter to WebRTC for receiving audio

There's currently a bug in WebRTC that causes us to always decode Opus
in stereo, which explains why the missing stereo parameter didn't
cause problems. But we'd like to fix that bug...

BUG=webrtc:6986, chromium:685910

Review-Url: https://codereview.chromium.org/2710483002
Cr-Commit-Position: refs/heads/master@{#451962}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a5c943ccde6a22137a1b9e23a4d...
==========

[commit-bot: I haz the power, 2017-02-22 09:54:53]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5a5c943ccde6a22137a1b9e23a4d...