[libcamera-devel] [PATCH v3 1/2] cam: Add Rectangle type parsing in capture script
Jacopo Mondi
jacopo at jmondi.org
Mon Aug 8 10:49:58 CEST 2022
Hi Paul,
On Mon, Aug 08, 2022 at 03:56:05PM +0900, paul.elder at ideasonboard.com wrote:
> Hi Jacopo,
>
> On Fri, Aug 05, 2022 at 09:20:26AM +0200, Jacopo Mondi wrote:
> > Hi Paul
> >
> > On Fri, Aug 05, 2022 at 04:47:46AM +0900, paul.elder at ideasonboard.com wrote:
> > > Hi Daniel,
> > >
> > > Thanks for the patches.
> > >
> > > On Tue, Jun 28, 2022 at 08:35:03AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > > > Hi Jacopo,
> > > >
> > > > On Mon, Jun 27, 2022 at 6:06 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > > >
> > > > > Hi Daniel
> > > > >
> > > > > On Mon, Jun 27, 2022 at 02:28:05PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > > > > > This change is required for AfWindows control from capture script.
> > > > > > Parser expects array of arrays of parameters, so it is possible to
> > > > > > specify multiple rectangles.
> > > > > >
> > > > > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > > > > ---
> > > > > > src/cam/capture_script.cpp | 145 ++++++++++++++++++++++++++++++++++---
> > > > > > src/cam/capture_script.h | 7 +-
> > > > > > 2 files changed, 139 insertions(+), 13 deletions(-)
> > > > > >
> > > > > > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp
> > > > > > index 9f22d5f7..6278a152 100644
> > > > > > --- a/src/cam/capture_script.cpp
> > > > > > +++ b/src/cam/capture_script.cpp
> > > > > > @@ -232,12 +232,15 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)
> > > > > > return -EINVAL;
> > > > > > }
> > > > > >
> > > > > > - std::string value = parseScalar();
> > > > > > - if (value.empty())
> > > > > > + const ControlId *controlId = it->second;
> > > > > > +
> > > > > > + ControlValue val = unpackControl(controlId);
> > > > > > + if (val.isNone()) {
> > > > > > + std::cerr << "Error unpacking control '" << name << "'"
> > > > > > + << std::endl;
> > > > > > return -EINVAL;
> > > > > > + }
> > > > > >
> > > > > > - const ControlId *controlId = it->second;
> > > > > > - ControlValue val = unpackControl(controlId, value);
> > > > > > controls.set(controlId->id(), val);
> > > > > >
> > > > > > return 0;
> > > > > > @@ -252,6 +255,104 @@ std::string CaptureScript::parseScalar()
> > > > > > return eventScalarValue(event);
> > > > > > }
> > > > > >
> > > > > > +ControlValue CaptureScript::parseRectangles()
> > > > > > +{
> > > > > > + std::vector<libcamera::Rectangle> rectangles;
> > > > > > +
> > > > > > + std::vector<std::vector<std::string>> arrays = parseArrays();
> > > > > > + if (arrays.empty())
> > > > > > + return {};
> > > > > > +
> > > > > > + for (const std::vector<std::string> &values : arrays) {
> > > > > > + if (values.size() != 4) {
> > > > > > + std::cerr << "Error parsing Rectangle: expected "
> > > > > > + "array with 4 parameters" << std::endl;
> > > > > > + return {};
> > > > > > + }
> > > > > > +
> > > > > > + Rectangle rect = unpackRectangle(values);
> > > > > > + rectangles.push_back(rect);
> > > > > > + }
> > > > > > +
> > > > > > + ControlValue controlValue;
> > > > > > + controlValue.set(Span<const Rectangle>(rectangles));
> > > > > > +
> > > > > > + return controlValue;
> > > > > > +}
> > > > > > +
> > > > > > +std::vector<std::vector<std::string>> CaptureScript::parseArrays()
> > > > > > +{
> > > > > > + EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > > > > > + if (!event)
> > > > > > + return {};
> > > > > > +
> > > > > > + event = nextEvent();
> > > > > > + if (!event)
> > > > > > + return {};
> > > > > > +
> > > > > > + std::vector<std::vector<std::string>> valueArrays;
> > > > > > +
> > > > > > + /* Parse single array. */
> > > > > > + if (event->type == YAML_SCALAR_EVENT) {
> > > > > > + std::string firstValue = eventScalarValue(event);
> > > > > > + if (firstValue.empty())
> > > > > > + return {};
> > > > > > +
> > > > > > + std::vector<std::string> remaining = parseSingleArray();
> > > > >
> > > > > nit: remaining can potentilly be {}
> > > >
> > > > Yes, but the vector can be empty not only because of error, but also
> > > > because there was a valid array but with only one element (already read
> > > > in firstValue).
> > > > We would need to add additional parameter returned from method to
> > > > distinguish between these two situations.
> > > > The higher level that calls parseArrays() should already know the
> > > > expected number of elements in the single array, so the error will be
> > > > caught anyway. I wouldn't spend too much time tweaking this solution as
> > > > it is not critical.
> > > > What do you think about leaving it as it is?
> > > >
> > > > >
> > > > > > +
> > > > > > + std::vector<std::string> values = { firstValue };
> > > > > > + values.insert(std::end(values),
> > > > > > + std::begin(remaining), std::end(remaining));
> > > > > > + valueArrays.push_back(values);
> > > > > > +
> > > > > > + return valueArrays;
> > > > > > + }
> > > > >
> > > > > This should address Laurent's comment and I assume it now works with
> > > > > both
> > > > > AfWindows:
> > > > > - [x, y, w, h]
> > >
> > > Gotta be honest, I'm not a big fan of representing Rectangles this way;
> > > it's a bit... flat (if that's the right word).
> > > I'd prefer it to be like (x,y)/wxh
> >
> > This would require specifying the rectangle as a string. How will it
> > look like in the .yaml file ? Do you have examples you have tested
> > your version to share ?
>
> Oh right I forgot we've got a yaml spec to conform to :S
>
> I just have it in there as-is:
>
> frames:
> - 1:
> ScalerCrop: (0,0)/1920x1080
>
> I suppose yaml doesn't have a "rectangle" type.
>
> So if it's turning that into a string vs into an array... yeah I guess
> an array of four ints is better.
I would indeed prefer to avoid strings. From a correctness point of
view we won't lose much as your code parses the string and if we ever
would have a schema we can impose a format, but I feel it would be
harder to express multiple rectangles. We could consider more complex
mappings like:
Rectangles:
- x: 0
y: 0
w: 100
h: 100
- x: 120
y: 120
w: 80
h: 80
which are maybe easier to validate, but as far as I can tell won't
give us anything more than a list of integers
>
> Alright, nvm my objection then :)
Care to send a tag ? :)
I guess we can collect that patch if it's fine with you!
Thanks
j
>
>
> Paul
>
> >
> > >
> > >
> > > > >
> > > > > and
> > > > > AfWindows:
> > > > > - [[x, y, w, h], [x1, y1, w1, h1] .. ]
> > > > >
> > > >
> > > > Exactly, I tested it with both versions :)
> > > >
> > > > Best regards
> > > > Daniel
> > > >
> > > > > > +
> > > > > > + /* Parse array of arrays. */
> > > > > > + while (1) {
> > > > > > + switch (event->type) {
> > > > > > + case YAML_SEQUENCE_START_EVENT: {
> > > > > > + std::vector<std::string> values = parseSingleArray();
> > > > > > + valueArrays.push_back(values);
> > > > > > + break;
> > > > > > + }
> > > > > > + case YAML_SEQUENCE_END_EVENT:
> > > > > > + return valueArrays;
> > > > > > + default:
> > > > > > + return {};
> > > > > > + }
> > > > > > +
> > > > > > + event = nextEvent();
> > > > > > + if (!event)
> > > > > > + return {};
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > +std::vector<std::string> CaptureScript::parseSingleArray()
> > > > > > +{
> > > > > > + std::vector<std::string> values;
> > > > > > +
> > > > > > + while (1) {
> > > > > > + EventPtr event = nextEvent();
> > > > > > + if (!event)
> > > > > > + return {};
> > > > > > +
> > > > > > + switch (event->type) {
> > > > > > + case YAML_SCALAR_EVENT: {
> > > > > > + std::string value = eventScalarValue(event);
> > > > > > + if (value.empty())
> > > > > > + return {};
> > > > > > + values.push_back(value);
> > > > > > + break;
> > > > > > + }
> > > > > > + case YAML_SEQUENCE_END_EVENT:
> > > > > > + return values;
> > > > > > + default:
> > > > > > + return {};
> > > > > > + }
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)
> > > > > > {
> > > > > > static const std::map<unsigned int, const char *> typeNames = {
> > > > > > @@ -277,9 +378,24 @@ void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)
> > > > > > << typeName << " control " << id->name() << std::endl;
> > > > > > }
> > > > > >
> > > > > > -ControlValue CaptureScript::unpackControl(const ControlId *id,
> > > > > > - const std::string &repr)
> > > > > > +ControlValue CaptureScript::unpackControl(const ControlId *id)
> > > > > > {
> > > > > > + /* 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 {};
> > > > > > +
> > > > >
> > > > > Thanks, this address my comment on v1.
> > > > >
> > > > > Nits apart it looks good to me
> > > > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > >
> > > > > Thanks
> > > > > j
> > > > >
> > > > > > ControlValue value{};
> > > > > >
> > > > > > switch (id->type()) {
> > > > > > @@ -324,13 +440,20 @@ ControlValue CaptureScript::unpackControl(const ControlId *id,
> > > > > > value.set<std::string>(repr);
> > > > > > break;
> > > > > > }
> > > > > > - case ControlTypeRectangle:
> > > > > > - /* \todo Parse rectangles. */
> > > > > > - break;
> > > > > > - case ControlTypeSize:
> > > > > > - /* \todo Parse Sizes. */
> > > > > > + default:
> > > > > > + std::cerr << "Unsupported control type" << std::endl;
> > > > > > break;
> > > > > > }
> > > > > >
> > > > > > return value;
> > > > > > }
> > > > > > +
> > > > > > +libcamera::Rectangle CaptureScript::unpackRectangle(const std::vector<std::string> &strVec)
> > > > > > +{
> > > > > > + int x = strtol(strVec[0].c_str(), NULL, 10);
> > > > > > + int y = strtol(strVec[1].c_str(), NULL, 10);
> > > > > > + unsigned int width = strtoul(strVec[2].c_str(), NULL, 10);
> > > > > > + unsigned int height = strtoul(strVec[3].c_str(), NULL, 10);
> > > > > > +
> > > > > > + return Rectangle(x, y, width, height);
> > > > > > +}
> > > > > > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h
> > > > > > index 8b4f8f62..fffe67e5 100644
> > > > > > --- a/src/cam/capture_script.h
> > > > > > +++ b/src/cam/capture_script.h
> > > > > > @@ -54,9 +54,12 @@ private:
> > > > > > int parseControl(EventPtr event, libcamera::ControlList &controls);
> > > > > >
> > > > > > std::string parseScalar();
> > > > > > + libcamera::ControlValue parseRectangles();
> > > > > > + std::vector<std::vector<std::string>> parseArrays();
> > > > > > + std::vector<std::string> parseSingleArray();
> > > > > >
> > > > > > void unpackFailure(const libcamera::ControlId *id,
> > > > > > const std::string &repr);
> > > > > > - libcamera::ControlValue unpackControl(const libcamera::ControlId *id,
> > > > > > - const std::string &repr);
> > > > > > + libcamera::ControlValue unpackControl(const libcamera::ControlId *id);
> > > > > > + libcamera::Rectangle unpackRectangle(const std::vector<std::string> &strVec);
> > > > > > };
> > > > > > --
> > > > > > 2.34.1
> > > > > >
More information about the libcamera-devel
mailing list