| Message ID | 20210507113728.14037-2-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | df36fb4abb37c216d1eaf60a1772047b5205d0b7 |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for the patch. On Fri, May 07, 2021 at 12:37:27PM +0100, David Plowman wrote: > Improve MdParser::Parse() by taking a Span, pointing to const data > that it should not change, as its input buffer. Looks good to me. I may have kept the variables named 'data' instead of 'buffer', but it doesn't matter much. > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/raspberrypi/cam_helper_imx219.cpp | 8 ++++---- > src/ipa/raspberrypi/cam_helper_imx477.cpp | 8 ++++---- > src/ipa/raspberrypi/md_parser.cpp | 23 ++++++++++++----------- > src/ipa/raspberrypi/md_parser.hpp | 20 ++++++++------------ > 4 files changed, 28 insertions(+), 31 deletions(-) > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp > index 566ecbca..e550fba6 100644 > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp > @@ -29,7 +29,7 @@ class MdParserImx219 : public MdParserSmia > { > public: > MdParserImx219(); > - Status Parse(void *data) override; > + Status Parse(libcamera::Span<const uint8_t> buffer) override; > Status GetExposureLines(unsigned int &lines) override; > Status GetGainCode(unsigned int &gain_code) override; > private: > @@ -118,7 +118,7 @@ MdParserImx219::MdParserImx219() > reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1; > } > > -MdParser::Status MdParserImx219::Parse(void *data) > +MdParser::Status MdParserImx219::Parse(libcamera::Span<const uint8_t> buffer) > { > bool try_again = false; > > @@ -132,7 +132,7 @@ MdParser::Status MdParserImx219::Parse(void *data) > /* Need to be ordered */ > uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG }; > reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1; > - int ret = static_cast<int>(findRegs(static_cast<uint8_t *>(data), > + int ret = static_cast<int>(findRegs(buffer, > regs, reg_offsets_, 3)); > /* > * > 0 means "worked partially but parse again next time", > @@ -148,7 +148,7 @@ MdParser::Status MdParserImx219::Parse(void *data) > if (reg_offsets_[i] == -1) > continue; > > - reg_values_[i] = static_cast<uint8_t *>(data)[reg_offsets_[i]]; > + reg_values_[i] = buffer[reg_offsets_[i]]; > } > > /* Re-parse next time if we were unhappy in some way. */ > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp > index 73a5ca7d..a4a58c15 100644 > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp > @@ -21,7 +21,7 @@ class MdParserImx477 : public MdParserSmia > { > public: > MdParserImx477(); > - Status Parse(void *data) override; > + Status Parse(libcamera::Span<const uint8_t> buffer) override; > Status GetExposureLines(unsigned int &lines) override; > Status GetGainCode(unsigned int &gain_code) override; > private: > @@ -107,7 +107,7 @@ MdParserImx477::MdParserImx477() > reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1; > } > > -MdParser::Status MdParserImx477::Parse(void *data) > +MdParser::Status MdParserImx477::Parse(libcamera::Span<const uint8_t> buffer) > { > bool try_again = false; > > @@ -126,7 +126,7 @@ MdParser::Status MdParserImx477::Parse(void *data) > GAINLO_REG > }; > reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1; > - int ret = static_cast<int>(findRegs(static_cast<uint8_t *>(data), > + int ret = static_cast<int>(findRegs(buffer, > regs, reg_offsets_, 4)); > /* > * > 0 means "worked partially but parse again next time", > @@ -142,7 +142,7 @@ MdParser::Status MdParserImx477::Parse(void *data) > if (reg_offsets_[i] == -1) > continue; > > - reg_values_[i] = static_cast<uint8_t *>(data)[reg_offsets_[i]]; > + reg_values_[i] = buffer[reg_offsets_[i]]; > } > > /* Re-parse next time if we were unhappy in some way. */ > diff --git a/src/ipa/raspberrypi/md_parser.cpp b/src/ipa/raspberrypi/md_parser.cpp > index d82c102c..852a1d34 100644 > --- a/src/ipa/raspberrypi/md_parser.cpp > +++ b/src/ipa/raspberrypi/md_parser.cpp > @@ -27,12 +27,13 @@ using namespace RPiController; > #define REG_VALUE 0x5a > #define REG_SKIP 0x55 > > -MdParserSmia::ParseStatus MdParserSmia::findRegs(unsigned char *data, > +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer, > uint32_t regs[], int offsets[], > unsigned int num_regs) > { > assert(num_regs > 0); > - if (data[0] != LINE_START) > + > + if (buffer[0] != LINE_START) > return NO_LINE_START; > > unsigned int current_offset = 1; // after the LINE_START > @@ -40,15 +41,15 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(unsigned char *data, > unsigned int reg_num = 0, first_reg = 0; > ParseStatus retcode = PARSE_OK; > while (1) { > - int tag = data[current_offset++]; > + int tag = buffer[current_offset++]; > if ((bits_per_pixel_ == 10 && > (current_offset + 1 - current_line_start) % 5 == 0) || > (bits_per_pixel_ == 12 && > (current_offset + 1 - current_line_start) % 3 == 0)) { > - if (data[current_offset++] != REG_SKIP) > + if (buffer[current_offset++] != REG_SKIP) > return BAD_DUMMY; > } > - int data_byte = data[current_offset++]; > + int data_byte = buffer[current_offset++]; > //printf("Offset %u, tag 0x%02x data_byte 0x%02x\n", current_offset-1, tag, data_byte); > if (tag == LINE_END_TAG) { > if (data_byte != LINE_END_TAG) > @@ -59,18 +60,18 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(unsigned char *data, > current_offset = > current_line_start + line_length_bytes_; > // Require whole line to be in the buffer (if buffer size set). > - if (buffer_size_bytes_ && > + if (buffer.size() && > current_offset + line_length_bytes_ > > - buffer_size_bytes_) > + buffer.size()) > return MISSING_REGS; > - if (data[current_offset] != LINE_START) > + if (buffer[current_offset] != LINE_START) > return NO_LINE_START; > } else { > // allow a zero line length to mean "hunt for the next line" > - while (data[current_offset] != LINE_START && > - current_offset < buffer_size_bytes_) > + while (buffer[current_offset] != LINE_START && > + current_offset < buffer.size()) > current_offset++; > - if (current_offset == buffer_size_bytes_) > + if (current_offset == buffer.size()) > return NO_LINE_START; > } > // inc current_offset to after LINE_START > diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp > index 926f676e..8e22b1d7 100644 > --- a/src/ipa/raspberrypi/md_parser.hpp > +++ b/src/ipa/raspberrypi/md_parser.hpp > @@ -8,6 +8,8 @@ > > #include <stdint.h> > > +#include <libcamera/span.h> > + > /* Camera metadata parser class. Usage as shown below. > > Setup: > @@ -21,17 +23,15 @@ parser->SetBitsPerPixel(bpp); > parser->SetLineLengthBytes(pitch); > parser->SetNumLines(2); > > -Note 1: if you don't know how many lines there are, you can use SetBufferSize > -instead to limit the total buffer size. > +Note 1: if you don't know how many lines there are, the size of the input > +buffer is used as a limit instead. > > Note 2: if you don't know the line length, you can leave the line length unset > -(or set to zero) and the parser will hunt for the line start instead. In this > -case SetBufferSize *must* be used so that the parser won't run off the end of > -the buffer. > +(or set to zero) and the parser will hunt for the line start instead. > > Then on every frame: > > -if (parser->Parse(data) != MdParser::OK) > +if (parser->Parse(buffer) != MdParser::OK) > much badness; > unsigned int exposure_lines, gain_code > if (parser->GetExposureLines(exposure_lines) != MdParser::OK) > @@ -75,11 +75,7 @@ public: > { > line_length_bytes_ = num_bytes; > } > - void SetBufferSize(unsigned int num_bytes) > - { > - buffer_size_bytes_ = num_bytes; > - } > - virtual Status Parse(void *data) = 0; > + virtual Status Parse(libcamera::Span<const uint8_t> buffer) = 0; > virtual Status GetExposureLines(unsigned int &lines) = 0; > virtual Status GetGainCode(unsigned int &gain_code) = 0; > > @@ -116,7 +112,7 @@ protected: > BAD_LINE_END = -4, > BAD_PADDING = -5 > }; > - ParseStatus findRegs(unsigned char *data, uint32_t regs[], > + ParseStatus findRegs(libcamera::Span<const uint8_t> buffer, uint32_t regs[], > int offsets[], unsigned int num_regs); > }; >
diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp index 566ecbca..e550fba6 100644 --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp @@ -29,7 +29,7 @@ class MdParserImx219 : public MdParserSmia { public: MdParserImx219(); - Status Parse(void *data) override; + Status Parse(libcamera::Span<const uint8_t> buffer) override; Status GetExposureLines(unsigned int &lines) override; Status GetGainCode(unsigned int &gain_code) override; private: @@ -118,7 +118,7 @@ MdParserImx219::MdParserImx219() reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1; } -MdParser::Status MdParserImx219::Parse(void *data) +MdParser::Status MdParserImx219::Parse(libcamera::Span<const uint8_t> buffer) { bool try_again = false; @@ -132,7 +132,7 @@ MdParser::Status MdParserImx219::Parse(void *data) /* Need to be ordered */ uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG }; reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1; - int ret = static_cast<int>(findRegs(static_cast<uint8_t *>(data), + int ret = static_cast<int>(findRegs(buffer, regs, reg_offsets_, 3)); /* * > 0 means "worked partially but parse again next time", @@ -148,7 +148,7 @@ MdParser::Status MdParserImx219::Parse(void *data) if (reg_offsets_[i] == -1) continue; - reg_values_[i] = static_cast<uint8_t *>(data)[reg_offsets_[i]]; + reg_values_[i] = buffer[reg_offsets_[i]]; } /* Re-parse next time if we were unhappy in some way. */ diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp index 73a5ca7d..a4a58c15 100644 --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp @@ -21,7 +21,7 @@ class MdParserImx477 : public MdParserSmia { public: MdParserImx477(); - Status Parse(void *data) override; + Status Parse(libcamera::Span<const uint8_t> buffer) override; Status GetExposureLines(unsigned int &lines) override; Status GetGainCode(unsigned int &gain_code) override; private: @@ -107,7 +107,7 @@ MdParserImx477::MdParserImx477() reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1; } -MdParser::Status MdParserImx477::Parse(void *data) +MdParser::Status MdParserImx477::Parse(libcamera::Span<const uint8_t> buffer) { bool try_again = false; @@ -126,7 +126,7 @@ MdParser::Status MdParserImx477::Parse(void *data) GAINLO_REG }; reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1; - int ret = static_cast<int>(findRegs(static_cast<uint8_t *>(data), + int ret = static_cast<int>(findRegs(buffer, regs, reg_offsets_, 4)); /* * > 0 means "worked partially but parse again next time", @@ -142,7 +142,7 @@ MdParser::Status MdParserImx477::Parse(void *data) if (reg_offsets_[i] == -1) continue; - reg_values_[i] = static_cast<uint8_t *>(data)[reg_offsets_[i]]; + reg_values_[i] = buffer[reg_offsets_[i]]; } /* Re-parse next time if we were unhappy in some way. */ diff --git a/src/ipa/raspberrypi/md_parser.cpp b/src/ipa/raspberrypi/md_parser.cpp index d82c102c..852a1d34 100644 --- a/src/ipa/raspberrypi/md_parser.cpp +++ b/src/ipa/raspberrypi/md_parser.cpp @@ -27,12 +27,13 @@ using namespace RPiController; #define REG_VALUE 0x5a #define REG_SKIP 0x55 -MdParserSmia::ParseStatus MdParserSmia::findRegs(unsigned char *data, +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer, uint32_t regs[], int offsets[], unsigned int num_regs) { assert(num_regs > 0); - if (data[0] != LINE_START) + + if (buffer[0] != LINE_START) return NO_LINE_START; unsigned int current_offset = 1; // after the LINE_START @@ -40,15 +41,15 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(unsigned char *data, unsigned int reg_num = 0, first_reg = 0; ParseStatus retcode = PARSE_OK; while (1) { - int tag = data[current_offset++]; + int tag = buffer[current_offset++]; if ((bits_per_pixel_ == 10 && (current_offset + 1 - current_line_start) % 5 == 0) || (bits_per_pixel_ == 12 && (current_offset + 1 - current_line_start) % 3 == 0)) { - if (data[current_offset++] != REG_SKIP) + if (buffer[current_offset++] != REG_SKIP) return BAD_DUMMY; } - int data_byte = data[current_offset++]; + int data_byte = buffer[current_offset++]; //printf("Offset %u, tag 0x%02x data_byte 0x%02x\n", current_offset-1, tag, data_byte); if (tag == LINE_END_TAG) { if (data_byte != LINE_END_TAG) @@ -59,18 +60,18 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(unsigned char *data, current_offset = current_line_start + line_length_bytes_; // Require whole line to be in the buffer (if buffer size set). - if (buffer_size_bytes_ && + if (buffer.size() && current_offset + line_length_bytes_ > - buffer_size_bytes_) + buffer.size()) return MISSING_REGS; - if (data[current_offset] != LINE_START) + if (buffer[current_offset] != LINE_START) return NO_LINE_START; } else { // allow a zero line length to mean "hunt for the next line" - while (data[current_offset] != LINE_START && - current_offset < buffer_size_bytes_) + while (buffer[current_offset] != LINE_START && + current_offset < buffer.size()) current_offset++; - if (current_offset == buffer_size_bytes_) + if (current_offset == buffer.size()) return NO_LINE_START; } // inc current_offset to after LINE_START diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp index 926f676e..8e22b1d7 100644 --- a/src/ipa/raspberrypi/md_parser.hpp +++ b/src/ipa/raspberrypi/md_parser.hpp @@ -8,6 +8,8 @@ #include <stdint.h> +#include <libcamera/span.h> + /* Camera metadata parser class. Usage as shown below. Setup: @@ -21,17 +23,15 @@ parser->SetBitsPerPixel(bpp); parser->SetLineLengthBytes(pitch); parser->SetNumLines(2); -Note 1: if you don't know how many lines there are, you can use SetBufferSize -instead to limit the total buffer size. +Note 1: if you don't know how many lines there are, the size of the input +buffer is used as a limit instead. Note 2: if you don't know the line length, you can leave the line length unset -(or set to zero) and the parser will hunt for the line start instead. In this -case SetBufferSize *must* be used so that the parser won't run off the end of -the buffer. +(or set to zero) and the parser will hunt for the line start instead. Then on every frame: -if (parser->Parse(data) != MdParser::OK) +if (parser->Parse(buffer) != MdParser::OK) much badness; unsigned int exposure_lines, gain_code if (parser->GetExposureLines(exposure_lines) != MdParser::OK) @@ -75,11 +75,7 @@ public: { line_length_bytes_ = num_bytes; } - void SetBufferSize(unsigned int num_bytes) - { - buffer_size_bytes_ = num_bytes; - } - virtual Status Parse(void *data) = 0; + virtual Status Parse(libcamera::Span<const uint8_t> buffer) = 0; virtual Status GetExposureLines(unsigned int &lines) = 0; virtual Status GetGainCode(unsigned int &gain_code) = 0; @@ -116,7 +112,7 @@ protected: BAD_LINE_END = -4, BAD_PADDING = -5 }; - ParseStatus findRegs(unsigned char *data, uint32_t regs[], + ParseStatus findRegs(libcamera::Span<const uint8_t> buffer, uint32_t regs[], int offsets[], unsigned int num_regs); };
Improve MdParser::Parse() by taking a Span, pointing to const data that it should not change, as its input buffer. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/raspberrypi/cam_helper_imx219.cpp | 8 ++++---- src/ipa/raspberrypi/cam_helper_imx477.cpp | 8 ++++---- src/ipa/raspberrypi/md_parser.cpp | 23 ++++++++++++----------- src/ipa/raspberrypi/md_parser.hpp | 20 ++++++++------------ 4 files changed, 28 insertions(+), 31 deletions(-)