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

remove RIC from Relative Index in Header Block figure; add Base value to two figures #2935

Merged
merged 5 commits into from
Oct 22, 2019
Merged

remove RIC from Relative Index in Header Block figure; add Base value to two figures #2935

merged 5 commits into from
Oct 22, 2019

Conversation

bencebeky
Copy link
Contributor

In my mind Base is an index, so it should point to an entry (relative index 0 means the entry with absolute index one fewer than Base, post-base index 0 means the same entry as Base). Required Insert Count is a counter, not an index, so it does not belong on figures.

@ianswett ianswett added the -qpack label Aug 2, 2019
@MikeBishop
Copy link
Contributor

@afrind
Copy link
Contributor

afrind commented Aug 13, 2019

Agree we should remove the arrow from count. I guess there's two minds on where array indices point. I think I inherited the current style. HPACK only has the 'insertion point' and 'dropping point' arrows, so sidesteps the issue altogether.

I'm inlined to say it's more clear as is, with Base pointing on the line between what was in the table when encoding started and the Post Base entries.

@bencebeky
Copy link
Contributor Author

While I understand the argument outlined in the blog post @MikeBishop linked above, and would not be unhappy if that was the widespread convention, I am worried that this is not the mental model that most engineers share. I think the index--element bijection is the general convention, and deviating from that might cause confusion.

A particular source of confusion arising in this situation even for people used to the "indices point between elments" convention is that on the two figures in question, indices increase to the left.
Therefore the element with a given index is not the one on the right of the arrow, but the one on the left, breaking the rule of thumb mentioned in the blog post.

@afrind
Copy link
Contributor

afrind commented Oct 15, 2019

This confusion comes from when we changed Absolute Index to start from 0, and Base Index became just "Base". What is Base now anyways? Base is now a Count, and perhaps we should rename it to Base Count everywhere.

Because Base is a Count and not an Index, it shouldn't really point anywhere in the diagram. Maybe we should remove the arrow entirely, and just say, Base = n - 2

If we really have to have an arrow, then where should it point when Base = Required Insert Count?

Required Insert Count = 2
Base = 2

Base
  |
  v 
  | 1 | 0 |   # Absolute
  | 0 | 1 |   # Relative 

or

Base
  |
  v 
    | 1 | 0 |   # Absolute
    | 0 | 1 |   # Relative 

Or when Base is 0 ?

Required Insert Count = 2
Base = 0

      Base
        |
        v 
  | 1 | 0 |   # Absolute
  | 0 | 1 |   # Relative 

or

        Base
          |
          v 
  | 1 | 0 |   # Absolute
  | 0 | 1 |   # Relative 

@bencebeky
Copy link
Contributor Author

Only the author of this PR and editors of the spec ever commented on this editorial issue. This indicates to me that this issue is insignificant. I encourage any of the editors to just make a decision on where Base should point or if it should appear on the drawing at all. I'm happy to implement that so that we can cross this PR off the list.

If the decision is that Base should point between two elements, or that it should be removed from the drawing, then I suggest adding text specifying that Base = n-2.

By the way it just occurred to me that RIC does not belong to this diagram at all. In fact it is possible that a header block does not reference the most recently inserted element, in which case RIC has a different value than currently indicated.

In my mental model Base is neither an index nor a count. It is an integer that the encoder sends to the decoder, which is then used for arithmetics described in the spec to convert between absolute and relative indices. I'm not convinced that renaming it to Base Count would increase clarity.

@afrind
Copy link
Contributor

afrind commented Oct 16, 2019

In a one-pass encoder, Base is the Insert Count when the encoder begins encoding the block. In a two-pass encoder, I assume most folks set Base = RIC. So I think it logically is a count, but lets put aside whether to rename it since that's not happening in this PR.

For now, I say remove RIC from the figure, but leave the arrow where it is.

@bencebeky
Copy link
Contributor Author

In a one-pass encoder, Base is the Insert Count when the encoder begins encoding the block. In a two-pass encoder, I assume most folks set Base = RIC. So I think it logically is a count, but lets put aside whether to rename it since that's not happening in this PR.

For now, I say remove RIC from the figure, but leave the arrow where it is.

Sounds good to me. Let me add text specifying the Base value because it is not clear for those not familiar with the "indices point between elements" mental model or would be confused by the element with a given index being the one not on the right of the arrow but on the left.

@bencebeky bencebeky changed the title QPACK [editorial] update RIC and Base on figures remove RIC from Relative Index in Header Block figure; add Base value to two figures Oct 17, 2019
@afrind afrind merged commit caf1104 into quicwg:master Oct 22, 2019
@bencebeky bencebeky deleted the bencebeky-qpack-base branch October 22, 2019 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants