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

Remove signaling dependency #222

Merged
merged 1 commit into from
Jun 21, 2024
Merged

Conversation

mengelbart
Copy link
Owner

@joerg-ott @SpencerDawkins could you please take a look at this? I tried to capture everything that was discussed in the meeting on Tuesday, but I'm not sure if this really solves all the problem(s) or if it introduces new issues...

Copy link
Contributor

@samhurst samhurst left a comment

Choose a reason for hiding this comment

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

Thanks for making this CR. Overall this looks fine from what we discussed on Tuesday, but here's a couple of small editorial suggests that I would make.

Comment on lines 565 to 566
of buffered DATAGRAMs exceeds the limit on buffered DATAGRAMs, the endpoint MUST
drop a DATAGRAMs. It is an implementation's choice which DATAGRAMs to drop.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
of buffered DATAGRAMs exceeds the limit on buffered DATAGRAMs, the endpoint MUST
drop a DATAGRAMs. It is an implementation's choice which DATAGRAMs to drop.
of buffered DATAGRAMs exceeds the limit on buffered DATAGRAMs, the endpoint MUST
drop a DATAGRAM frame. It is an implementation's choice which DATAGRAMs to drop.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Uppercase DATAGRAM is defined in the terminology section as referring to a QUIC DATAGRAM frame because we tried to avoid confusion between QUIC frames and media frames.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, and it's actually worse than what @mengelbart said. From the current draft:

Datagram:
The term "datagram" is ambiguous. Without a qualifier, "datagram" could refer to a UDP packet, or a QUIC DATAGRAM frame, as defined in QUIC's unreliable DATAGRAM extension [RFC9221], or an RTP packet encapsulated in UDP, or an RTP packet capsulated in QUIC DATAGRAM frame. This document uses the uppercase "DATAGRAM" to refer to a QUIC DATAGRAM frame and the term RoQ datagram as a short form of "RTP packet encapsulated in a QUIC DATAGRAM frame".

We might consider adding an issue for "things that have surprised readers in the past".

  • There are some terms that we use, that really do require the reader to look them up in the terminology section. This is one of them. And I note that unqualified "frame" doesn't appear in the terminology section - maybe it should.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I note that @samhurst's suggestion also fixed "a DATAGRAMs" - we should take care of that as well!

Comment on lines +558 to +559
the stream or DATAGRAM with the corresponding RTP stream. The endpoint can
buffer streams and DATAGRAMs using an unknown flow identifier until they can be
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use the correct RFC 2119 wording here:

Suggested change
the stream or DATAGRAM with the corresponding RTP stream. The endpoint can
buffer streams and DATAGRAMs using an unknown flow identifier until they can be
the stream or DATAGRAM with the corresponding RTP stream. The endpoint MAY
buffer streams and DATAGRAMs using an unknown flow identifier until they can be

Copy link
Owner Author

Choose a reason for hiding this comment

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

I used non-RFC 2119 language here because it is nothing that a peer needs to be prepared for. Some time ago, we removed a lot of SHOULDs and some MAYs so I would like to avoid adding more new ones unless it is necessary. @SpencerDawkins and @joerg-ott, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree with Mathis. This paragraph gives implementation guidance that does not affect normative protocol behavior. The actions are of local significance only.

Copy link
Collaborator

@SpencerDawkins SpencerDawkins Jun 10, 2024

Choose a reason for hiding this comment

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

Warning, I'm a retired AD and still a BCP14 pedant ...

What @mengelbart and @joerg-ott are describing is a bunch of changes that we made based on my previous comments (trying to limit the use of BCP14 language to an absolute minimum), for two reasons.

  • As noted, the point of BCP14 language is to describe what a receiver MUST be prepared to deal with, and that includes when a sender does something besides what it SHOULD do. That's the difference between SHOULD and MAY - we are supposed to analyze the reasons behind a SHOULD, and the actions a receiver ought to take if the sender doesn't do that.

For some reason, RFC2119 also included MAY/OPTIONAL, which doesn't have much to do with interoperation (I can't remember which AD I'm paraphrasing, but "an implementation MAY do a LOT of things that a receiver has to deal with, and saying that using BCP14 language isn't helpful".

  • In the specific case of this draft, we're targeting Experimental, with a planned advance to Standards Track when we have more implementation (and perhaps even deployment) experience. We tried to minimize the use of normative language, just to focus on the bare minimum that an implementation needs to be prepared to deal with.

If a receiver buffers at least some media with surprising flow IDs, but doesn't buffer all media with surprising flow IDs for ever, the RTP sender probably can't figure that out. The receiving user can tell that, of course, but that's what we're trying to provide a warning about.

If we do create an issue about surprises for readers, the way we're using BCP14 language should probably be included in that list!

@SpencerDawkins
Copy link
Collaborator

SpencerDawkins commented Jun 10, 2024

@mengelbart, I'd suggest creating a new issue 😀 with the points you are addressing from discussion at the interim meeting. I'll comment on the comments from @samhurst, but I suspect that this PR has enough interaction with what we're currently calling sdp-roq, based on last week's discussions, that we really do need to up-level ,in order to make the right changes in THIS draft, and my experience with GitHub is that discussing what a PR is trying to do and discussing the changes proposed to do it, isn't as productive as discussing what a PR will be trying to do in an issue.

I haven't gone through the YouTube recording from the interim meeting carefully (yet) for our presentations, but I'll be doing that, and updating the minutes for both presentations, today or tomorrow.

@samhurst, thank you for following our work! You're seeing the current text with newer eyes than mine, @joerg-ott's, and maybe even @mengelbart's, and it's helpful to hear about anything you see that doesn't seem clear, correct, or useful.

@mengelbart mengelbart linked an issue Jun 10, 2024 that may be closed by this pull request
@mengelbart
Copy link
Owner Author

Thanks, @spencer! I created Issue #223 with a list of points that came up in the discussion and which I tried to resolve in this PR.

@SpencerDawkins
Copy link
Collaborator

Thanks, @spencer! I created Issue #223 with a list of points that came up in the discussion and which I tried to resolve in this PR.

Excellent! I suggest that we hold off for a day or two while I figure out what REALLY happened during our sessions at the meeting (which includes looking at the YouTube recordings, and at the chat, and updating the minutes), and then refocus later this week.

Does that make sense to people?

@mengelbart mengelbart force-pushed the fix/signaling-dependency-and-buffering branch from e0f2159 to 4e1ae7c Compare June 21, 2024 14:55
@mengelbart mengelbart merged commit f905fdd into main Jun 21, 2024
2 checks passed
@mengelbart mengelbart deleted the fix/signaling-dependency-and-buffering branch June 21, 2024 14:56
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.

Resolve Signaling dependency discussed in interim meeting
4 participants