Header Length for improving parsing #520
Conversation
There was a problem hiding this comment.
Individual Review:
We also need to specify what happens when:
- the section ends before the section length
- the section extends beyond the section length (eg: the varint length would extend past the section length).
Probably ought to make both a protocol violation and tear the whole session down?
draft-ietf-moq-transport.md
Outdated
| ~~~ | ||
| OBJECT_DATAGRAM Message { | ||
| Object Header Length (i), |
There was a problem hiding this comment.
This one isn't strictly necessary because it's a datagram -- you have to have the whole message so there is no need for trial parsing.
|
I think this is good for control messages, but I'd mildly prefer to keep the data stream headers without length, since those are sensitive to the framing overhead. |
draft-ietf-moq-transport.md
Outdated
| @@ -1877,17 +1893,20 @@ Sending a track on one stream: | |||
|
|
|||
| ~~~ | |||
| STREAM_HEADER_TRACK { | |||
| Stream Header Length = 3 | |||
There was a problem hiding this comment.
I would normally consider payload length var int as a header. It seems a bit weird that a separate var int isn't considered a header.
There was a problem hiding this comment.
Trying to think about how I would code this either way. Lets say there were 62 bytes of data after this header. If I encode this number as 1 byte varint with value 63. Or I could encode as 2 byte varint with value of 64. Not sure how this changes what I think we should do - just an edge case worth thinking about.
draft-ietf-moq-transport.md
Outdated
| payload and send the Object as datagram. In certain scenarios where the object | ||
| size can be larger than maximum datagram size for the session, the Object | ||
| will be dropped. | ||
|
|
||
| The `Object Header Length` specifies the total encoded length in bytes for |
There was a problem hiding this comment.
Does it include the size of the header length itself?
draft-ietf-moq-transport.md
Outdated
| @@ -1797,10 +1801,13 @@ TODO: figure out how a relay closes these streams | |||
| When a stream begins with `STREAM_HEADER_TRACK`, all objects on the stream | |||
| belong to the track requested in the Subscribe message identified by `Subscribe | |||
| ID`. All objects on the stream have the `Publisher Priority` specified in the | |||
| stream header. | |||
| stream header. The `Stream Header Length` specifies the total encoded length | |||
| of the header in bytes which includes Subscribe ID, Track Alias and Publisher | |||
There was a problem hiding this comment.
May not need to call out each header as including ...
Suppose it depends how often streams are changing. Adding object header length does add overhead, but it should be one byte in most uses. |
|
Victors comment is really making me think about if we need this on objects or not. I'm very torn and think we should talk about how to do while still keeping the framing overhead as low as possible. |
|
Chair Comment: It seems like there's general support for control messages, but some discussion required for objects. Can we move the object changes to a separate PR so we can land the control changes for the next draft cut? |
draft-ietf-moq-transport.md
Outdated
|
|
||
| ~~~ | ||
| STREAM_HEADER_TRACK Message { | ||
| Stream Header Length (i), |
There was a problem hiding this comment.
I wouldn't bother fixing STREAM_HEADER_TRACK, since I expect it'll be removed soonish.
Revert what I think is an unintentional change
draft-06 change See: [PR 520](moq-wg/moq-transport#520), [§6](https://www.ietf.org/archive/id/draft-ietf-moq-transport-06.html#section-6), [§1.3](https://www.ietf.org/archive/id/draft-ietf-moq-transport-06.html#section-1.3-5.2.1) We're not doing anything particularly useful with this. Partly, this is because there seems to be agreement that messages shouldn't be skipped, and it's not clear what else we'd use these lengths for.
Addresses #474