Closed Bug 1212907 Opened 4 years ago Closed 4 years ago

a=rid attribute support

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed
Blocking Flags:

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(1 file)

Covers parse, serialization, and test code for a=rid.
Assignee: nobody → docfaraday
Blocks: 1192390
Bug 1212907: (WIP) a=rid support
backlog: --- → webrtc/webaudio+
Rank: 10
Priority: -- → P1
Comment on attachment 8671654 [details]
MozReview Request: Bug 1212907: a=rid support r=mt

Bug 1212907: a=rid support
Attachment #8671654 - Attachment description: MozReview Request: Bug 1212907: (WIP) a=rid support → MozReview Request: Bug 1212907: a=rid support
Comment on attachment 8671654 [details]
MozReview Request: Bug 1212907: a=rid support r=mt

Bug 1212907: a=rid support
Attachment #8671654 - Flags: review?(martin.thomson)
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 ?
Attachment #8671654 - Flags: review?(martin.thomson) → review+
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?
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.
https://reviewboard.mozilla.org/r/21627/#review19927

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

Oh, should it be a double?
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.
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 on attachment 8671654 [details]
MozReview Request: Bug 1212907: a=rid support r=mt

Bug 1212907: a=rid support r=mt
Attachment #8671654 - Attachment description: MozReview Request: Bug 1212907: a=rid support → MozReview Request: Bug 1212907: a=rid support r=mt
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b7ee07e45847
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.