[libcamera-devel] [PATCH] cam: capture_script: Support parsing array controls
Jacopo Mondi
jacopo at jmondi.org
Fri Nov 4 13:29:54 CET 2022
On Fri, Nov 04, 2022 at 10:41:22AM +0000, Kieran Bingham wrote:
> 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 ;-)
>
It will fail later when trying to wrap the vector's memory in a
Span<bool>
value = Span<bool>(values.data(), idx);
error: no matching function for call to
`libcamera::Span<bool>::Span(unsigned int*, unsined int&)’
> > > + */
> > > + 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?)
I don't think that's different than any other control types.
When constructing a ControlValue the data wrapped in the Span are
copied in the ControlValue memory by ControlValue::set().
If you consider that for other types we use a variable with local
scope to store the data as we parse repr, if no copy would happen at
ControlValue construction time we would access invalid memory.
>
>
> 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