moq-wg / moq-transport

draft-ietf-moq-transport
Other
70 stars 16 forks source link

Add object status used to indicate lost or nonexistent objects #429

Closed fluffy closed 2 months ago

fluffy commented 2 months ago

This PR adds a status field to the object that allows an relay to indicate an object or group was lost or dropped.

It also allows a producer to indicate a object or group will not be produced including the cases for end of group and end of track.

The PR probably needs a bit more text on what what producers and relays do but I think this is close enough to get some initial discussion on this solution.

Fixes #318 Fixes #427 Fixes most of #423

Closes #334 Closes #426

ianswett commented 2 months ago

I think this fine. Like you, at this point I don't really care much how we spell it.

Compared with my proposal in #426, this is conceptually way simpler (no special rules for each forwarding preference) but is less byte-efficient. I'm happy to go with whatever the group decides.

I think I'm happy to use a byte for this. We can always optimize it later in a number of ways if we want.

ianswett commented 2 months ago

I think this fine. Like you, at this point I don't really care much how we spell it. Compared with my proposal in #426, this is conceptually way simpler (no special rules for each forwarding preference) but is less byte-efficient. I'm happy to go with whatever the group decides.

I think I'm happy to use a byte for this. We can always optimize it later in a number of ways if we want.

We can actually save a byte by omitting Object ID except for the first Object in the stream, because this allows the Objects to be sequential. Similarly, we can omit GroupID if the delivery preference is track per stream.

ianswett commented 2 months ago

Individual Review:

Most of the statuses are not mutually exclusive (eg: an object can be available at the publisher, the end of a group, the end of track and dropped by the relay), though some are (an object can't be the end of track without being the end of a group, I think?). I wonder if there's a different spelling that captures this?

The dropped/end/never-existed signals seem more important than other objects and we may need to be more specific about reliability properties and/or stream placement.

This is a good point. Conceptually we need two enums: One to indicate if it's a Group Fin or Track Fin and one to indicate why there's no payload.

There are a lot of ways to encode that, but it's a bit nice, because the first enum is an immutable property of the Object and the latter is sort of a payload alternative.

kixelated commented 2 months ago

I've only had a chance to glance, but how does this interact with RESET_STREAM?

Putting drop notifications within the OBJECT itself means that an object can only be dropped prior to being written to a stream. This is okay when used in conjunction with stream priorities, but it's not great. Ideally we can renege after committing data to a stream by resetting it.

IMO there should be a message indicating which objects were dropped upon a RESET_STREAM.

fluffy commented 2 months ago

Notes to myself:

Need text on reset.

Way to deal with sending zero length object

Way to indicate end track and I dropped it.

suhasHere commented 2 months ago

Notes to myself:

Need text on reset.

  • could send object group drop on new stream
  • open an issue if we need this it

Way to deal with sending zero length object

  • add an enum for this object is empty

Way to indicate end track and I dropped it.

  • perhaps better way than sending same object twice

@fluffy i did leave the following comment https://github.com/moq-wg/moq-transport/pull/429#discussion_r1569172957

kixelated commented 2 months ago

After reviewing fully, yeah I don't think we can decouple drops from resets.

When any publisher (including a relay) decides to drop an object, it will also reset the associated QUIC stream. Otherwise, the publisher has to delay writing the object to streams (like TCP low watermark) so it retains the ability to drop it.

And the problem with TCP low watermark in MoQ is of course... priorities. Newer content is usually more important, so even if we already have N bytes queued in the QUIC layer, our new content needs to skip the queue. The only way to do that is to actually write to the QUIC stream marked at a higher priority... or reset streams currently queued. The PR doesn't allow either of these as I understand.

kixelated commented 2 months ago

I like what Cullen suggested in the editor's meeting. When we want to drop an object, we RESET_STREAM and write a dropped message on a new stream (that won't be reset). Relays will forward this message too. The caveat is that the object may have been successfully delivered despite the RESET_STREAM so the subscriber has to be prepared to ignore some drop messages.

kixelated commented 2 months ago

Sorry I left a PR review a little too fast between meetings and taking care of the newborn. I had a chance to think things through a bit more instead of leaving vague and passive-agressive messages like:

I think there's a better way to signal drops though than to retransmit the same OBJECT but with a different status/payload after a RESET_STREAM.

I'm warming up to the status in OBJECT, however it needs to be a bitmask. An object can be both dropped, the end of a group, and the end of a track. It's a little annoying to have to process duplicate OBJECTs with different statuses but it's fine I guess.

An alternative would be a reliable SUBSCRIBE control stream that contains reliable messages GROUP_END, TRACK_END, OBJECT_DROPPED messages. There would be no need to retransmit anything on a RESET_STREAM but the overhead would be marginally higher.

ex. GROUP_END would consist of the message type, the group_id VarInt, and a object_id VarInt only. The track would be inferred from the subscribe control stream. The added benefit is that a subscriber would know the size of the group BEFORE receiving the final object; it would know how much is still in flight.

afrind commented 2 months ago

An object can be both dropped, the end of a group, and the end of a track

I think Cullen's update addressed this by making "end of group" and "end of track" have different object IDs than the last actual object. Should we further state that only objects with status 0 can be "dropped by a relay"? All the other statuses may be needed by the subscriber, and all should be very small.

Would you be ok with moving this PR forward if we split out 0x5 and 0x6 for now?

fluffy commented 2 months ago

Note to cullen - remove the two codes. Clean up other comments. Ping Ian when done and ready to merge.

fluffy commented 2 months ago

@ianswett - Can you give this a read and merge if you think it is OK. I did make the change in the PR luke sent to this but it ended up in another PR so github did not see it as done.

suhasHere commented 2 months ago

I did another read through and it looks good to me !! Thanks Cullen

afrind commented 2 months ago

Individual Comment:

For applications where they make no sense ( like audio only ) the publisher would not bother.

This means a relay cannot assume the existence of these marker objects. That's unfortunate, because I'd like the end-of-group marker to help my relay know when to close fanout streams. I don't want to rely on QUIC FIN for this - a publisher may use a QUIC FIN to terminate a stream cleanly (delivering all written objects) as part of a graceful shutdown, without wanting to signal the end of group. In this case, I explicitly don't want to close fanout streams, in case the publisher is reconnecting (make before break).

Would making end markers optional for DATAGRAM tracks sufficiently address the audio only use case?

fluffy commented 2 months ago

For applications where they make no sense ( like audio only ) the publisher would not bother.

This means a relay cannot assume the existence of these marker objects. That's unfortunate, because I'd like the end-of-group marker to help my relay know when to close fanout streams. I don't want to rely on QUIC FIN for this - a publisher may use a QUIC FIN to terminate a stream cleanly (delivering all written objects) as part of a graceful shutdown, without wanting to signal the end of group. In this case, I explicitly don't want to close fanout streams, in case the publisher is reconnecting (make before break).

Would making end markers optional for DATAGRAM tracks sufficiently address the audio only use case?

I get your point and need to think about all of that a bit. Lets say the client did send them, would it be a problem if it just crashed without sending them. Would the relay also have a timer or some other way to clean up the state ? Or just wait for subscribe to end ? Just thinking through implications but you raise a good point.