1437166 - Implement RsdparsaSdpAttributeList::GetSsrcGroup()

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P3)

Description

[Dan Minor [:dminor]]

RsdparsaSdpAttributeList::GetSsrcGroup() needs an implementation.

Comment 1

[Johannes Willbold]

Attachment: Bug 1437166: Part 1: Implemented ssrc-group attribute parsing in rust.

Implemented ssrc-group attribute parsing in rust
Extended rust test case test_parse_attribute_ssrc_group

Review commit: https://reviewboard.mozilla.org/r/252140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252140/

Comment 2

[Johannes Willbold]

Attachment: Bug 1437166: Part 2: Implemented RsdparsaSdpAttributeList::GetSsrcGroup.

Implemented RsdparsaSdpAttributeList::GetSsrcGroup
Added LoadSsrcGroup
Added C++/Rust glue code for the ssrc-group attribute
Added U32Vec in glue code.
Added C++ unit test CheckSsrcGroup (only for the rust parser

Review commit: https://reviewboard.mozilla.org/r/252142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252142/

Comment 3

[Dan Minor [:dminor]]

Comment on attachment 8986909 [details]
Bug 1437166: Part 2: Implemented RsdparsaSdpAttributeList::GetSsrcGroup.

https://reviewboard.mozilla.org/r/252142/#review258744

lgtm

Comment 4

[Byron Campen [:bwc]]

Oof. I did not notice this being filed, but we don't use/support ssrc-group at all, and probably never will since we settled on unified plan.

https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/SipccSdpAttributeList.cpp#1436

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8986908 [details]
Bug 1437166: Part 1: Implemented ssrc-group attribute parsing in rust.

https://reviewboard.mozilla.org/r/252140/#review259268

I'm really sorry, but this will never be used.

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8986909 [details]
Bug 1437166: Part 2: Implemented RsdparsaSdpAttributeList::GetSsrcGroup.

https://reviewboard.mozilla.org/r/252142/#review259270

Sorry...

Comment 7

[Dan Minor [:dminor]]

I'm sorry, I filed a bug for this because the function was not implemented, I didn't know it was not relevant and simply could have been removed.

Comment 8

[Johannes Willbold]

Ok, so should I remove the GetSsrcGroup() function to fix this?

Comment 9

[Byron Campen [:bwc]]

Yeah, if feature-parity is the goal for this bug, then removing all of the ssrc-group stuff would do the trick.

Comment 10

[Johannes Willbold]

Attachment: Bug 1437166: Removed GetSsrcGroup entirely.

Removed all occurances of GetSsrcGroup in the C++/Rust and C++/sipcc glue code
Removed C++ unit test CheckSsrcGroupInSessionLevel

Review commit: https://reviewboard.mozilla.org/r/252894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252894/

Comment 11

[Nils Ohlmeier [:drno]]

Sorry I noticed too late, but Ssrc-groups is needed for RTX support. We don't have RTX support yet. But since we might want/need to support in the future I think we want the parser to support it at least.

Comment 12

[Johannes Willbold]

So should I upload my original patch again?

Comment 13

[Nils Ohlmeier [:drno]]

Maybe we can actually implement RTX without ssrc-group:

So the retransmission stream would use the same MID as the original stream. So the MID enables proper routing to the correct receiver. But the question is that RTX will have a different SSRC then the original stream. But then we need to not restart the receiver based on new/unknown SSRC, but instead need to interpret it as a repair packet. And if understand the example I was able to find the PT should still allow us to differentiate between "SSRC switch on the normal stream" vs "repair packet for the normal stream".

Comment 14

[Nils Ohlmeier [:drno]]

Examples: 
Without ssrc-group: https://tools.ietf.org/id/draft-ietf-rtcweb-sdp-08.html#sec.successful-simulcast-rtx
With ssrc-group: https://webrtchacks.com/sdp-anatomy/

Comment 15

[Dan Minor [:dminor]]

Comment on attachment 8987652 [details]
Bug 1437166: Removed GetSsrcGroup entirely.

https://reviewboard.mozilla.org/r/252894/#review259608

::: media/webrtc/signaling/gtest/sdp_unittests.cpp
(Diff revision 1)
>      ASSERT_FALSE(mSdp->GetAttributeList().HasAttribute(
>            SdpAttribute::kSsrcAttribute));
>    }
>  }
>  
> -const std::string kSsrcGroupInSessionSDP =

I'm not certain, we may want to keep the test just to make sure ssrc group doesn't cause problems for the parsers.

Comment 16

[Byron Campen [:bwc]]

Comment on attachment 8987652 [details]
Bug 1437166: Removed GetSsrcGroup entirely.

https://reviewboard.mozilla.org/r/252894/#review259608

> I'm not certain, we may want to keep the test just to make sure ssrc group doesn't cause problems for the parsers.

We already ensure that unknown attributes don't trip us up, we can remove this.

Comment 17

[Byron Campen [:bwc]]

(In reply to Nils Ohlmeier [:drno] from comment #13)
> Maybe we can actually implement RTX without ssrc-group:
> 
> So the retransmission stream would use the same MID as the original stream.
> So the MID enables proper routing to the correct receiver. But the question
> is that RTX will have a different SSRC then the original stream. But then we
> need to not restart the receiver based on new/unknown SSRC, but instead need
> to interpret it as a repair packet. And if understand the example I was able
> to find the PT should still allow us to differentiate between "SSRC switch
> on the normal stream" vs "repair packet for the normal stream".

Do we have any idea how this will actually be specified? Does Chrome do RTX, and if so, does it use ssrc-group for it?

Comment 18

[Byron Campen [:bwc]]

Comment on attachment 8987652 [details]
Bug 1437166: Removed GetSsrcGroup entirely.

https://reviewboard.mozilla.org/r/252894/#review259678

Let's wait on this for now; we should not be modifying code to tear out ssrc-group, but we should also not be modifying code to fully implement it either.

Comment 19

[Nils Ohlmeier [:drno]]

Comment on attachment 8987652 [details]
Bug 1437166: Removed GetSsrcGroup entirely.

https://reviewboard.mozilla.org/r/252894/#review260600

Comment 20

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #17)
> (In reply to Nils Ohlmeier [:drno] from comment #13)
> > Maybe we can actually implement RTX without ssrc-group:
> > 
> > So the retransmission stream would use the same MID as the original stream.
> > So the MID enables proper routing to the correct receiver. But the question
> > is that RTX will have a different SSRC then the original stream. But then we
> > need to not restart the receiver based on new/unknown SSRC, but instead need
> > to interpret it as a repair packet. And if understand the example I was able
> > to find the PT should still allow us to differentiate between "SSRC switch
> > on the normal stream" vs "repair packet for the normal stream".
> 
> Do we have any idea how this will actually be specified? Does Chrome do RTX,
> and if so, does it use ssrc-group for it?

Yes lots of service depend heavily on supporting RTX. And with Plan B you need ssrc-group to get it working.

We discussed this briefly with abr this week. And I'm convinced that we can implement RTX in the unified plan just with MID and PT, so need for ssrc-group. And in case of simulcast you need repair RID anyway. So I think we remove the code for ssrc-group (which I think Dan only added because the Rust parser supports it).

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/881acf98f994
Removed GetSsrcGroup entirely. r=dminor,drno

Comment 22

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/881acf98f994