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

moq-transport draft-06 initial wire format updates #8

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

englishm
Copy link
Owner

@englishm englishm commented Oct 29, 2024

Work in progress. Includes contributions from @zafergurel's #7 branch.

Draft diffs:

Needed for wire format compatibility:

Since there's a fair bit to tackle there still, I'm mostly interested in getting parsing/encoding support in place first. We can open additional issues to follow up and add correct behaviors implementing all of these features afterwards.

@englishm
Copy link
Owner Author

Added a version bump, took #7 and got everything building again. Still not able to round-trip media moq-pub -> moq-relay-ietf -> moq-sub again yet, so that's probably the next bit to troubleshoot, and then will look at the other checklist items from the draft-05 -> draft-06 diff.

@zafergurel
Copy link
Contributor

I'm closing #7.

@zafergurel zafergurel mentioned this pull request Oct 30, 2024
@englishm
Copy link
Owner Author

Rebased to pull in fix from #10

@lminiero
Copy link
Contributor

lminiero commented Oct 31, 2024

@englishm should I want to test the new wire format updates, what should I pull? Since I wanted all this to be ready in my library before the IETF and the hackathon, I ended up incrementally implementing the changes from all drafts, meaning that (at least in theory) I now dynamically support from -03 up to -07 (including a basic demo of FETCH). I could only test -06 and -07 against myself, though, so I really have no clue if I implemented everything correctly (including the control message length, which was a considerable refactoring in my code).

@englishm
Copy link
Owner Author

@lminiero as you can see I'm still in the process of making those same incremental updates to get us up to draft-07 as well.

We don't (yet) have version negotiation logic to support multiple version simultaneously (we just fail at SETUP on incompatible versions) so my plan for now is to maintain branches for each draft version as we add support for it. That way we can backport non-version-specific fixes if we want to interop with other implementations on older drafts.

So far we have:

Once I complete and land this pull request's changes, I'll create another set of releases for crates.io and also start a draft-ietf-moq-transport-06 branch. Then I'll start in on the draft-ietf-moq-transport-07 changes.

I'm not sure how much of this I'll get through before next week, but I'm aiming for having at least basic object and message parsing and encoding in place for draft-07, even if we don't have all the correct relay behavior implemented yet.

P.S. Maintaining dynamic support for all draft versions at once sounds like... a lot. I'm not sure I'd recommend that while the rate of change is still so high, but I'd be happy to try interop against your implementation with the versions we've implemented so far. You can find me on the quic.video Discord or on the video-dev Slack or various other places if you'd like to coordinate that.

@lminiero
Copy link
Contributor

lminiero commented Oct 31, 2024

We don't (yet) have version negotiation logic to support multiple version simultaneously (we just fail at SETUP on incompatible versions)

Yeah, due to the control message length change in -06 I had to split dynamic negotiation in two parts, "legacy" (3/4/5) and "any" (6/7) myself.

So far we have:

* https://github.com/englishm/moq-rs/tree/draft-ietf-moq-transport-04

* https://github.com/englishm/moq-rs/tree/draft-ietf-moq-transport-05

Thanks for the pointers! I'll definitely test against both, since I originally tested -03 with Luke's moq-rs, -04 with Meta's demos, and -05 with none at all... so expanding the interop tests would be indeed great! I'll keep you posted in case I bump into issues on either side.

P.S. Maintaining dynamic support for all draft versions at once sounds like... a lot. I'm not sure I'd recommend that while the rate of change is still so high, but I'd be happy to try interop against your implementation with the versions we've implemented so far. You can find me on the quic.video Discord or on the video-dev Slack or various other places if you'd like to coordinate that.

Yes, I definitely don't plan to keep them all there forever 😅 But -03 and -04 are where I started, and the only ones I had working interop with, so it felt like a pity to throw them away. This past couple of weeks I added -05, -06 and -07 and I somehow managed to keep most of the code unified, with just a few version-fences to do things differently when needed (most messages just change in one or two attributes anyway). Of course that's where I could bump into interop issues, e.g., where the fences don't really do their job and suddenly I'm sending broken messages.

I'll definitely ping you on the video-dev slack (we bumped into each other already there a few times!). I'm trying to clean up my code to make it releasable soon, but it's still a bit tricky to link to quictls properly in my Makefile: Fedora 40 has a convenient pkg-config custom name that makes it easy to link to it, but that's not something I can rely upon once people use it on other systems. If you're ok with struggling a bit with the build system, I can definitely send you the code I wrote so far there.

@zafergurel
Copy link
Contributor

zafergurel commented Nov 2, 2024

@englishm
I've completed the following with the subgroup changes.

Remove missing group status (moq-wg/moq-transport#509, §7.1.1.1)

@zafergurel
Copy link
Contributor

The following are completed in #15 :

  • Track Namespace + Track as ordered N-tuple of bytes + sequence of bytes
  • SUBSCRIBE_NAMESPACE / UNSUBSCRIBE_NAMESPACE and OKs

@zafergurel
Copy link
Contributor

Implementing MAX_SUBSCRIBE_ID is only possible by first implementing the standard errors.

The draft says the publisher should close the connection with the error 'Too Many Subscribes' if a Subscribe ID exceeds MAX_SUBSCRIBE_ID. And 'Too Many Subscribes' is commented out as the rest of the standard errors in moq-transport/src/error.rs. Therefore, we need to use this file (by replacing the custom error messages) to conform to the standards.

englishm and others added 8 commits November 21, 2024 02:44
This may not be what we really want yet, but it seems to match what was
done on the moq-pub side and it builds, so that's a start...
This may not be what we really want yet, but it seems to match what was
done on the moq-pub side and it builds, so that's a start...
englishm and others added 17 commits November 21, 2024 02:44
This may not be what we really want yet, but it seems to match what was
done on the moq-pub side and it builds, so that's a start...
In draft-06 and going forward we don't have separate Group per Stream
and Object per Stream modes, but can control the number of objects which
are put on each stream using SubGroups

We may want to come back around and provide convenience APIs for using
SubGroups in each of these ways in the future.
In draft-06 and going forward we don't have separate Group per Stream
and Object per Stream modes, but can control the number of objects which
are put on each stream using SubGroups

Removed from moq-transport, so skipping over it here, too.
This commit adds the new error type
"Too Many Subscribes" to moq-transport/src/error.rs.
However, this file is commented out and the
current change doesn't have an effect yet.
This commit includes the changes for adding subcribe
namespace messages. The track namespace tuple format
is not yet supported. Also, the methods for handling
these messages are not implemented yet.
This commit adds the new Tuple type x(tuple).
The commit changes the type of track namespace
from string to tuple. So many changes in one commit.
draft-06 change:

Object Payload Length ([PR 487](moq-wg/moq-transport#487), [§7.2](https://www.ietf.org/archive/id/draft-ietf-moq-transport-06.html#section-7.2), [§7.3](https://www.ietf.org/archive/id/draft-ietf-moq-transport-06.html#section-7.3), [§1.3](https://www.ietf.org/archive/id/draft-ietf-moq-transport-06.html#section-1.3-5.2.1))

Object stream was removed by the addition of Peeps/SubGroups, but we
still have datagrams so add this length field there.
This commit adds the length field to control messages,
client setup and server setup.
draft-06

encode/decode support for MaxSubscribeId

MAX_SUBSCRIBE_ID control message

- [PR 491](moq-wg/moq-transport#491)
- [PR 528](moq-wg/moq-transport#528)
- [§6.16](https://www.ietf.org/archive/id/draft-ietf-moq-transport-06.html#section-6.16)

Just parsing for the control message so far, not wired up anywhere yet,
will need error codes
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.

3 participants