Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Priority and Group Send Order to SUBSCRIBE, clarify Publisher Priority #470
Add Priority and Group Send Order to SUBSCRIBE, clarify Publisher Priority #470
Changes from 27 commits
e43f244
0d45b9d
c1346ec
31b0ba2
0b693ae
f1b88f4
f5043c6
3c0f6e5
9097d73
81c6a32
9fc4721
619bc11
436605f
081554d
a085820
441ecf0
a97a91f
7ddfd11
1bd6788
c04f70e
6804eca
8d41133
b6ec889
73376b3
13d2e4c
751c65e
a6ed5ef
a2a29de
09e6d8e
74ca340
e32a488
4da15a1
779ddae
cc451f2
d366503
37fa963
0d351ff
c528c9c
fd21a0d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's less confusing if you say publisher priority is per stream, instead of per track.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will need to update this bit, since we agreed it's latest, not highest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is right as it is worded ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We agreed on highest at this point, the latest is for other part. So I think this is correct as written here. Just to reason about this this, think about a track being send stream per object where publisher priorities changed. I don't think publisher priority is per stream - to start with it is for datagrams too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was part of the discussion from Wednesday's call. The consensus was that using highest priority value in the track to select a track does not work well. If you use highest, then priority can only be increased and not decreased.
Example, assume two tracks, stream-per-group and each has 2 groups to send from:
Track A: group1, pri=1; group2, pri=2
Track B: group1, pri=2; group2, pri=1
Using max, the algorithm would treat these tracks with equal priority, and move to another tiebreaker.
The room felt (I think unanimously) that the most recent value should set the track priority, so in the above example, Track B would be higher than Track A. Which group from Track B is determined by the group-send-order.
Reflecting more, I think logically what bits we want to send in the above example depends on the group order. If using ascending groups, a track's priority is the priority of it's lowest group to send and if descending, a track's priority is the priority of it's highest group to send. Now, if Track A and Track B are both ascending, A > B. If they are both descending, B > A. If they mismatch, it is a tie.
When using stream/datagram-per-object, the priority of a group is the highest priority among queued object for that group.
That feels right to me but the implementation might be complex to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not that
sub-pri == orig-pub-pri
, it's whensub1.pri == sub2.pri
andpub1.pri == pub2.pri
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given we use "send order" to talk about Ascending / Descending, I got confused when I read this. I think it could be less confusing to replace "send order" with "track selected to send next object" or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ send order is / the track whose object is being selected is /
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, this text was a bit unclear. I rewrote it to hopefully be more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems slightly under specified. If a last mile relay received two different values in both SUBSCRIBE and SUBSCRIBE_OK, we need to say which one takes priority. We agreed on the end subscriber would take priority so this should have a bit more text to say value in SUBSCRIBE overrides in value in SUBSCRIBE_OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I rewrote this paragraph to better match the current framing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems correct as written and covers the case where the publisher priority is the same but we need to also cover the case where where publisher priorities are not the same in which case publisher priority determines which is sent first. This may just be implied here but better to say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I missed "...and the same priority level".
That may be workable - it affords an option to create a "virtual layer" within the track, which may be what Mo was asking for re temporal scalability?
eg: 30 fps objects get
pri=<track priority>
and 60 fps objects getpri=<track priority> + 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I added a bit more text to clarify what happens when priority varies within a group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should add the note on publisher preference here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suhasHere : what's your suggestion? Something like:
In the absence of other specific reasons, relays SHOULD give all upstream subscriptions equal priority
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hesitant to spend text telling relays what to do, since they'll do what they want in reality. Telling them what not to do because it's a footgun seems more valuable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expanded the sentence a bit more to note that a relay can use a single Subscriber Priority value for all upstream subscriptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A subscriber will often want to use the default publisher priority, so this isn't always true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, I changed this to text that points out the subscriber priority provides a mechanism to override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may have a problem here. If we expect aggregating relays to not forward subscriber priority preferences, then, in fact, all middle hops are likely to fall to publisher priority and may incentivize priority inflation. On the other hand, relays are going to do what they're going to do, and, it's possible that publisher priorities may be ignored for middle hops in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get what you are saying here but relays have to deal with a reasonable solution to this just to even be able to run lots of different applications on the same relay network even when application never have more than one namespace. I think there are two keys parts of this 1) we are not trying to specify how a relay network moves data around inside the relays and 2) relays will do what relays do.
So thought relays could have if they don't think about this problem, I don't think we can say much more that this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will work in practice. There's too many use-cases where publishers are not coordinated because they're using different software or versions.
ex. I want to show multiple Twitch streams on the same page. There's just no way that I can have OBS and ffmpeg agree to publish with the same priority scheme. It doesn't have to be malicious; one of the publishers is undoubtedly going to monopolize the connection.
Dialing a new connection is expensive and removes the ability to use subscriber priorities. I would totally agree with using multiple QUIC sessions if it had such a layer, but the alternative is WebTransport pooling...
And yes, you could use subscriber priorities only to avoid this issue, but that's a big limitation that could be easily solved by sandboxing namespaces. Relays can't use the subscriber's priority anyway and will have to sandbox namespaces if using MoQ over the backbone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all non-normative text intended to describe a potential problem. I don't want to create yet another session layer in MoQ when there's already one in WebTransport and eventually there might be one in QUIC. Different applications are different, and for some apps, multiple namespaces may be fine, and for others it's not. Possibly there's a better way of saying that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather say that a relay shouldn't compare publisher priorities from uncoordinated publishers, but comparing them from coordinate publishers is fine. Within a namespace, we can assert all tracks have coordinated priority, but between two namespaces, it would presently have to be implementation dependent. That said, it shouldn't be hard to fix - break off part of namespace into another tuple field (application or something).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alan, I don't think that would help, that just moves the same problem to the next layer and it would be turtles all the way down. The basic issue is when a content delivery network delivers content for lots of different applications/customers, it needs to decide how it will be "fair" between the application/customers and that is way more complicated that just use whatever the priority header asked for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Ian. it is tough to be prescriptive and the proposed text pretty much is well defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Luke that saying "this won't be a problem" is not good. We should say that there's some risk to blindly comparing publisher priorities when you don't know if they are coordinated, caveat emptor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this with more nuanced language that indicates namespaces may or may not have compatible prioritization schemes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok with merging the new language. Long term, I don't plan to compare uncoordinated publisher priorities in my generic relay, and may make a proposal for how to avoid doing so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A proxy is a relay too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent of this sentence is to make it clear that they're different. Which I think is useful, because proxies typically have substantially different functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't a relay be a 1:1 relay? I'd just chop the sentence at
The recently merged terminology says
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets a bit off topic, but I think the more important property of a relay is that it republishes tracks from an upstream publisher without modification. I think there may be another entity we could define which could consume one or more tracks from upstream publishers and act as the original publisher for some new tracks (e.g. in the case of "manifest manipulation" use cases at the edge), but I think that's something we should discuss separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for this bit of text, I'm fine with it either way. To me, it makes explicit that relays may optionally be relatively stateless fan out nodes without any significant local caches, so long as they still allow downstream subscribers to subscribe to tracks from an upstream publisher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the definition, I'll shorten this sentence and clarify some other text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about changing the group order too? It would be useful for toggling between reliable and unreliable playback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 add Group Order to Subscribe Update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry that could create a number of edge cases. A large benefit of SUBSCRIBE_UPDATE vs UNSUBSCRIBE/SUBSCRIBE is it avoids missing Objects or double-sending Objects.
But I'm not writing the code, so if you think it's useful and won't add unnecessary complexity, I'll add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omitting this from subscribe update in favor of unsub/sub is ok, but you might want to add a note that it's intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @ianswett
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Luke, I thought you were complaining about too many options :-) I don't see the use case with this but at first glance does not seem like it would complicated the implementation too much so I am fine with it getting added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a sentence in the Priorities section about how you need to use UNSUBSCRIBE and SUBSCRIBE.
One use case I can think of (ie: switching from live head (Descending) to slightly behind live (Ascending)) is better suited to UNSUBSCRIBE and SUBSCRIBE anyway, because you'd want to specify the new start point at wherever you currently are in time, not what's in scope for the current subscription.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new text looks good.