1173601 - simulcast attribute support in sdparta

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

Covers parse, serialization, API, and tests for simulcast attributes in sdparta.

Comment 1

[Maire Reavy [:mreavy]]

Byron -- I believe you're planning to own this yourself in Q3 (as part of driving simulcast forward).

Comment 2

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1173601: a=simulcast support r=mt

Bug 1173601: a=simulcast support r?mt

Comment 3

[Martin Thomson [:mt:]]

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.

Comment 4

[Byron Campen [:bwc]]

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.

Comment 5

[Byron Campen [:bwc]]

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.

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8644562 [details]
MozReview Request: Bug 1173601: a=simulcast support r=mt

Bug 1173601: a=simulcast support r?mt

Comment 7

[Byron Campen [:bwc]]

Trigger try build once try reopens.

Comment 8

[Martin Thomson [:mt:]]

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

[Martin Thomson [:mt:]]

Comment on attachment 8644562 [details]
MozReview Request: Bug 1173601: a=simulcast support r=mt

https://reviewboard.mozilla.org/r/15257/#review13763

Ship It!

Comment 10

[Byron Campen [:bwc]]

Check back on try

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8644562 [details]
MozReview Request: Bug 1173601: a=simulcast support r=mt

Bug 1173601: a=simulcast support r=mt

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b615b3462e4

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3b615b3462e4