[libcamera-devel] [PATCH] cam: capture_script: Support parsing array controls
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Nov 4 11:41:22 CET 2022
Quoting Paul Elder via libcamera-devel (2022-11-04 06:38:12)
> On Wed, Nov 02, 2022 at 05:05:56PM +0100, Jacopo Mondi via libcamera-devel wrote:
> > Add support for parsing array controls to the cam capture script.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > ---
> > v1->v2:
> > - Rebased on src/apps/
> > - Use the correct size when parsing boolean controls as noted by Paul
> > ---
> > src/apps/cam/capture_script.cpp | 158 ++++++++++++++++++++++++++++----
> > src/apps/cam/capture_script.h | 5 +
> > 2 files changed, 146 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp
> > index 5a27361ce4d9..062a7258e414 100644
> > --- a/src/apps/cam/capture_script.cpp
> > +++ b/src/apps/cam/capture_script.cpp
> > @@ -454,24 +454,9 @@ void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)
> > << typeName << " control " << id->name() << std::endl;
> > }
> >
> > -ControlValue CaptureScript::unpackControl(const ControlId *id)
> > +ControlValue CaptureScript::parseScalarControl(const ControlId *id,
> > + const std::string repr)
> > {
> > - /* Parse complex types. */
> > - switch (id->type()) {
> > - case ControlTypeRectangle:
> > - return parseRectangles();
> > - case ControlTypeSize:
> > - /* \todo Parse Sizes. */
> > - return {};
> > - default:
> > - break;
> > - }
> > -
> > - /* Parse basic types represented by a single scalar. */
> > - const std::string repr = parseScalar();
> > - if (repr.empty())
> > - return {};
> > -
> > ControlValue value{};
> >
> > switch (id->type()) {
> > @@ -524,6 +509,145 @@ ControlValue CaptureScript::unpackControl(const ControlId *id)
> > return value;
> > }
> >
> > +ControlValue CaptureScript::parseArrayControl(const ControlId *id,
> > + const std::vector<std::string> &repr)
> > +{
> > + ControlValue value{};
> > +
> > + switch (id->type()) {
> > + case ControlTypeNone:
> > + break;
> > + case ControlTypeBool: {
> > + /*
> > + * This is unpleasant, but we cannot use an std::vector<> as its
> > + * boolean type overload does not allow to access the raw data,
> > + * as boolean values are stored in a bitmask for efficiency.
> > + *
> > + * As we need a contiguous memory region to wrap in a Span<>,
> > + * use an array instead but be strict about not overflowing it
> > + * by limiting the number of controls we can store.
> > + *
> > + * Be loud but do not fail, as the issue would present at
> > + * runtime and it's not fatal.
Out of interest, would a std::vector<int> (or unsigned int?, or char)
work to store the bools ? Or does that cause too much implicit type
conversions or other pains?
I suspect it doesn't necessarily solve all the problems, or causes
different ones ;-)
> > + */
> > + static constexpr unsigned int kMaxNumBooleanControls = 1024;
> > + std::array<bool, kMaxNumBooleanControls> values;
> > + unsigned int idx = 0;
> > +
> > + for (const std::string &s : repr) {
> > + bool val;
> > +
> > + if (s == "true") {
> > + val = true;
> > + } else if (s == "false") {
> > + val = false;
> > + } else {
> > + unpackFailure(id, s);
> > + return value;
> > + }
> > +
> > + if (idx == kMaxNumBooleanControls) {
>
> By habit I'd s/==/>=/, but even without it it's correct, so eh.
>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>
> > + std::cerr << "Cannot parse more than "
> > + << kMaxNumBooleanControls
> > + << " boolean controls" << std::endl;
> > + break;
> > + }
> > +
> > + values[idx++] = val;
> > + }
> > +
> > + value = Span<bool>(values.data(), idx);
> > + break;
> > + }
> > + case ControlTypeByte: {
> > + std::vector<uint8_t> values;
> > + for (const std::string &s : repr) {
> > + uint8_t val = strtoll(s.c_str(), NULL, 10);
> > + values.push_back(val);
> > + }
> > +
> > + value = Span<const uint8_t>(values.data(), values.size());
> > + break;
> > + }
> > + case ControlTypeInteger32: {
> > + std::vector<int32_t> values;
> > + for (const std::string &s : repr) {
> > + int32_t val = strtoll(s.c_str(), NULL, 10);
> > + values.push_back(val);
> > + }
> > +
> > + value = Span<const int32_t>(values.data(), values.size());
> > + break;
> > + }
> > + case ControlTypeInteger64: {
> > + std::vector<int64_t> values;
> > + for (const std::string &s : repr) {
> > + int64_t val = strtoll(s.c_str(), NULL, 10);
> > + values.push_back(val);
> > + }
> > +
> > + value = Span<const int64_t>(values.data(), values.size());
> > + break;
> > + }
> > + case ControlTypeFloat: {
> > + std::vector<float> values;
> > + for (const std::string &s : repr)
> > + values.push_back(strtof(s.c_str(), NULL));
> > +
> > + value = Span<const float>(values.data(), values.size());
> > + break;
> > + }
> > + case ControlTypeString: {
> > + value = Span<const std::string>(repr.data(), repr.size());
Is this safe?
Does a copy of repr.data() need to be made somewhere and stored
explicitly?
Perhaps it's ok if the script is guaranteed to remain in memory while
for the lifetime of the control value ? but .. ?
Aside from that, which I'll let you identify if it needs anything
different (or an explicit test?)
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > + break;
> > + }
> > + default:
> > + std::cerr << "Unsupported control type" << std::endl;
> > + break;
> > + }
> > +
> > + return value;
> > +}
> > +
> > +ControlValue CaptureScript::unpackControl(const ControlId *id)
> > +{
> > + /* Parse complex types. */
> > + switch (id->type()) {
> > + case ControlTypeRectangle:
> > + return parseRectangles();
> > + case ControlTypeSize:
> > + /* \todo Parse Sizes. */
> > + return {};
> > + default:
> > + break;
> > + }
> > +
> > + /* Check if the control has a single scalar value or is an array. */
> > + EventPtr event = nextEvent();
> > + if (!event)
> > + return {};
> > +
> > + switch (event->type) {
> > + case YAML_SCALAR_EVENT: {
> > + const std::string repr = eventScalarValue(event);
> > + if (repr.empty())
> > + return {};
> > +
> > + return parseScalarControl(id, repr);
> > + }
> > + case YAML_SEQUENCE_START_EVENT: {
> > + std::vector<std::string> array = parseSingleArray();
> > + if (array.empty())
> > + return {};
> > +
> > + return parseArrayControl(id, array);
> > + }
> > + default:
> > + std::cerr << "Unexpected event type: " << event->type << std::endl;
> > + return {};
> > + }
> > +}
> > +
> > libcamera::Rectangle CaptureScript::unpackRectangle(const std::vector<std::string> &strVec)
> > {
> > int x = strtol(strVec[0].c_str(), NULL, 10);
> > diff --git a/src/apps/cam/capture_script.h b/src/apps/cam/capture_script.h
> > index 7a0ddebb00b5..40042c0330f0 100644
> > --- a/src/apps/cam/capture_script.h
> > +++ b/src/apps/cam/capture_script.h
> > @@ -56,6 +56,11 @@ private:
> > int parseFrame(EventPtr event);
> > int parseControl(EventPtr event, libcamera::ControlList &controls);
> >
> > + libcamera::ControlValue parseScalarControl(const libcamera::ControlId *id,
> > + const std::string repr);
> > + libcamera::ControlValue parseArrayControl(const libcamera::ControlId *id,
> > + const std::vector<std::string> &repr);
> > +
> > std::string parseScalar();
> > libcamera::ControlValue parseRectangles();
> > std::vector<std::vector<std::string>> parseArrays();
> > --
> > 2.38.1
> >
More information about the libcamera-devel
mailing list