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

Add SUBSCRIBE_UPDATE #385

Closed
wants to merge 12 commits into from
Closed

Add SUBSCRIBE_UPDATE #385

wants to merge 12 commits into from

Conversation

ianswett
Copy link
Collaborator

@ianswett ianswett commented Feb 19, 2024

Fixes #363, Fixes #269

This attempts to provide the minimum functionality I expect we'll need, regardless of how SUBSCRIBE is changed in the near future.

@ianswett ianswett added the Subscribe Related to SUBSCRIBE message and subscription handling label Feb 19, 2024
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
draft-ietf-moq-transport.md Outdated Show resolved Hide resolved
SUBSCRIBE_UPDATE Message {
Subscribe ID (i),
EndGroup (Location),
EndObject (Location),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be optional and only set if the subscription needs to be ended.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the context of this PR and the current draft, where the only thing you can change is the end, this message is OK. However, if we add more parameters to subscribe (eg #381), then we need a way to update some of the parameters and not others.

We should also clarify what happens if you issue the SUBSCRIBE twice for the same ID - I expect in this PR as is, we would make that an error?

@@ -744,7 +744,7 @@ MOQT Message {
|-------|-----------------------------------------------------|
| 0x9 | UNANNOUNCE ({{message-unannounce}}) |
|-------|-----------------------------------------------------|
| 0xA | UNSUBSCRIBE ({{message-unsubscribe}}) |
| 0xA | SUBSCRIBE_UPDATE ({{message-subscribe-update}}) |
|-------|-----------------------------------------------------|
Copy link
Collaborator

@suhasHere suhasHere Feb 20, 2024

Choose a reason for hiding this comment

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

I feel Unsubscribe makes more sense if one wants to end the subscription.
We don;t need a new message to update an existing subscription. A subscriber can update an existing subscription by sending SUBSCRIBE message with SubscribeId that matches an existing subscription ( as described in #381)

modification to the end of an existing subscription. One common type of
modification is unsubscription.

If an update cannot be completed by the relay, it replies with a SUBSCRIBE_ERROR
Copy link
Collaborator

Choose a reason for hiding this comment

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

But what about SUBSCRIBE_OK? We should send it too, especially if the end groups use relative offsets.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also what if there are multiple SUBSCRIBE_UPDATES? Do the SUBSCRIBE_ERRORs have to be returned in order? With no OK, it's impossible to differentiate if the 1st or 2nd UPDATE failed.

modification is unsubscription.

If an update cannot be completed by the relay, it replies with a SUBSCRIBE_ERROR
with error code 'Update Failed' and the subscription is unchanged.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like the subscription being unchanged. It causes a partial failure which is just really difficult to support.


~~~
UNSUBSCRIBE Message {
Subscribe ID (i)
SUBSCRIBE_UPDATE Message {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also update the start group/object? It would be super useful for relays to deal with:

viewer1: SUBSCRIBE start=-2
(pause)
viewer2: SUBSCRIBE start=-3

Currently, the relay would issue to origin:
SUBSCRIBE start=-2
(wait for SUBSCRIBE_OK -2=65)
SUBSCRIBE start=64 end=65

Which is complicated and incurs delay. But if we allow updating start ranges:
SUBSCRIBE start=-2
SUBSCRIBE_UPDATE start=-3

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updating the start point can mess with object mapping to streams, especially moving backwards. In your example, if the track is stream-per-track, the publisher may open a stream and being publishing group=65. In the current design, where subscription ID appears in objects, the second subscribe for group=64 will come on its own new stream. If we allow updating the subscription start point backwards to 64, what should the publisher do? Should it close the open stream and open a new one, beginning at group=64, and resend objects from group=65?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do think updating the start is potentially interesting and useful.

Updates of the start should not be able to move the start point of the subscription earlier. As you said, it's just too complex. For similar reasons, I think updating the end to be farther in the future is error prone as well, though there may be use cases for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on start can only be moved forward, end can only be moved backwards

Copy link
Contributor

Choose a reason for hiding this comment

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

As a side note where I don't really care ... I'm not sure the use case to move the start forward

Copy link
Collaborator

@afrind afrind left a comment

Choose a reason for hiding this comment

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

Individual Review:

We should have a way to update the endpoint of a subscription (like subscribe update), and also a way to unsubscribe immediately when you don't know or care about the end. I suppose we can do that by setting the new end-point to absolute 0/0.


~~~
UNSUBSCRIBE Message {
Subscribe ID (i)
SUBSCRIBE_UPDATE Message {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Updating the start point can mess with object mapping to streams, especially moving backwards. In your example, if the track is stream-per-track, the publisher may open a stream and being publishing group=65. In the current design, where subscription ID appears in objects, the second subscribe for group=64 will come on its own new stream. If we allow updating the subscription start point backwards to 64, what should the publisher do? Should it close the open stream and open a new one, beginning at group=64, and resend objects from group=65?

SUBSCRIBE_UPDATE Message {
Subscribe ID (i),
EndGroup (Location),
EndObject (Location),
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the context of this PR and the current draft, where the only thing you can change is the end, this message is OK. However, if we add more parameters to subscribe (eg #381), then we need a way to update some of the parameters and not others.

We should also clarify what happens if you issue the SUBSCRIBE twice for the same ID - I expect in this PR as is, we would make that an error?

@ianswett ianswett changed the title Replace UNSUBSCRIBE with SUBSCRIBE_UPDATE Add SUBSCRIBE_UPDATE Feb 28, 2024
Copy link
Contributor

@fluffy fluffy left a comment

Choose a reason for hiding this comment

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

My main comment is I don't think we shoudl try and use UPDATE as the way we do UNSUSCRIBE

## SUBSCRIBE_UPDATE {#message-subscribe-update}

A subscriber issues a `SUBSCRIBE_UPDATE` message to a publisher to request a
modification to the end of an existing subscription. One common type of
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the "end of existing" should say the "end of the subscription range".


A subscriber issues a `SUBSCRIBE_UPDATE` message to a publisher to request a
modification to the end of an existing subscription. One common type of
modification is unsubscription.
Copy link
Contributor

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 is right. The update should just be to change the metadata about the subscribe, not change the state of the subscribe. We should use some other message to cause the SUSBCRIBE to end and remove all the state about the SUBSCRIBE.

@@ -761,7 +761,7 @@ MOQT Message {
|-------|-----------------------------------------------------|
| 0x9 | UNANNOUNCE ({{message-unannounce}}) |
|-------|-----------------------------------------------------|
| 0xA | UNSUBSCRIBE ({{message-unsubscribe}}) |
| 0xA | SUBSCRIBE_UPDATE ({{message-subscribe-update}}) |
|-------|-----------------------------------------------------|
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need both the UNSUBSCRIBE and UPDATE ( or whatever names we decide to call them ).

modification to the end of an existing subscription. One common type of
modification is unsubscription.

If an update cannot be completed by the relay, it replies with a SUBSCRIBE_ERROR
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be an UPDATE_OK/ERROR. Don't care if it is two message or not. But should be clear that if some part of the UPDATE can not be applied, then no part of the update is applied, subscribe stays with same meta data as before, and error returned. So an update is all or nothing.

A subscriber issues a `UNSUBSCRIBE` message to a publisher indicating it is no
longer interested in receiving media for the specified track.
The subscription is not fully terminated until a SUBSCRIBE_RST or SUBSCRIBE_FIN
are received.
Copy link
Contributor

Choose a reason for hiding this comment

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

Uh, I think this needs to be UNSUBSCRIBE or whatever the SUBSCRIBE CLOSE/STOP/FIN/RST/END is called


~~~
UNSUBSCRIBE Message {
Subscribe ID (i)
SUBSCRIBE_UPDATE Message {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on start can only be moved forward, end can only be moved backwards


~~~
UNSUBSCRIBE Message {
Subscribe ID (i)
SUBSCRIBE_UPDATE Message {
Copy link
Contributor

Choose a reason for hiding this comment

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

As a side note where I don't really care ... I'm not sure the use case to move the start forward

Subscribe ID (i)
SUBSCRIBE_UPDATE Message {
Subscribe ID (i),
EndGroup (Location),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a location this shoudl be an optional var int

SUBSCRIBE_UPDATE Message {
Subscribe ID (i),
EndGroup (Location),
EndObject (Location),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be var int not Location


* Subscribe ID: Subscription Identifer as defined in {{message-subscribe-req}}.

* EndGroup: The last Group requested in the subscription, inclusive. EndGroup's
Mode is None for an open-ended subscription.
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 prefer to make the end group / object optional in the update instead of having a None value. I think it makes a better patter for how to add future things to the update.

ianswett added a commit that referenced this pull request Apr 18, 2024
Replace #385 based on the simplified SUBSCRIBE.
@ianswett ianswett mentioned this pull request Apr 18, 2024
@afrind
Copy link
Collaborator

afrind commented Apr 27, 2024

Is this overtaken by #435 ?

ianswett added a commit that referenced this pull request Apr 30, 2024
Replace #385 based on the simplified SUBSCRIBE.

Fixes #269
Closes #385
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Subscribe Related to SUBSCRIBE message and subscription handling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UNSUBSCRIBE is underspecified Multiple subscriptions and updating a subscription
5 participants