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

QPACK: add examples #4093

Merged
merged 4 commits into from Sep 23, 2020
Merged

QPACK: add examples #4093

merged 4 commits into from Sep 23, 2020

Conversation

afrind
Copy link
Contributor

@afrind afrind commented Sep 11, 2020

Feedback welcome

Fixes: #1454

Feedback welcome

Fixes: #1454
@ianswett ianswett added -qpack editorial An issue that does not affect the design of the protocol; does not require consensus. labels Sep 11, 2020
Copy link
Contributor

@bencebeky bencebeky left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

draft-ietf-quic-qpack.md Outdated Show resolved Hide resolved
draft-ietf-quic-qpack.md Outdated Show resolved Hide resolved
draft-ietf-quic-qpack.md Outdated Show resolved Hide resolved
* Fixed typos
* Added more context around what the examples are doing
* Added a Stream Cancellation example
* Added more detail for field line representations
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

It looks like there is a small error (just with attribution, not the encoding), but this covers most of the important cases. It's a shame that the indices are so small, because I don't think that this provides enough information for someone to understand how references to the dynamic table work. All references in examples have an encoded index of 0.

Comment on lines 1580 to 1581
510b 2f69 6e64 6578 | Literal Field Line with Name Reference
2e68 746d 6c | (:path=/index.html)
Copy link
Member

Choose a reason for hiding this comment

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

It might help if you include the numeric values included in each of these descriptions. In this case, it's (static table 1).

Stream: Encoder
3f8b | Set Dynamic Table Capacity=170
01c0 0c77 7777 2e69 | Insert With Name Reference (static table)
6574 662e 6f72 67 | (:authortiy=www.ietf.org)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
6574 662e 6f72 67 | (:authortiy=www.ietf.org)
7466 2e6f 7267 | (:authority=www.ietf.org)


~~~
Stream: Encoder
3f8b | Set Dynamic Table Capacity=170
Copy link
Member

Choose a reason for hiding this comment

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

I read this as extending to the next byte.

Suggested change
3f8b | Set Dynamic Table Capacity=170
3f8b01 | Set Dynamic Table Capacity=170

~~~
Stream: Encoder
3f8b | Set Dynamic Table Capacity=170
01c0 0c77 7777 2e69 | Insert With Name Reference (static table)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
01c0 0c77 7777 2e69 | Insert With Name Reference (static table)
c00c 7777 772e 6965 | Insert With Name Reference (static table 0)

The encoder sets the dynamic table capacity, inserts a header with a dynamic
name reference, then sends a potentially blocking, encoded field section
referencing this new entry. The decoder acknowledges processing the encoded
field section.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
field section.
field section, which transitively acknowledges the encoder instructions up to the Required Insert Count.

Or something like that.

Stream: 4
0280 | Required Insert Count = 1, Base = 0
10 | Indexed Field Line With Post-Base Index
| Absolute Index=1
Copy link
Member

Choose a reason for hiding this comment

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

This one is useful to break down (RIC + delta_base + 0 = 1). Shame that we have such small numbers involved here.

@afrind
Copy link
Contributor Author

afrind commented Sep 14, 2020

@martinthomson: I can add a few more dynamic table entries to the examples to get indexes > 0 -- do you think 1-2 is good, or do you think it needs to be another order of magnitude?

@martinthomson
Copy link
Member

At most 2. The great thing about the current example is that it is small and easy to fit in your head. If you can reference the existing entries, that might help, but you don't have quite enough to work with. Maybe you can use :path to keep things neat.

* Fixed some typos
* Added absolute index calculations
* Added one more entry to the dynamic table to allow for non-zero index
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

This is excellent. Thanks for adding the one extra reference and the www.example.com.

I didn't check the examples carefully, but I checked a couple and those line up neatly.

@afrind afrind merged commit 7804d57 into master Sep 23, 2020
@afrind afrind deleted the qpack-examples branch September 23, 2020 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-qpack editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QPACK could use some examples
5 participants