1212907 - a=rid attribute support

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P1)

Description

[Byron Campen [:bwc]]

Covers parse, serialization, and test code for a=rid.

Comment 1

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1212907: a=rid support r=mt

Bug 1212907: (WIP) a=rid support

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8671654 [details]
MozReview Request: Bug 1212907: a=rid support r=mt

Bug 1212907: a=rid support

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8671654 [details]
MozReview Request: Bug 1212907: a=rid support r=mt

Bug 1212907: a=rid support

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8671654 [details]
MozReview Request: Bug 1212907: a=rid support r=mt

https://reviewboard.mozilla.org/r/21627/#review19927

This looks pretty good.  One comment there on the grammar for Peter though.

::: media/webrtc/signaling/src/sdp/SdpAttribute.h:816
(Diff revision 2)
> +   param-val         = byte-string

This is a bit arsey.  Does this mean that you could have a ',' or ';' in a parameter?

::: media/webrtc/signaling/src/sdp/SdpAttribute.h:851
(Diff revision 2)
> +    uint32_t maxFps;

Why can't the max FPS be non-ordinal?

::: media/webrtc/signaling/test/sdp_unittests.cpp:3908
(Diff revision 2)
> +}

Needs a test with pt after some other parameter.  Comment that it's non-standard grammar, of course.

::: media/webrtc/signaling/test/sdp_unittests.cpp:4092
(Diff revision 2)
> +        ParseRid("0123456789az-_ recv max-width=800"));

Speaking of bizarre values, do we happily accept things like \x07 ?

Comment 5

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/21627/#review19927

> This is a bit arsey.  Does this mean that you could have a ',' or ';' in a parameter?

Yep, I think Adam fixed that last night. I'll make sure to scrape the latest BNF.

> Needs a test with pt after some other parameter.  Comment that it's non-standard grammar, of course.

That seems like it would fall under testing that prevents behavior from changing, as opposed to testing that prevents regressions. Maybe I could add a test that verifies that nothing crashes, but does no other checking.

> Speaking of bizarre values, do we happily accept things like \x07 ?

Right now we'll accept anything before the first ' '. Do you think it is worthwhile adding validation?

Comment 6

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/21627/#review19927

> Why can't the max FPS be non-ordinal?

I don't understand what you're getting at here.

Comment 7

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/21627/#review19927

> I don't understand what you're getting at here.

Oh, should it be a double?

Comment 8

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/21627/#review19927

> That seems like it would fall under testing that prevents behavior from changing, as opposed to testing that prevents regressions. Maybe I could add a test that verifies that nothing crashes, but does no other checking.

The point is that if someone fixes it, then you want to make sure they test their changes.

> Right now we'll accept anything before the first ' '. Do you think it is worthwhile adding validation?

I think that in general, I'd be happier if we had validation for these sorts of things, but it's not a high priority.  Whatever you decide to do will be OK.

Comment 9

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/21627/#review19927

> Oh, should it be a double?

Oh, non-integral. The BNF (the latest one anyway) makes all of these integral values.

> The point is that if someone fixes it, then you want to make sure they test their changes.

I guess I can see the utility of that.

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8671654 [details]
MozReview Request: Bug 1212907: a=rid support r=mt

Bug 1212907: a=rid support r=mt

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ee07e45847

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7ee07e45847

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7ee07e45847