-
Notifications
You must be signed in to change notification settings - Fork 23
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
Rewrite the Priority section to improve clarity #518
Conversation
This change provides a step-by-step algorithm for how to pick the next object to send, and which priorities take precedence over another, all in one place. This PR may or may not change the normative behavior of priorities depending on one's interpretation of the current text (which, as we learned in today's call, appears to vary between people). Fixes moq-wg#517
@fluffy You've expressed interest in seeing a step-by-step algorithm for sending things to be explicitly spelled out before. Does the text in this PR match your expectations of how this should look like? |
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.
LGTM
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.
Individual Comment
1. If two objects have the same subscriber priority, but a different publisher | ||
priority, the one with **the highest publisher priority** is sent first. |
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 implies that publisher priority trumps subscriber group order. That was not how I read the previous algorithm. Higher publisher priority would select a subscription to send from, then you go through to group order. This is how we got min.
I like the property that says group order is ascending or descending only, and priority doesn't influence group order. Trying to use the object priority as both intra and inter track is weird.
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.
Trying to use the object priority as both intra and inter track is weird.
I agree that it might feel a little bit weird, but if priority is applied before group order, this makes sense, since this should be more expressive than just having two of those.
As to publisher priority vs group order, I feel like for the use cases that I can think of, the group order should always apply after the publisher priority. E.g. for the active vs non-active speaker case, you'd always want to prioritize the more-useful audio over less-useful regardless of stream position; or for SVC, you'd probably rather have all of the base layer frames than have the newest group in the best quality and a missing group right before it.
(Of course, even if that is not true, you can choose the opposite behavior by setting publisher priority as the same for all track, and then using peep IDs to prioritize those within the 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.
The problem that I see if that the publisher gets the final say on group order here -- if the subscriber asks for one track with pri=0 and order=asc, but the publisher sets the peep priorities for each group in descending order, the data will be delivered dsc. I'd find it more consistent and easier to think about if we separate publisher track priority from peep/object priority (Martin's Option 3). Then it's sub pri -> pub pri -> group order -> peep/obj pri -> peep ID
.
In any case, this PR doesn't feel like a clarification so much as a change to the algorithm.
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.
Adding an explicit track priority is completely reasonable, though it'd be good to understand what cases it was necessary? For SVC, I'm quite sure you'd want the peep priority to be more important than group order.
We only have 256 priority values, so your example won't last that long for most use cases. Also, your example is somewhat adversarial, so I'm not sure how realistic it 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.
I created #519 to discuss the question of whether publishers should specify a priority in addition to the subscriber 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.
Looking at wills slides from seattle interim it is clear that publisher priority override group id other wise nearly all the uses cases that use publisher priority don't work. I think if we just step back and look at uses cases, we can reason our way though the conversation we have had several times and end up back at the space spot of what Victor has in the PR now
There are not enough priorities codes for a publisher to even do the example Alan raised at top of this thread. I view that sort of use cases as just a broken application if the publisher and subscriber are trying to do totally conflicting things and does not bother me much that weird things happen in that case.
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.
Individual Comment:
I agree the Seattle proposal was: pick a track by sub priority, then min(pub-priority-of-pending-objects), select a group by group order, order by object ID.
There's a complaint that min priority is too hard to implement. Sounds like the new proposal is:
Pick a track by sub priority, then pub priority, then group by group order, order by peep/object ID.
Here's what I think is a weird case:
The subscriber asked for ascending groups.
In group 1, all objects have the same pub pri = 100.
In group 2, some objects have pub pri 100 but some are more important and get pub pri 90
-> The publisher wants to express relative priority of the objects within the group
The new algorithm will deliver group 2 pri=90 objects first and that seems wrong. The Seattle algorithm would send group 1 object in obj ID order, then group 2 pri=90 objects, then group 2 pri=100 objects. This seemed more correct, if harder to implement. You can sort of fix this by incrementing the base priority of objects in every group, but you will run out of priority levels after 256 groups.
There seemed to be general consensus that groups would only be delivered ascending or descending and I think we should try to preserve that property -- do people care less about this now? I'm inclined to think we need to set aside some time to discuss at the hybrid interim next month.
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.
One observation about ascending / descending order is that it seems the use case for it are all times where it is a fetch not a subscribe. It also seems that for flow control reason fetch will likely flow over one stream. I wonder if we could just remove descending order for subscribe. I've pointed out in the past it is totally useless and in most cases where the things are not congested and the subscribe is on the live edge, the subscribe will get things in the order the publisher published them regardless of how any of the priorities and orders are set. This will mean a subscriber asks for things in reverse order and will then get most things in forward order with perhaps a few object in reverse order then there was a bit of congestion. I don't object to it being here if others want it because it is all trivial to implement but I remain of the opinion order is unless for subscribe and critical for fetch.
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.
Some nits, but I think this is very close.
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
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 a huge improvement and I like it.
I do have one questions about if we need the concept of scheduleable that I would like to get to resolution on.
|
||
_Subscriber Priority_ is a priority number associated with an individual | ||
subscription. It is specified in the SUBSCRIBE message, and can be updated via | ||
SUBSCRIBE_UPDATE message. The subscriber priority of an individual schedulable |
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 would add here something along lines of "when it is changed, in a relay it is implementation dependent on how it will apply to object that the relay has already received".
Clearly if a something had been pushed into the QUIC stack but not yet pushed to the wire, it would be pretty hard to change. I think it is fine to leave this very implementation dependent because this is something that is happening for a short time on the edge of the change and weather a given object gets the old or new priority during the switch over does not matter much to apps.
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 tried to add a clarifying sentence, PTAL
## Definitions | ||
|
||
MoQT maintains priorities between different _schedulable objects_. | ||
A schedulable object in MoQT is either: |
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.
Let me ask a questions first. If every object was a schedulable object, does it change the algorithm in the rest of this in any way ? When I read the rest of this I think the answer is No but I feel like others might feel the answer is yes and that I am missing something that is a key issue for other people.
The way I would implement this is put all the object on a priority queue where the priority on the q was an integer basically formed by concatenating subscriber priority, publisher priority, and either the group id or reverse group id based on direction, then peep id, then object id. Then when there is space available send an object, you grab the next object out of queue and and send it. I want to understand if this is wrong. If it is wrong, I am probably fine with what people want but I think it will help me get to understanding why people keep saying this is complicated. If this is right, then I don't think we need the concept of a schedulable object.
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.
@vasilvv - This question here is the only question I have of significance on this PR. Love the PR - thanks.
subscription, **the group order** of the associated subscription is used to | ||
decide the one that is sent first. | ||
1. If two objects belong to the same group of the same track received through | ||
the same subscription, the one with **the lowest Peep ID** (for tracks with |
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.
Not a big deal but I would have a preference of limiting peep ID to 8 bits if they are going to be used as one of the priority fields.
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 do not see any reason to add peep into this. If the publisher wanted the object in one peep to be delivered ahead of another peep, it should have set the publisher priority. I can probably live with this but I'd like to understand why we need it.
At interim meeting, decided to give Will 2 weeks to write an alternative PR to this one and can't get something better, merge this. |
Are you sure that was me? I don't have an alternate PR in mind. |
@afrind has the token to produce an alternative |
Alternate proposal is here: #585 |
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.
Just updating my review after all the conversation in the WG. We might need to tweek a bit of this later but I am in favour of merging this more or less as is.
This change provides a step-by-step algorithm for how to pick the next object to send, and which priorities take precedence over another, all in one place.
This PR may or may not change the normative behavior of priorities depending on one's interpretation of the current text (which, as we learned in today's call, appears to vary between people).
Fixes #517
Closes #585