Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split getContributingSources into two methods, for CSRCs and SSRCs. #1133

Merged
merged 1 commit into from May 4, 2017

Conversation

taylor-b
Copy link
Contributor

@taylor-b taylor-b commented Apr 20, 2017

Fixes #1101.

There's now getContributingSources and getSynchronizationSources,
which return RTCRtpContributingSources and
RTCRtpSynchronizationSources, respectively. These interfaces are
slightly different, in that only RTCRtpSynchronizationSource has
voiceActivityFlag, and non-nullable audioLevel.

There's now getContributingSources and getSynchronizationSources,
which return RTCRtpContributingSources and
RTCRtpSynchronizationSources, respectively. These interfaces are
slightly different, in that only RTCRtpSynchronizationSource has
voiceActivityFlag, and non-nullable audioLevel.
@taylor-b
Copy link
Contributor Author

I liked Harald's suggestion to just call it getSsrcs, but that seemed inconsistent with getContributingSources (which could just as easily be getCsrcs).

Another naming thing I don't like is that the CSRC or SSRC identifier is called source, as opposed to just ssrc/csrc, or even identifier. Is it too late to change this?

Naming gripes aside, I don't think there are any real issues with this.

<p>The <dfn>RTCRtpContributingSource</dfn> and
<dfn>RTCRtpSynchronizationSource</dfn> objects contain information about
a given contributing source (CSRC) or synchronization source (SSRC),
including the most recent time a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: weird line-break

Copy link
Contributor Author

@taylor-b taylor-b Apr 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this to make the diff cleaner, will rely on auto-formatting tool at some point in the future.

<code><a>RTCRtpSynchronizationSource</a></code> object corresponding to the
SSRC identifier is always updated, and if the RTP packet contains CSRC
identifiers, then the <code><a>RTCRtpContributingSource</a></code>
objects corresponding to those CSRC identifiers are also updated.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SSRC and CSRC numbers are independent of each other? You could have one RTP packet (ssrc:1, csrc:[1,...]) and then another one (ssrc:2, csrc:[1,...])? Does it make sense to have a reference to the most recent RTCRtpSynchronizationSource from an RTCRtpContributingSource?

Hmm actually that could be misleading because the SSRC could be updated and so a CSRC's SSRC could represent a different state and time than the CSRC. But what about a source id? Or is this not useful?

It would seem the SSRC's audioLevel would be enough (CSRC's audioLevel being a copy) if it wasn't for the reference representing a different state and time. Copy needed to have audioLevel for that state/time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could have one RTP packet (ssrc:1, csrc:[1,...]) and then another one (ssrc:2, csrc:[1,...])?

I don't think this is a scenario we need to worry about. Technically the IDs might share the same space ("CSRC identifier" is just shorthand for "SSRC identifier of the CSRC") but I can't imagine how a CSRC could also be a primary SSRC in the same session.

The intention is basically that RTCRtpContributingSource represents "information from CSRC list and RFC 6465 extension" and RTCRtpSynchronizationSource represents "information from SSRC field and RFC 6464 extension".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I wasn't thinking about CSRC and SSRC namespaces colliding but rather if different SSRCs transport the same CSRCs at different points in time. I'll update the example to:

"You could have one RTP packet (ssrc:1, csrc:[10,...]) and then another one (ssrc:2, csrc:[10,...])?"

Would the CSRC information be copies of the latest SSRC packet information that contained that CSRC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your question now, sorry for my mixup.

Anyway: I was thinking of each RTCRtpContributingSource object as being tied to a particular RTCRtpReceiver. That seems more sensible given how the API is expected to be used.

So, if the same source is contributing to two different streams, I'd suggest having two different RTCRtpContributingSource objects, one for each stream it's contributing to.

If the application really does need to know "which stream did this source most recently contribute to?," it still can do this by comparing timestamps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that sounds good to me.

defined in [[!RFC6464]], if the RFC 6464 header extension is
present. If the RFC 6464 extension header is not present, the
browser will compute a value for <code>audioLevel</code> as if it
had come from RFC 6464.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a significant cost to making computing audioLevel non-optional per RTP packet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully not, since it's already computed for stats. Though I'm not changing that in this PR, just moving it around.

@henbos
Copy link
Contributor

henbos commented Apr 20, 2017

Chrome having shipped (M59, in Canary) with names getContributingSources and RTCRtpContributingSource.source I don't think it is worth changing the name.

@stefhak
Copy link
Contributor

stefhak commented May 4, 2017

Decided to merge at the May 2nd VI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants