From dfa9ab6e04689b99267092e016a91d9254f94cd8 Mon Sep 17 00:00:00 2001 From: jacqueline Date: Tue, 22 Nov 2022 23:10:42 +1100 Subject: [PATCH] template-ify the cbor stuff --- src/audio/chunk.cpp | 43 +++++++++------ src/cbor/cbor_decoder.cpp | 63 +-------------------- src/cbor/include/cbor_decoder.hpp | 92 ++++++++++++++++++++++++++++--- 3 files changed, 112 insertions(+), 86 deletions(-) diff --git a/src/audio/chunk.cpp b/src/audio/chunk.cpp index a8864930..40564069 100644 --- a/src/audio/chunk.cpp +++ b/src/audio/chunk.cpp @@ -10,18 +10,22 @@ namespace audio { /* - * The maximum size that we expect a header to take up. + * The amount of space to allocate for the first chunk's header. After the first + * chunk, we have a more concrete idea of the header's size and can allocate + * space for future headers more compactly. */ -// TODO: tune this. -static const size_t kMaxHeaderSize = 64; +// TODO: measure how big headers tend to be to pick a better value. +static const size_t kInitialHeaderSize = 32; auto WriteChunksToStream(MessageBufferHandle_t *stream, uint8_t *working_buffer, size_t working_buffer_length, std::function callback, TickType_t max_wait) -> EncodeWriteResult { + + size_t header_size = kInitialHeaderSize; while (1) { // First, ask the callback for some data to write. size_t chunk_size = callback( - working_buffer + kMaxHeaderSize, - working_buffer_length - kMaxHeaderSize); + working_buffer + header_size, + working_buffer_length - header_size); if (chunk_size == 0) { // They had nothing for us, so bail out. @@ -31,22 +35,28 @@ auto WriteChunksToStream(MessageBufferHandle_t *stream, uint8_t *working_buffer, // Put together a header. cbor::Encoder encoder(cbor::CONTAINER_ARRAY, 3, working_buffer, working_buffer_length); encoder.WriteUnsigned(TYPE_CHUNK_HEADER); - // Note here that we need to write the offset of the chunk into the header. - // We could be smarter here and write the actual header size, allowing us to - // pack slightly more data into each message, but this is hard so I haven't - // done it. Please make my code better for me. - encoder.WriteUnsigned(kMaxHeaderSize); + encoder.WriteUnsigned(header_size); encoder.WriteUnsigned(chunk_size); - if (encoder.Finish().has_error()) { + + size_t new_header_size = header_size; + cpp::result encoder_res = encoder.Finish(); + if (encoder_res.has_error()) { return CHUNK_ENCODING_ERROR; - }; + } else { + // We can now tune the space to allocate for the header to be closer to + // its actual size. We pad this by 2 bytes to allow extra space for the + // chunk size and header size fields to each spill over into another byte + // each. + new_header_size = encoder_res.value() + 2; + } // Try to write to the buffer. Note the return type here will be either 0 or - // kMaxHeaderSize + chunk_size, as MessageBuffer doesn't allow partial - // writes. + // header_size + chunk_size, as MessageBuffer doesn't allow partial writes. size_t actual_write_size = xMessageBufferSend( - *stream, working_buffer, kMaxHeaderSize + chunk_size, max_wait); + *stream, working_buffer, header_size + chunk_size, max_wait); + + header_size = new_header_size; if (actual_write_size == 0) { // We failed to write in time, so bail out. This is techinically data loss @@ -84,8 +94,7 @@ auto ReadChunksFromStream(MessageBufferHandle_t *stream, uint8_t *working_buffer return CHUNK_STREAM_ENDED; } - // Work the size and position of the chunk (don't assume it's at - // kMaxHeaderSize offset for future-proofing). + // Work the size and position of the chunk. header_length = decoder.ParseUnsigned().value_or(0); chunk_length = decoder.ParseUnsigned().value_or(0); if (decoder.Failed()) { diff --git a/src/cbor/cbor_decoder.cpp b/src/cbor/cbor_decoder.cpp index eb43e163..f0b497b3 100644 --- a/src/cbor/cbor_decoder.cpp +++ b/src/cbor/cbor_decoder.cpp @@ -18,68 +18,6 @@ static auto ArrayDecoder::Create(uint8_t *buffer, size_t buffer_len) -> cpp::res return std::move(decoder); } - -auto ArrayDecoder::ParseString() -> cpp::result { - if (error_ != CborNoError) { - return cpp::fail(error_); - } - - if (!cbor_value_is_byte_string(&it_)) { - error_ = CborErrorIllegalType; - return cpp::fail(error_); - } - uint8_t *buf; size_t len; CborValue new_val; - error_ = cbor_value_dup_byte_string(&it_, &buf, &len, &new_val); - if (error_ != CborNoError) { - return cpp::fail(error_); - } - std::string ret(buf, len); - free(buf); - val_ = new_val; - return ret; -} - -auto ArrayDecoder::ParseUnsigned() -> cpp::result { - if (error_ != CborNoError) { - return cpp::fail(error_); - } - - if (!cbor_value_is_unsigned_integer(&it_)) { - error_ = CborErrorIllegalType; - return cpp::fail(error_); - } - uint64_t ret; - error_ = cbor_value_get_uint64(&it_, &ret); - if (error_ != CborNoError) { - return cpp::fail(error_); - } - error_ = cbor_value_advance(&it_); - if (error_ != CborNoError) { - return cpp::fail(error_); - } - return ret; -} - -auto ArrayDecoder::ParseSigned() -> cpp::result { - if (error_ != CborNoError) { - return cpp::fail(error_); - } - if (!cbor_value_is_unsigned_integer(&it_)) { - error_ = CborErrorIllegalType; - return cpp::fail(error_); - } - uint64_t ret; - error_ = cbor_value_get_uint64(&it_, &ret); - if (error_ != CborNoError) { - return cpp::fail(error_); - } - error_ = cbor_value_advance(&it_); - if (error_ != CborNoError) { - return cpp::fail(error_); - } - return ret; -} - static auto MapDecoder::Create(uint8_t *buffer, size_t buffer_len) -> cpp::result, CborError> { auto decoder = std::make_unique(); cbor_parser_init(buffer, buffer_len, &decoder->parser_, &decoder->root_); @@ -141,6 +79,7 @@ auto MapDecoder::FindSigned(const std::string &key) -> std::optional { return {}; } if (cbor_value_map_find_value(&it_, key.c_str(), &val) != CborNoError) { + // Don't store this as an error; missing keys are fine. return {}; } if (!cbor_value_is_integer(&val)) { diff --git a/src/cbor/include/cbor_decoder.hpp b/src/cbor/include/cbor_decoder.hpp index 249db9cc..39151ca8 100644 --- a/src/cbor/include/cbor_decoder.hpp +++ b/src/cbor/include/cbor_decoder.hpp @@ -1,15 +1,61 @@ #pragma once +#include #include + namespace cbor { + static auto parse_stdstring(CborValue *val, std::string *out) -> CborError { + uint8_t *buf; size_t len; + CborError err = cbor_value_dup_byte_string(val, &buf, &len, NULL); + if (err != CborNoError) { + return err; + } + *out = std::move(std::string(buf, len)); + free(buf); + return err + } + class ArrayDecoder { public: - static auto Create(uint8_t *buffer, size_t buffer_len) -> cpp::result, CborError>; + static auto Create(uint8_t *buffer, size_t buffer_len) + -> cpp::result, CborError>; + + template + auto NextValue() -> cpp::result; - auto ParseString() -> cpp::result; - auto ParseUnsigned() -> cpp::result; - auto ParseSigned() -> cpp::result; + template<> auto NextValue() -> cpp::result { + return NextValue(&cbor_value_is_integer, &cbor_value_get_int); + } + template<> auto NextValue() -> cpp::result { + return NextValue(&cbor_value_is_unsigned_integer, &cbor_value_get_uint64); + } + template<> auto NextValue() -> cpp::result { + return NextValue(&cbor_value_is_byte_string, &parse_stdstring); + } + + template + auto NextValue( + bool(*is_valid)(CborValue*), + CborError(*parse)(CborValue*, T*)) -> cpp::result { + if (error_ != CborNoError) { + return cpp::fail(error_); + } + if (!is_valid(&it_)) { + error_ = CborErrorIllegalType; + return cpp::fail(error_); + } + T ret; + error_ = parse(&it_, &ret); + if (error_ != CborNoError) { + return cpp::fail(error_); + } + error_ = cbor_value_advance(&it_); + if (error_ != CborNoError) { + return cpp::fail(error_); + } + return ret; + } auto Failed() -> CborError { return error_; } @@ -27,9 +73,41 @@ namespace cbor { public: static auto Create(uint8_t *buffer, size_t buffer_len) -> cpp::result, CborError>; - auto FindString(const std::string &key) -> std::optional; - auto FindUnsigned(const std::string &key) -> std::optional; - auto FindSigned(const std::string &key) -> std::optional; + template + auto FindValue(const std::string &key) -> std::optional; + + template<> auto FindValue(const std::string &key) -> std::optional { + return FindValue(key, &cbor_value_is_integer, &cbor_value_get_int); + } + template<> auto FindValue(const std::string &key) -> std::optional { + return FindValue(key, &cbor_value_is_unsigned_integer, &cbor_value_get_uint64); + } + template<> auto FindValue(const std::string &key) -> std::optional { + return FindValue(key, &cbor_value_is_byte_string, &parse_stdstring); + } + + template + auto FindValue( + const std::string &key, + bool(*is_valid)(CborValue*), + CborError(*parse)(CborValue*, T*)) -> std::optional { + if (error_ != CborNoError) { + return {}; + } + if (cbor_value_map_find_value(&it_, key.c_str(), &val) != CborNoError) { + return {}; + } + if (!is_valid(&val)) { + error_ = CborErrorIllegalType; + return {}; + } + T ret; + error_ = parse(&val, &ret); + if (error_ != CborNoError) { + return cpp::fail(error_); + } + return ret; + } auto Failed() -> CborError { return error_; }