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

Do we need two ways to represent Base Index? #2002

Closed
kazuho opened this issue Nov 14, 2018 · 7 comments
Closed

Do we need two ways to represent Base Index? #2002

kazuho opened this issue Nov 14, 2018 · 7 comments
Labels
-qpack design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@kazuho
Copy link
Member

kazuho commented Nov 14, 2018

At the moment, there are two ways to Base Index identical to Largest Reference: use (S, Delta Base Index) = (1, 0) or (0, 0).

Would it be better to add one offset when the sign bit is set, i.e. change

   if sign == 0:
      baseIndex = largestReference + deltaBaseIndex
   else:
      baseIndex = largestReference - deltaBaseIndex

to

   if sign == 0:
      baseIndex = largestReference + deltaBaseIndex
   else:
      baseIndex = largestReference - deltaBaseIndex - 1

?

I ask this because IMO we have an inconsistency; Post Base Index already has this type of offset so that a Post Base Index of zero does not become equal to Relative Index of zero.

EDIT. Inserted "already" to avoid confusion.

@martinthomson
Copy link
Member

martinthomson commented Nov 14, 2018

Hmm, diagrams suggest that we already have the latter form. My code might not be interoperable, but it subtracts the extra 1 already.

Oh, have I mentioned recently that I hate post-base indexing? Could we perhaps reconsider its addition and have any single-pass encoder lose any opportunity to use insertions it makes after committing to a largest reference? Maybe I should open another issue.

@kazuho
Copy link
Member Author

kazuho commented Nov 14, 2018

@martinthomson

Hmm, diagrams suggest that we already have the latter form. My code might not be interoperable, but it subtracts the extra 1 already.

The ones with the diagram; i.e. Post Base Index that appears in the request and push streams is OK.

However, the Delta Base Index calculated in section 5.4.1 is missing the "subtract the extra 1." That is what I am complaining.

Oh, have I mentioned recently that I hate post-base indexing? Could we perhaps reconsider its addition and have any single-pass encoder lose any opportunity to use insertions it makes after committing to a largest reference? Maybe I should open another issue.

I think it might be sensible to open a new issue for the purpose.

Even though I have an implementation that emits post-base indexing (for the time being), I agree that it is an unnecessary complexity at least in the current form. Because in the current form, even if you emit the headers in single pass, you still need to adjust the Header Data Prefix afterwards so that Largest Reference will include the correct value. To put it another way, it is impossible to write a "streaming encoder."

Therefore, my preference goes to either removing post-base indexing, or to removing "Largest Reference" to allow true one-pass encoding (i.e. streaming encoding). In the latter case, every decoder can / will check if an unresolvable reference is included in each of the header field rather than relying on "Largest Reference."

@dtikhonov
Copy link
Member

At the moment, there are two ways to Base Index identical to Largest Reference: use (S, Delta Base Index) = (1, 0) or (0, 0).

This is not true. The draft states:

A sign bit set to 1 when the Delta Base Index is 0 MUST be treated as a decoder error.

@dtikhonov
Copy link
Member

Oh, have I mentioned recently that I hate post-base indexing?

Why do you hate it?

Could we perhaps reconsider its addition and have any single-pass encoder lose any opportunity to use insertions it makes after committing to a largest reference? Maybe I should open another issue.

I am against both reconsidering and removing post-base indexing: the former because we already had this discussion and came to a consensus (however "rough") and the latter is because this would penalize already-extant implementations that use this feature profitably.

@kazuho
Copy link
Member Author

kazuho commented Nov 14, 2018

@dtikhonov

At the moment, there are two ways to Base Index identical to Largest Reference: use (S, Delta Base Index) = (1, 0) or (0, 0).

This is not true. The draft states:

A sign bit set to 1 when the Delta Base Index is 0 MUST be treated as a decoder error.

Oh thank you for pointing that out. I missed that.

Though I still think that we have an inconsistency in the design. As pointed out, post-base indexing in the request and push streams do not call index of zero to be an error. Instead, they adjust the offset so that a post-base index of zero do not overlap with a non-post-base index with zero. Should we follow the pattern for the delta base index as well?

@dtikhonov
Copy link
Member

I agree that inconsistency exists.

kazuho added a commit to kazuho/base-drafts that referenced this issue Nov 14, 2018
As discussed in quicwg#2002, there are two ways to encode Delta Base
Index of zero at the wire level; i.e. (sign-bit, delta-base-index)
= (0, 0) and (1, 0).

To avoid having multiple ways to represent one value, we prohibit
the latter form from being used.  A receiver is required to raise
an error when it sees the latter.

This is not only an unnecessary complexity but also contradicts
from the approach we use for Post-Base Indexes.  In case of Post-
Base Indexes, we introduce an offset of one so that the Post-Base
Index of zero and a non-Post-Base Index of zero do not overlap.

The commit adopts the approach to Delta Base Index; giving us
consistency in the design and also removing an error check at the
cost of requiring one subtraction.
@afrind afrind added the design An issue that affects the design of the protocol; resolution requires consensus. label Nov 27, 2018
afrind pushed a commit that referenced this issue Dec 4, 2018
As discussed in #2002, there are two ways to encode Delta Base
Index of zero at the wire level; i.e. (sign-bit, delta-base-index)
= (0, 0) and (1, 0).

To avoid having multiple ways to represent one value, we prohibit
the latter form from being used.  A receiver is required to raise
an error when it sees the latter.

This is not only an unnecessary complexity but also contradicts
from the approach we use for Post-Base Indexes.  In case of Post-
Base Indexes, we introduce an offset of one so that the Post-Base
Index of zero and a non-Post-Base Index of zero do not overlap.

The commit adopts the approach to Delta Base Index; giving us
consistency in the design and also removing an error check at the
cost of requiring one subtraction.
@afrind
Copy link
Contributor

afrind commented Dec 11, 2018

Closed by #2005

@afrind afrind closed this as completed Dec 11, 2018
@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-qpack design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
None yet
Development

No branches or pull requests

5 participants