Clean up HpackVarintEncoder interface. Turns out no caller makes use of the incremental encoding feature. gfe-relnote: No functional change; not flag protected. PiperOrigin-RevId: 254798717 Change-Id: I07c0519be47321f25ec006061fda39791202c348
diff --git a/http2/hpack/tools/hpack_block_builder.cc b/http2/hpack/tools/hpack_block_builder.cc index 5a79f78..be62346 100644 --- a/http2/hpack/tools/hpack_block_builder.cc +++ b/http2/hpack/tools/hpack_block_builder.cc
@@ -18,20 +18,7 @@ EXPECT_LE(prefix_length, 8); HpackVarintEncoder varint_encoder; - - unsigned char c = - varint_encoder.StartEncoding(high_bits, prefix_length, varint); - buffer_.push_back(c); - - if (!varint_encoder.IsEncodingInProgress()) { - return; - } - - // After the prefix, at most 63 bits can remain to be encoded. - // Each octet holds 7 bits, so at most 9 octets are necessary. - // TODO(bnc): Move this into a constant in HpackVarintEncoder. - varint_encoder.ResumeEncoding(/* max_encoded_bytes = */ 10, &buffer_); - DCHECK(!varint_encoder.IsEncodingInProgress()); + varint_encoder.Encode(high_bits, prefix_length, varint, &buffer_); } void HpackBlockBuilder::AppendEntryTypeAndVarint(HpackEntryType entry_type,
diff --git a/http2/hpack/varint/hpack_varint_encoder.cc b/http2/hpack/varint/hpack_varint_encoder.cc index 2e7ad3e..f69c734 100644 --- a/http2/hpack/varint/hpack_varint_encoder.cc +++ b/http2/hpack/varint/hpack_varint_encoder.cc
@@ -4,6 +4,8 @@ #include "net/third_party/quiche/src/http2/hpack/varint/hpack_varint_encoder.h" +#include <limits> + #include "net/third_party/quiche/src/http2/platform/api/http2_logging.h" namespace http2 { @@ -11,6 +13,20 @@ HpackVarintEncoder::HpackVarintEncoder() : varint_(0), encoding_in_progress_(false) {} +void HpackVarintEncoder::Encode(uint8_t high_bits, + uint8_t prefix_length, + uint64_t varint, + Http2String* output) { + unsigned char first_byte = StartEncoding(high_bits, prefix_length, varint); + output->push_back(first_byte); + if (!IsEncodingInProgress()) { + return; + } + + ResumeEncoding(std::numeric_limits<size_t>::max(), output); + DCHECK(!IsEncodingInProgress()); +} + unsigned char HpackVarintEncoder::StartEncoding(uint8_t high_bits, uint8_t prefix_length, uint64_t varint) {
diff --git a/http2/hpack/varint/hpack_varint_encoder.h b/http2/hpack/varint/hpack_varint_encoder.h index 68f7474..a5a9cd8 100644 --- a/http2/hpack/varint/hpack_varint_encoder.h +++ b/http2/hpack/varint/hpack_varint_encoder.h
@@ -5,6 +5,7 @@ #ifndef QUICHE_HTTP2_HPACK_VARINT_HPACK_VARINT_ENCODER_H_ #define QUICHE_HTTP2_HPACK_VARINT_HPACK_VARINT_ENCODER_H_ +#include <cstddef> #include <cstdint> #include "net/third_party/quiche/src/http2/platform/api/http2_export.h" @@ -19,6 +20,16 @@ public: HpackVarintEncoder(); + // Encode |varint|, appending encoded data to |*output|. Appends between 1 + // and 11 bytes in total. Must not be called when another encoding task + // previously started by StartEncoding() is still in progress. No encoding + // will be in progress after this method finishes. + void Encode(uint8_t high_bits, + uint8_t prefix_length, + uint64_t varint, + Http2String* output); + + private: // Start encoding an integer. Return the first encoded byte (composed of // optional high bits and 1 to 8 bit long prefix). It is possible that this // completes the encoding. Must not be called when previously started @@ -36,7 +47,6 @@ // Returns true if encoding an integer has started and is not completed yet. bool IsEncodingInProgress() const; - private: // The original integer shifted to the right by the number of bits already // encoded. The lower bits shifted away have already been encoded, and // |varint_| has the higher bits that remain to be encoded.
diff --git a/http2/hpack/varint/hpack_varint_encoder_test.cc b/http2/hpack/varint/hpack_varint_encoder_test.cc index a6043a0..96ade85 100644 --- a/http2/hpack/varint/hpack_varint_encoder_test.cc +++ b/http2/hpack/varint/hpack_varint_encoder_test.cc
@@ -13,12 +13,6 @@ namespace test { namespace { -// Freshly constructed encoder is not in the process of encoding. -TEST(HpackVarintEncoderTest, Done) { - HpackVarintEncoder varint_encoder; - EXPECT_FALSE(varint_encoder.IsEncodingInProgress()); -} - struct { uint8_t high_bits; uint8_t prefix_length; @@ -39,11 +33,12 @@ HpackVarintEncoder varint_encoder; for (size_t i = 0; i < HTTP2_ARRAYSIZE(kShortTestData); ++i) { - EXPECT_EQ(kShortTestData[i].expected_encoding, - varint_encoder.StartEncoding(kShortTestData[i].high_bits, - kShortTestData[i].prefix_length, - kShortTestData[i].value)); - EXPECT_FALSE(varint_encoder.IsEncodingInProgress()); + Http2String output; + varint_encoder.Encode(kShortTestData[i].high_bits, + kShortTestData[i].prefix_length, + kShortTestData[i].value, &output); + ASSERT_EQ(1u, output.size()); + EXPECT_EQ(kShortTestData[i].expected_encoding, output[0]); } } @@ -111,34 +106,17 @@ // Test encoding byte by byte, also test encoding in // a single ResumeEncoding() call. - for (bool byte_by_byte : {true, false}) { for (size_t i = 0; i < HTTP2_ARRAYSIZE(kLongTestData); ++i) { Http2String expected_encoding = Http2HexDecode(kLongTestData[i].expected_encoding); - ASSERT_FALSE(expected_encoding.empty()); - - EXPECT_EQ(static_cast<unsigned char>(expected_encoding[0]), - varint_encoder.StartEncoding(kLongTestData[i].high_bits, - kLongTestData[i].prefix_length, - kLongTestData[i].value)); - EXPECT_TRUE(varint_encoder.IsEncodingInProgress()); Http2String output; - if (byte_by_byte) { - while (varint_encoder.IsEncodingInProgress()) { - EXPECT_EQ(1u, varint_encoder.ResumeEncoding(1, &output)); - } - } else { - // TODO(bnc): Factor out maximum number of extension bytes into a - // constant in HpackVarintEncoder. - EXPECT_EQ(expected_encoding.size() - 1, - varint_encoder.ResumeEncoding(10, &output)); - EXPECT_FALSE(varint_encoder.IsEncodingInProgress()); - } - EXPECT_EQ(expected_encoding.size() - 1, output.size()); - EXPECT_EQ(expected_encoding.substr(1), output); + varint_encoder.Encode(kLongTestData[i].high_bits, + kLongTestData[i].prefix_length, + kLongTestData[i].value, &output); + + EXPECT_EQ(expected_encoding, output); } - } } struct { @@ -158,21 +136,33 @@ HpackVarintEncoder varint_encoder; for (size_t i = 0; i < HTTP2_ARRAYSIZE(kLastByteIsZeroTestData); ++i) { - EXPECT_EQ( - kLastByteIsZeroTestData[i].expected_encoding_first_byte, - varint_encoder.StartEncoding(kLastByteIsZeroTestData[i].high_bits, - kLastByteIsZeroTestData[i].prefix_length, - kLastByteIsZeroTestData[i].value)); - EXPECT_TRUE(varint_encoder.IsEncodingInProgress()); - Http2String output; - EXPECT_EQ(1u, varint_encoder.ResumeEncoding(1, &output)); - ASSERT_EQ(1u, output.size()); - EXPECT_EQ(0b00000000, output[0]); - EXPECT_FALSE(varint_encoder.IsEncodingInProgress()); + varint_encoder.Encode(kLastByteIsZeroTestData[i].high_bits, + kLastByteIsZeroTestData[i].prefix_length, + kLastByteIsZeroTestData[i].value, &output); + ASSERT_EQ(2u, output.size()); + EXPECT_EQ(kLastByteIsZeroTestData[i].expected_encoding_first_byte, + output[0]); + EXPECT_EQ(0b00000000, output[1]); } } +// Test that encoder appends correctly to non-empty string. +TEST(HpackVarintEncoderTest, Append) { + HpackVarintEncoder varint_encoder; + Http2String output("foo"); + EXPECT_EQ(Http2HexDecode("666f6f"), output); + + varint_encoder.Encode(0b10011000, 3, 103, &output); + EXPECT_EQ(Http2HexDecode("666f6f9f60"), output); + + varint_encoder.Encode(0b10100000, 5, 8, &output); + EXPECT_EQ(Http2HexDecode("666f6f9f60a8"), output); + + varint_encoder.Encode(0b10011000, 3, 202147110, &output); + EXPECT_EQ(Http2HexDecode("666f6f9f60a89f9f8ab260"), output); +} + } // namespace } // namespace test } // namespace http2
diff --git a/quic/core/qpack/qpack_constants.h b/quic/core/qpack/qpack_constants.h index 2812e63..35e4d56 100644 --- a/quic/core/qpack/qpack_constants.h +++ b/quic/core/qpack/qpack_constants.h
@@ -77,11 +77,6 @@ // Every possible input must match exactly one instruction. using QpackLanguage = std::vector<const QpackInstruction*>; -// TODO(bnc): Move this into HpackVarintEncoder. -// The integer encoder can encode up to 2^64-1, which can take up to 10 bytes -// (each carrying 7 bits) after the prefix. -const uint8_t kMaxExtensionBytesForVarintEncoding = 10; - // Wire format defined in // https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#rfc.section.5
diff --git a/quic/core/qpack/qpack_instruction_encoder.cc b/quic/core/qpack/qpack_instruction_encoder.cc index 400a828..b2acdbd 100644 --- a/quic/core/qpack/qpack_instruction_encoder.cc +++ b/quic/core/qpack/qpack_instruction_encoder.cc
@@ -42,11 +42,8 @@ case State::kSbit: DoStaticBit(); break; - case State::kVarintStart: - DoVarintStart(output); - break; - case State::kVarintResume: - DoVarintResume(output); + case State::kVarintEncode: + DoVarintEncode(output); break; case State::kStartString: DoStartString(); @@ -73,7 +70,7 @@ return; case QpackInstructionFieldType::kVarint: case QpackInstructionFieldType::kVarint2: - state_ = State::kVarintStart; + state_ = State::kVarintEncode; return; case QpackInstructionFieldType::kName: case QpackInstructionFieldType::kValue: @@ -95,13 +92,11 @@ state_ = State::kStartField; } -void QpackInstructionEncoder::DoVarintStart(std::string* output) { +void QpackInstructionEncoder::DoVarintEncode(std::string* output) { DCHECK(field_->type == QpackInstructionFieldType::kVarint || field_->type == QpackInstructionFieldType::kVarint2 || field_->type == QpackInstructionFieldType::kName || field_->type == QpackInstructionFieldType::kValue); - DCHECK(!varint_encoder_.IsEncodingInProgress()); - uint64_t integer_to_encode; switch (field_->type) { case QpackInstructionFieldType::kVarint: @@ -115,35 +110,9 @@ break; } - output->push_back( - varint_encoder_.StartEncoding(byte_, field_->param, integer_to_encode)); + varint_encoder_.Encode(byte_, field_->param, integer_to_encode, output); byte_ = 0; - if (varint_encoder_.IsEncodingInProgress()) { - state_ = State::kVarintResume; - return; - } - - if (field_->type == QpackInstructionFieldType::kVarint || - field_->type == QpackInstructionFieldType::kVarint2) { - ++field_; - state_ = State::kStartField; - return; - } - - state_ = State::kWriteString; -} - -void QpackInstructionEncoder::DoVarintResume(std::string* output) { - DCHECK(field_->type == QpackInstructionFieldType::kVarint || - field_->type == QpackInstructionFieldType::kVarint2 || - field_->type == QpackInstructionFieldType::kName || - field_->type == QpackInstructionFieldType::kValue); - DCHECK(varint_encoder_.IsEncodingInProgress()); - - varint_encoder_.ResumeEncoding(std::numeric_limits<size_t>::max(), output); - DCHECK(!varint_encoder_.IsEncodingInProgress()); - if (field_->type == QpackInstructionFieldType::kVarint || field_->type == QpackInstructionFieldType::kVarint2) { ++field_; @@ -169,7 +138,7 @@ string_to_write_ = huffman_encoded_string_; } - state_ = State::kVarintStart; + state_ = State::kVarintEncode; } void QpackInstructionEncoder::DoWriteString(std::string* output) {
diff --git a/quic/core/qpack/qpack_instruction_encoder.h b/quic/core/qpack/qpack_instruction_encoder.h index ae37b9e..a831a07 100644 --- a/quic/core/qpack/qpack_instruction_encoder.h +++ b/quic/core/qpack/qpack_instruction_encoder.h
@@ -43,11 +43,9 @@ kStartField, // Write static bit to |byte_|. kSbit, - // Start encoding an integer (|varint_| or |varint2_| or string length) with - // a prefix, using |byte_| for the high bits. - kVarintStart, - // Resume encoding an integer. - kVarintResume, + // Encode an integer (|varint_| or |varint2_| or string length) with a + // prefix, using |byte_| for the high bits. + kVarintEncode, // Determine if Huffman encoding should be used for |name_| or |value_|, set // up |name_| or |value_| and |huffman_encoded_string_| accordingly, and // write the Huffman bit to |byte_|. @@ -61,8 +59,7 @@ void DoOpcode(); void DoStartField(); void DoStaticBit(); - void DoVarintStart(std::string* output); - void DoVarintResume(std::string* output); + void DoVarintEncode(std::string* output); void DoStartString(); void DoWriteString(std::string* output);