Simplify IsFmtpParam according to RFC 4855.

Simplify IsFmtpParam according to RFC 4855.

This should help pave the way for injectable audio codecs, since
external implementations need to be able to signal arbitrary fmtp
parameters.

BUG=webrtc:5806
Review-Url: https://codereview.webrtc.org/2661453003
Cr-Commit-Position: refs/heads/master@{#16360}
Committed: https://chromium.googlesource.com/external/webrtc/+/aa4b0775aa9877410fb45fe406e786aafef6f934

[ossu, 2017-01-27 13:00:42]

ossu@webrtc.org changed reviewers:
+ hta@webrtc.org

[ossu, 2017-01-27 13:00:42]

Hi hta,

This CL changes how we filter fmtp parameters in webrtcsdp.cc. It's closer to
how they're defined in the RFCs and should in practice match what we're already
doing.

PTAL

https://codereview.webrtc.org/2661453003/diff/1/webrtc/pc/webrtcsdp.cc
File webrtc/pc/webrtcsdp.cc (left):

https://codereview.webrtc.org/2661453003/diff/1/webrtc/pc/webrtcsdp.cc#oldcod...
webrtc/pc/webrtcsdp.cc:1626: return false;
I added a debug printout here and ran through our tests. Only ptime and maxptime
was filtered out, so our previous implementation seems to match what the RFC
says, albeit in a roundabout way.

[hta-webrtc, 2017-01-27 13:59:30]

Can you add an unittest in webrtcsdp_unittest.cc that tries to build:

1) from a parameter map containing "ptime" and "stereo"
2) from a parameter map containing "Ptime" and "Stereo"

These two should behave differently before and after the patch. So figure out
which one is correct, and go for that.

https://codereview.webrtc.org/2661453003/diff/1/webrtc/pc/webrtcsdp.cc
File webrtc/pc/webrtcsdp.cc (right):

https://codereview.webrtc.org/2661453003/diff/1/webrtc/pc/webrtcsdp.cc#newcod...
webrtc/pc/webrtcsdp.cc:1605: stricmp(name.c_str(), kCodecParamMaxPTime) != 0;
There's a functional change here. You're switching from case sensitve compare to
case insensitive compare.
Do we have a spec-valid reason for that?
(I don't guarantee that the old way was right!)

[ossu, 2017-01-27 15:13:25]

Sure, I can add a test to ensure ptime and maxptime gets put in a= regardless of
case, and also that we don't filter out some unknown parameter from fmtp.

https://codereview.webrtc.org/2661453003/diff/1/webrtc/pc/webrtcsdp.cc
File webrtc/pc/webrtcsdp.cc (right):

https://codereview.webrtc.org/2661453003/diff/1/webrtc/pc/webrtcsdp.cc#newcod...
webrtc/pc/webrtcsdp.cc:1605: stricmp(name.c_str(), kCodecParamMaxPTime) != 0;
On 2017/01/27 13:59:30, hta-webrtc wrote:
> There's a functional change here. You're switching from case sensitve compare
to
> case insensitive compare.
> Do we have a spec-valid reason for that?
> (I don't guarantee that the old way was right!)

Yes, the parameter names are case insensitive and we treat them like that in
other places. Referring back to RFC 4855, we have:

  The format for a parameter is specified in RFC 2045 as
        attribute "=" value

and in RFC 2045:

       parameter := attribute "=" value

       attribute := token
                    ; Matching of attributes
                    ; is ALWAYS case-insensitive.

[hta-webrtc, 2017-01-27 15:27:17]

On 2017/01/27 15:13:25, ossu wrote:
> Sure, I can add a test to ensure ptime and maxptime gets put in a= regardless
of
> case, and also that we don't filter out some unknown parameter from fmtp.
> 
> https://codereview.webrtc.org/2661453003/diff/1/webrtc/pc/webrtcsdp.cc
> File webrtc/pc/webrtcsdp.cc (right):
> 
>
https://codereview.webrtc.org/2661453003/diff/1/webrtc/pc/webrtcsdp.cc#newcod...
> webrtc/pc/webrtcsdp.cc:1605: stricmp(name.c_str(), kCodecParamMaxPTime) != 0;
> On 2017/01/27 13:59:30, hta-webrtc wrote:
> > There's a functional change here. You're switching from case sensitve
compare
> to
> > case insensitive compare.
> > Do we have a spec-valid reason for that?
> > (I don't guarantee that the old way was right!)
> 
> Yes, the parameter names are case insensitive and we treat them like that in
> other places. Referring back to RFC 4855, we have:
> 
>   The format for a parameter is specified in RFC 2045 as
>         attribute "=" value
> 
> and in RFC 2045:
> 
>        parameter := attribute "=" value
> 
>        attribute := token
>                     ; Matching of attributes
>                     ; is ALWAYS case-insensitive.

Sounds good - then we have gotten a bug fixed at the same time!

Make sure you put in the RFC 2045 reference in the unit test.

[ossu, 2017-01-27 16:33:31]

On 2017/01/27 15:27:17, hta-webrtc wrote:
> On 2017/01/27 15:13:25, ossu wrote:
> > Sure, I can add a test to ensure ptime and maxptime gets put in a=
regardless
> of
> > case, and also that we don't filter out some unknown parameter from fmtp.
> > 
> > https://codereview.webrtc.org/2661453003/diff/1/webrtc/pc/webrtcsdp.cc
> > File webrtc/pc/webrtcsdp.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2661453003/diff/1/webrtc/pc/webrtcsdp.cc#newcod...
> > webrtc/pc/webrtcsdp.cc:1605: stricmp(name.c_str(), kCodecParamMaxPTime) !=
0;
> > On 2017/01/27 13:59:30, hta-webrtc wrote:
> > > There's a functional change here. You're switching from case sensitve
> compare
> > to
> > > case insensitive compare.
> > > Do we have a spec-valid reason for that?
> > > (I don't guarantee that the old way was right!)
> > 
> > Yes, the parameter names are case insensitive and we treat them like that in
> > other places. Referring back to RFC 4855, we have:
> > 
> >   The format for a parameter is specified in RFC 2045 as
> >         attribute "=" value
> > 
> > and in RFC 2045:
> > 
> >        parameter := attribute "=" value
> > 
> >        attribute := token
> >                     ; Matching of attributes
> >                     ; is ALWAYS case-insensitive.
> 
> Sounds good - then we have gotten a bug fixed at the same time!
> 
> Make sure you put in the RFC 2045 reference in the unit test.

I see now that we're not consistently treating codec parameter names
case-insensitively in other parts of the code.
Coincidentally, I'm working on a separate CL to better deal with parameter and
format names case-insensitively.

So for this CL, we have a couple of options. I can either:
1. Add in case-insensitive matching on ptime and maxptime for the translation
into a= parameters in webrtcsdp.cc only, for now.
2. Keep the rest of the code, and this CL, as-is (plus tests, of course) and add
a TODO about case being handled wrong.
3. Change the comparisons in this CL back to using operator==() and revisit this
in my other CL.

Regardless, I'll add a bug about not doing case-insensitive comparisons of
parameter names, that references the RFC, and assign it to myself.

I prefer option 3, above. It'll keep the code slightly more legible while not
introducing new wrong behavior. WDYT?

[ossu, 2017-01-30 13:13:35]

Alright. I've gone with option 3 and added some tests.

[hta-webrtc, 2017-01-30 13:55:06]

lgtm

Will you file a bug for the case-sensitivity issue?

[ossu, 2017-01-30 14:12:21]

Sure will!

https://bugs.chromium.org/p/webrtc/issues/detail?id=7081

[ossu, 2017-01-30 14:12:28]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2017-01-30 14:12:35]

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

[hta-webrtc, 2017-01-30 14:17:19]

The CQ bit was checked by hta@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 14:17:25]

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

[commit-bot: I haz the power, 2017-01-30 15:16:26]

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

[commit-bot: I haz the power, 2017-01-30 15:16:27]

Dry run: This issue passed the CQ dry run.

[hta-webrtc, 2017-01-30 15:32:14]

If you were trying to submit, I may have stepped on your toes by scheduling a
dry run.... the CQ info was a bit confusing.
In general, I always do a dry run before asking for approval, the bots have
saved me from stupidity so many times - so when I saw there was no dry run done
on this patch set, I triggered one.

[ossu, 2017-01-30 15:38:53]

On 2017/01/30 15:32:14, hta-webrtc wrote:
> If you were trying to submit, I may have stepped on your toes by scheduling a
> dry run.... the CQ info was a bit confusing.
> In general, I always do a dry run before asking for approval, the bots have
> saved me from stupidity so many times - so when I saw there was no dry run
done
> on this patch set, I triggered one.

Ah, alright. I usually do a dry run as well but not this time... submitting now.

[ossu, 2017-01-30 15:38:57]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2017-01-30 15:39:03]

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

[commit-bot: I haz the power, 2017-01-30 15:41:19]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485790737495990,
"parent_rev": "55d6539b8696554d1e87725f76e613895c9d73c6", "commit_rev":
"aa4b0775aa9877410fb45fe406e786aafef6f934"}

[commit-bot: I haz the power, 2017-01-30 15:41:23]

Description was changed from

==========
Simplify IsFmtpParam according to RFC 4855.

This should help pave the way for injectable audio codecs, since
external implementations need to be able to signal arbitrary fmtp
parameters.

BUG=webrtc:5806
==========

to

==========
Simplify IsFmtpParam according to RFC 4855.

This should help pave the way for injectable audio codecs, since
external implementations need to be able to signal arbitrary fmtp
parameters.

BUG=webrtc:5806

Review-Url: https://codereview.webrtc.org/2661453003
Cr-Commit-Position: refs/heads/master@{#16360}
Committed:
https://chromium.googlesource.com/external/webrtc/+/aa4b0775aa9877410fb45fe40...
==========

[commit-bot: I haz the power, 2017-01-30 15:41:24]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/aa4b0775aa9877410fb45fe40...