Add SdpHelper for more reliable codec configuration in WebrtcTransport.

Add SdpMessage for more reliable codec configuration in WebrtcTransport.

Previously codec configuration was done in WebrtcTransport by string
substitutions. It worked properly only as long as rtp payload type
values do not change. Added SdpHelper that allows to implement this
logic more reliably, without dependency on payload type values.

BUG=669121
Committed: https://crrev.com/1ac2609ef72f7cdc66b76e9ad1ba351e25393623
Cr-Commit-Position: refs/heads/master@{#434809}

[Sergey Ulanov, 2016-11-28 19:21:58]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-11-28 19:22:05]

Patchset #1 (id:20001) has been deleted

[Sergey Ulanov, 2016-11-28 19:25:55]

Patchset #1 (id:40001) has been deleted

[Sergey Ulanov, 2016-11-28 19:35:30]

Patchset #1 (id:60001) has been deleted

[Sergey Ulanov, 2016-11-28 19:36:21]

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

[Jamie, 2016-11-28 21:02:52]

LGTM with nits.

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
File remoting/protocol/sdp_helper.cc (right):

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.cc:16: for (auto& line : sdp_lines_) {
const auto?

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.cc:48: auto& line = sdp_lines_[i];
const?

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.cc:54: if (line.substr(space_pos + 1, codec.size())
== codec &&
Can we assume there will be only one space between the prefix and the codec
string?

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
File remoting/protocol/sdp_helper.h (right):

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:19: // SdpHelper is used to process SDP session
descriptions generated by WebRTC,
Maybe define SDP the first time you use it.

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:23: // signatures verifications is the same that
was signed on the sending side.
s/signatures verifications/signature verification/

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:26: class SdpHelper {
*Helper is a bit generic as a name. If this encapsulates an SDP message, maybe
SdpMessage would be better?

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:34: // Returns string representation of the SDP.
The result always has
s/SDP/SDP message/

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:35: // line-endings normalized and empty lines
removed.
Be specific about what the normalized line-endings are. Looking at the code, it
seems that we're normalizing on \n. If this class is always used, it shouldn't
make any difference to our code, so I wonder if it would make more sense to
normalize on on CRLF for consistency?

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:39: // not listed anywhere in SDP.
s/SDP/the SDP message/

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:44: // Find the line in form of
"a=rtpmap:<payload_type> <codec>/.." with the
s/in form of/of the form/

[Sergey Ulanov, 2016-11-28 22:45:09]

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
File remoting/protocol/sdp_helper.cc (right):

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.cc:16: for (auto& line : sdp_lines_) {
On 2016/11/28 21:02:51, Jamie wrote:
> const auto?

Done.

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.cc:48: auto& line = sdp_lines_[i];
On 2016/11/28 21:02:51, Jamie wrote:
> const?

Done.

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.cc:54: if (line.substr(space_pos + 1, codec.size())
== codec &&
On 2016/11/28 21:02:51, Jamie wrote:
> Can we assume there will be only one space between the prefix and the codec
> string?

Yes, I think we can assume that. The standard defines the format of the string
as follows (see https://tools.ietf.org/html/rfc4566):
  a=rtpmap:<payload type> <encoding name>/<clock rate> [/<encoding
         parameters>]

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
File remoting/protocol/sdp_helper.h (right):

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:19: // SdpHelper is used to process SDP session
descriptions generated by WebRTC,
On 2016/11/28 21:02:52, Jamie wrote:
> Maybe define SDP the first time you use it.

Done.

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:23: // signatures verifications is the same that
was signed on the sending side.
On 2016/11/28 21:02:52, Jamie wrote:
> s/signatures verifications/signature verification/

Done.

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:26: class SdpHelper {
On 2016/11/28 21:02:52, Jamie wrote:
> *Helper is a bit generic as a name. If this encapsulates an SDP message, maybe
> SdpMessage would be better?

Done.

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:34: // Returns string representation of the SDP.
The result always has
On 2016/11/28 21:02:52, Jamie wrote:
> s/SDP/SDP message/

Done.

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:35: // line-endings normalized and empty lines
removed.
On 2016/11/28 21:02:52, Jamie wrote:
> Be specific about what the normalized line-endings are. Looking at the code,
it
> seems that we're normalizing on \n. If this class is always used, it shouldn't
> make any difference to our code, so I wonder if it would make more sense to
> normalize on on CRLF for consistency?

The client normalizes with LF, so changing it to CRLF would break compatibility
with the client.

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:39: // not listed anywhere in SDP.
On 2016/11/28 21:02:52, Jamie wrote:
> s/SDP/the SDP message/

Done.

https://codereview.chromium.org/2536623003/diff/80001/remoting/protocol/sdp_h...
remoting/protocol/sdp_helper.h:44: // Find the line in form of
"a=rtpmap:<payload_type> <codec>/.." with the
On 2016/11/28 21:02:52, Jamie wrote:
> s/in form of/of the form/

Done.

[Sergey Ulanov, 2016-11-28 22:46:33]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-11-28 22:46:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2536623003/#ps100001
(title: "address feedback")

[commit-bot: I haz the power, 2016-11-28 22:48:01]

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

[Sergey Ulanov, 2016-11-28 22:49:10]

Description was changed from

==========
Add SdpHelper for more reliable codec configuration in WebrtcTransport.

Previously codec configuration was done in WebrtcTransport by string
substitutions. It worked properly only as long as rtp payload type
values do not change. Added SdpHelper that allows to implement this
logic more reliably, without dependency on payload type values.

BUG=669121
==========

to

==========
Add SdpMessage for more reliable codec configuration in WebrtcTransport.

Previously codec configuration was done in WebrtcTransport by string
substitutions. It worked properly only as long as rtp payload type
values do not change. Added SdpHelper that allows to implement this
logic more reliably, without dependency on payload type values.

BUG=669121
==========

[commit-bot: I haz the power, 2016-11-29 01:08:20]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480373193651120,
"parent_rev": "b348a27a0f16354cbe790ce7d2d7281251dc3285", "commit_rev":
"8e7079e159eed597cc9f7bd7822074bd97048ab1"}

[commit-bot: I haz the power, 2016-11-29 01:08:45]

Description was changed from

==========
Add SdpMessage for more reliable codec configuration in WebrtcTransport.

Previously codec configuration was done in WebrtcTransport by string
substitutions. It worked properly only as long as rtp payload type
values do not change. Added SdpHelper that allows to implement this
logic more reliably, without dependency on payload type values.

BUG=669121
==========

to

==========
Add SdpMessage for more reliable codec configuration in WebrtcTransport.

Previously codec configuration was done in WebrtcTransport by string
substitutions. It worked properly only as long as rtp payload type
values do not change. Added SdpHelper that allows to implement this
logic more reliably, without dependency on payload type values.

BUG=669121
==========

[commit-bot: I haz the power, 2016-11-29 01:08:46]

Committed patchset #2 (id:100001)

[commit-bot: I haz the power, 2016-11-29 01:19:57]

Description was changed from

==========
Add SdpMessage for more reliable codec configuration in WebrtcTransport.

Previously codec configuration was done in WebrtcTransport by string
substitutions. It worked properly only as long as rtp payload type
values do not change. Added SdpHelper that allows to implement this
logic more reliably, without dependency on payload type values.

BUG=669121
==========

to

==========
Add SdpMessage for more reliable codec configuration in WebrtcTransport.

Previously codec configuration was done in WebrtcTransport by string
substitutions. It worked properly only as long as rtp payload type
values do not change. Added SdpHelper that allows to implement this
logic more reliably, without dependency on payload type values.

BUG=669121

Committed: https://crrev.com/1ac2609ef72f7cdc66b76e9ad1ba351e25393623
Cr-Commit-Position: refs/heads/master@{#434809}
==========

[commit-bot: I haz the power, 2016-11-29 01:19:58]

Patchset 2 (id:??) landed as
https://crrev.com/1ac2609ef72f7cdc66b76e9ad1ba351e25393623
Cr-Commit-Position: refs/heads/master@{#434809}