Seek more clarity on Largest Reference/Base Index encoding #1403
Conversation
martinthomson
added
the
editorial
draft-ietf-quic-qpack.md
Outdated
|
|
||
| baseIndex = largestReference + deltaBaseIndex | ||
| To save space, Base Index is encoded as a relative to the Base Index using a |
There was a problem hiding this comment.
relative to Largest Reference
There was a problem hiding this comment.
Also, "encoded relative to" rather than "encoded as a relative to". Otherwise, I need to see how we encode second cousins.... 😉
draft-ietf-quic-qpack.md
Outdated
| is: | ||
|
|
||
| ~~~ | ||
| baseIndex = largestReference + sign * deltaBaseIndex |
There was a problem hiding this comment.
While order of operations makes this correct, it might be worth parentheses for clarity.
draft-ietf-quic-qpack.md
Outdated
| baseIndex = largestReference + sign * deltaBaseIndex | ||
| ~~~ | ||
|
|
||
| If the encoder inserted entries to the table while the encoding the header |
There was a problem hiding this comment.
I know I wrote the previous iteration of this text, but this is baking in the assumption that Base Index was fixed before encoding began (i.e. single-pass encoding). Should that be called out?
draft-ietf-quic-qpack.md
Outdated
|
|
||
| A header block that does not reference the dynamic table can use any value for | ||
| Base Index and Largest Reference; sending zero values for both fields is the | ||
| most efficient. |
There was a problem hiding this comment.
You can use any value for Base Index, and zero is the most efficient. I'd argue that zero for Largest Reference is probably a SHOULD, given that it has a defined meaning and isn't just used for relative indexing.
There was a problem hiding this comment.
Yeah, I considered that, but it's not a SHOULD really, especially since we have defined Largest Reference already (it's the largest absolute index in the block). I'll adjust and let you decide if you want to merge.
There was a problem hiding this comment.
Having no references and the value not being zero is a violation of what that field means. More generally, I think a decoder that wanted to have an error case for "You didn't actually reference the Largest Reference" would be well within its rights, and we might want to either mention that and define the error code or explicitly prohibit that.
Alan's doc, his call.
There was a problem hiding this comment.
Rather than bog this PR down, let's fix that separately.
martinthomson
changed the title
draft-ietf-quic-qpack.md
Outdated
| baseIndex = largestReference + (sign * deltaBaseIndex) | ||
| ~~~ | ||
|
|
||
| An encoder is expected to determine the absolute value of Base Index before |
draft-ietf-quic-qpack.md
Outdated
| less than Base Index, so deltaBaseIndex will be positive and encoded with S=0. | ||
| When Largest Reference and Base Index are equal, deltaBaseIndex is 0 and encoded | ||
| with S=0. | ||
| That is, after turning a sign bit of 1 to -1 and 0 to 1, the absolute Base Index |
There was a problem hiding this comment.
I find the language about turning a sign bit somewhat confusing.
Is it more clear to explain using a conditional?
if sign:
baseIndex = largestReference - deltaBaseIndex
else:
baseIndex = largestReference + deltaBaseIndex
Or index into an array?
signMultiplier = [ 1, -1 ]
baeIndex = largestReference + signMultipler[ sign ] * deltaBaseIndex
There was a problem hiding this comment.
Yes, this is always hard to explain, but the conditional is nice. Verbose FTW.
draft-ietf-quic-qpack.md
Outdated
| An encoder is expected to determine the absolute value of Base Index before | ||
| encoding a header block. If the encoder inserted entries in the dynamic table | ||
| while encoding the header block, Largest Reference will be greater than Base | ||
| Index, so the encoded difference is negative and the sign bit will be 1. If the |
There was a problem hiding this comment.
nit: is vs. will be - can it be consistent?
draft-ietf-quic-qpack.md
Outdated
| baseIndex = largestReference + deltaBaseIndex | ||
| To save space, Base Index is encoded relative to Largest Reference using a | ||
| one-bit sign and the `Delta Base Index` value. A sign bit of 0 indicates that | ||
| the Base Index has a greater absolute index than the Largest Reference; the |
There was a problem hiding this comment.
greater than or equal to, technically. We don't mention the zero case at all until the bottom.
There was a problem hiding this comment.
Well the sentence says that the encoder added entries. I don't think that it's a problem to not mention the zero case.
There was a problem hiding this comment.
I don't see where this sentence says it added entries - are you referring to somewhere else? I think Mike's right here - if the Base Index is equal to LR then the sign bit will be 0. The original text had that, and it's included on lines 578-9 below. Is it clearer if we bring it back up here?
I agree we don't need to mention the case where they are both 0 here.
draft-ietf-quic-qpack.md
Outdated
| baseIndex = largestReference + deltaBaseIndex | ||
| To save space, Base Index is encoded relative to Largest Reference using a | ||
| one-bit sign and the `Delta Base Index` value. A sign bit of 0 indicates that | ||
| the Base Index has a greater absolute index than the Largest Reference; the |
There was a problem hiding this comment.
I don't see where this sentence says it added entries - are you referring to somewhere else? I think Mike's right here - if the Base Index is equal to LR then the sign bit will be 0. The original text had that, and it's included on lines 578-9 below. Is it clearer if we bring it back up here?
I agree we don't need to mention the case where they are both 0 here.
draft-ietf-quic-qpack.md
Outdated
| the Base Index has a greater absolute index than the Largest Reference; the | ||
| value of Delta Base Index is added to the Largest Reference to determine the | ||
| absolute value of the Base Index. A sign bit of 1 indicates that the Base Index | ||
| is smaller than the Base Index. That is: |
There was a problem hiding this comment.
Base Index is smaller than the Largest Reference
I made some changes in #1363 to the explanation of how these are encoded because I found the explanation baffling. Here's that text, without the swap of the two values.
Now that I've read this 100 times, I think that it doesn't change anything material. It does have one change: making sign=1, delta=0 a decoder error. We could reclaim a bit by making the formula more complex, but that's a hard trade-off.