Skip to content

Count object extension headers in bytes, not fields #723

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

Merged
merged 2 commits into from
Feb 26, 2025

Conversation

vasilvv
Copy link
Collaborator

@vasilvv vasilvv commented Feb 25, 2025

This makes it possible to skip those without parsing, and easier to parse without trial decoding.

Fixes #718

This makes it possible to skip those without parsing, and easier to
parse without trial decoding.

Fixes moq-wg#718
* Object Extension Count: The number of Object Extensions present. A value of 0
indicates that no Object Extension Headers are present.
* Object Extension Size: The total size of the Object Extension Headers block,
in bytes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need to update Canonical Field section as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the Canonical Object Fields section, though?

@@ -2680,7 +2680,7 @@ STREAM_HEADER_GROUP {
}
{
Object ID = 0
Extension Count = 2
Extension Headers Size = 33
Copy link
Collaborator

Choose a reason for hiding this comment

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

:) did you count the bytes to be 33 :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but I did it in a calculator, so it may be not 33 (and of course, varint serialization is ambiguous).

@ianswett ianswett added the Wire Format Related to how messages are serialized and parsed label Feb 25, 2025
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:

lgtm.

Do we need to specify an error when the length of the extensions block exceeds the Extensions Length?

Copy link
Collaborator

@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.

LGTM

@ianswett ianswett merged commit 05cdbec into moq-wg:main Feb 26, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Wire Format Related to how messages are serialized and parsed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change from extensions count number to length of all extensions
5 participants