Closed
Bug 1173601
Opened 9 years ago
Closed 9 years ago
simulcast attribute support in sdparta
Categories
(Core :: WebRTC: Signaling, defect, P1)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla43
backlog | webrtc/webaudio+ |
People
(Reporter: bwc, Assigned: bwc)
References
Details
Attachments
(1 file)
Covers parse, serialization, API, and tests for simulcast attributes in sdparta.
Assignee | ||
Updated•9 years ago
|
Rank: 10
Priority: -- → P1
Assignee | ||
Updated•9 years ago
|
backlog: --- → webRTC+
Comment 1•9 years ago
|
||
Byron -- I believe you're planning to own this yourself in Q3 (as part of driving simulcast forward).
Assignee: nobody → docfaraday
Assignee | ||
Comment 2•9 years ago
|
||
Bug 1173601: a=simulcast support r?mt
Attachment #8644562 -
Flags: review?(martin.thomson)
Comment 3•9 years ago
|
||
Comment on attachment 8644562 [details]
MozReview Request: Bug 1173601: a=simulcast support r=mt
https://reviewboard.mozilla.org/r/15257/#review13695
I think that the stability of the spec is a problem here. What is your plan for dealing with that?
::: media/webrtc/signaling/src/sdp/SdpAttribute.h:1330
(Diff revision 1)
> + class Versions : public std::vector<Version>
Has-a, not is-a please. I know that it's a little more clumsy, but it's hard to keep your invariants contained if you expose all those methods.
::: media/webrtc/signaling/src/sdp/SdpAttribute.cpp:980
(Diff revision 1)
> + if (!sendVersions.IsSet() &&
> + !recvVersions.IsSet() &&
> + !sendrecvVersions.IsSet()) {
> + return;
> + }
Is this even a valid state to be in? Should we permit the addition of attributes that don't result in a line (or lines) being added to the SDP?
::: media/webrtc/signaling/src/sdp/SipccSdpAttributeList.cpp:516
(Diff revision 1)
> - std::ostringstream fullError;
> - fullError << error << " at column " << errorPos;
> + std::ostringstream fullError(error + " at column ");
> + fullError << errorPos;
Strange choice of change, why?
::: media/webrtc/signaling/src/sdp/SdpAttribute.h:1299
(Diff revision 1)
> +// Note: This ABNF is buggy since it does not include a ':' after "a=simulcast"
> +// The authors have said this will be fixed in a subsequent version.
> +// TODO(bug 1191986): Update this once the new version is out.
This bothers me a little. I think that I would rather implement the grammar that we expect to see standardized rather than commit something that we know is going to change. Otherwise, we have to put in code that handles an expired version that we happened to have shipped.
Attachment #8644562 -
Flags: review?(martin.thomson)
Assignee | ||
Comment 4•9 years ago
|
||
https://reviewboard.mozilla.org/r/15257/#review13695
Fortunately, the parse code (and representation) is all in one place in c++, so handling movement in the spec will be much easier than it would be with two representations (one in sipcc and one in SDParta), parse code in sipcc, and serialization code in SDParta. If the spec moves in some non-backward-compatible way, we'll still have trouble, and need to adopt the usual "emit the old version, accept both old and new" tactic. This only becomes relevant once we start implementing the simulcast negotiation, of course, which I'm still thinking through.
> Strange choice of change, why?
Just consistency with how I was using istringstream.
> Is this even a valid state to be in? Should we permit the addition of attributes that don't result in a line (or lines) being added to the SDP?
I could go either way on this. We already have other attributes that will serialize nothing if they aren't valid. Maybe could use an assertion.
> Has-a, not is-a please. I know that it's a little more clumsy, but it's hard to keep your invariants contained if you expose all those methods.
This actually has exactly the invariants I wanted; it was a choice between this or having |Versions| be a simple typedef std::vector<Version> along with a static Parse(Versions&,...); static Serialize(const Versions&,...); and static IsSet(const Versions&); Having Versions essentially be a typedef with a few functions grafted onto it felt cleaner to me.
> This bothers me a little. I think that I would rather implement the grammar that we expect to see standardized rather than commit something that we know is going to change. Otherwise, we have to put in code that handles an expired version that we happened to have shipped.
The comment (containing the bad ABNF) needs to be updated, not the impl. We emit the ':', and sipcc parses it. sipcc might also accept the version without the ':'. I can make this clearer.
Assignee | ||
Comment 5•9 years ago
|
||
https://reviewboard.mozilla.org/r/15257/#review13695
> This actually has exactly the invariants I wanted; it was a choice between this or having |Versions| be a simple typedef std::vector<Version> along with a static Parse(Versions&,...); static Serialize(const Versions&,...); and static IsSet(const Versions&); Having Versions essentially be a typedef with a few functions grafted onto it felt cleaner to me.
I should also point out that the typedef + static functions approach can't be used with |ParseInvalid| in sdp_unittests.cpp, so it ends up being a bit more test code too.
Assignee | ||
Updated•9 years ago
|
Attachment #8644562 -
Flags: review?(martin.thomson)
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8644562 [details]
MozReview Request: Bug 1173601: a=simulcast support r=mt
Bug 1173601: a=simulcast support r?mt
Comment 8•9 years ago
|
||
https://reviewboard.mozilla.org/r/15257/#review13695
> I should also point out that the typedef + static functions approach can't be used with |ParseInvalid| in sdp_unittests.cpp, so it ends up being a bit more test code too.
You convinced me. It makes me a little concerned about encapsulation, is all, but we're already breaking many of those rules with SdpAttribute children.
Comment 9•9 years ago
|
||
Comment on attachment 8644562 [details]
MozReview Request: Bug 1173601: a=simulcast support r=mt
https://reviewboard.mozilla.org/r/15257/#review13763
Ship It!
Attachment #8644562 -
Flags: review?(martin.thomson) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Check back on try
Assignee | ||
Updated•9 years ago
|
Attachment #8644562 -
Attachment description: MozReview Request: Bug 1173601: a=simulcast support r?mt → MozReview Request: Bug 1173601: a=simulcast support r=mt
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8644562 [details]
MozReview Request: Bug 1173601: a=simulcast support r=mt
Bug 1173601: a=simulcast support r=mt
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
Comment 12•9 years ago
|
||
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•