[libcamera-devel] [PATCH 1/1] cam: Add Rectangle type parsing in capture script

[Daniel Semkowicz]

Hi Jacopo,

On Thu, Jun 23, 2022 at 11:01 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Daniel,
>
> On Wed, Jun 22, 2022 at 06:02:26PM +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.
>
> You know already :) Missing SoB tag
>
> > ---
> >  src/cam/capture_script.cpp | 104 +++++++++++++++++++++++++++++++++----
> >  src/cam/capture_script.h   |   9 +++-
> >  2 files changed, 102 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp
> > index 9f22d5f7..ff620abd 100644
> > --- a/src/cam/capture_script.cpp
> > +++ b/src/cam/capture_script.cpp
> > @@ -232,12 +232,19 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)
> >               return -EINVAL;
> >       }
> >
> > -     std::string value = parseScalar();
> > -     if (value.empty())
> > -             return -EINVAL;
> > -
> >       const ControlId *controlId = it->second;
> > -     ControlValue val = unpackControl(controlId, value);
> > +     ControlValue val;
> > +
> > +     if (controlId->type() == ControlTypeRectangle) {
> > +             int result = parseRectangles(val);
> > +             if (result)
> > +                     return result;
> > +     } else {
> > +             int result = parseBasicType(controlId, val);
> > +             if (result)
> > +                     return result;
> > +     }
> > +
>
> Won't all of this be better placed in the "case ControlTypeRectangle:"
> statement of CaptureScript::unpackControl() ? Have I missed why is it
> necessary to bring it at this level ? Can't you call parseRectangles()
> from there ? if it's the parseScalar() call required to parse a basic
> type this can't it be handled as:
>
> ControlValue CaptureScript::unpackControl(const ControlId *id)
> {
>
>         /* Parse complex types */
>         swith (id->type()) {
>         case Rectangle:
>                 parseRectangles();
>                 break
>         case Size:
>                 /* error not supported */
>                 break;
>         default:
>                 break;
>         }
>
>         /* Parse basic types represented by a single scalar. */
>
>         std::string repr = parseScalar();
>         if (value.empty())
>                 return -EVINVAL;
>
>         switch (id->type()) {
>         case None:
>         case Bool:
>                 ....
>
>         }
> }
>
> Do you think this would be possible ?

It looked like the unpackControl() method was specialized to only unpack
the specific type from the string that was already read from yaml.
And reading the string from the yaml scalar event was done in parseScalar().
I just tried to maintain the already existing convention of separated
functionality of "parse" and "unpack" methods.

Unfortunately, in both cases We still need to have two paths (for basic
and complex types) - two switches instead of one.

One thing I see better in your approach is that We will have both switches
in one function and I think it would be a more clear to read.

>
> >       controls.set(controlId->id(), val);
> >
> >       return 0;
> > @@ -252,6 +259,77 @@ std::string CaptureScript::parseScalar()
> >       return eventScalarValue(event);
> >  }
> >
> > +int CaptureScript::parseBasicType(const ControlId *id, ControlValue &controlValue)
> > +{
> > +     std::string value = parseScalar();
> > +     if (value.empty())
> > +             return -EINVAL;
> > +
> > +     controlValue = unpackBasicType(id, value);
> > +
> > +     return 0;
> > +}
> > +
> > +int CaptureScript::parseRectangles(libcamera::ControlValue &controlValue)
> > +{
> > +     std::vector<libcamera::Rectangle> rectangles;
> > +
> > +     EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > +     if (!event)
> > +             return -EINVAL;
> > +
> > +     while (1) {
> > +             event = nextEvent();
> > +             if (!event)
> > +                     return -EINVAL;
> > +
> > +             if (event->type == YAML_SEQUENCE_END_EVENT) {
> > +                     break;
> > +             }
>
> no braces for single line statement.
>
> > +
> > +             if (event->type != YAML_SEQUENCE_START_EVENT) {
> > +                     return -EINVAL;
> > +             }
>
> I would consider a switch to make sure we catch all cases, even with
> hill-formed yaml files
>
>         while (1) {
>                 bool sequenceEnd = false;
>
>                 switch (event->type) {
>                 case YAML_SEQUENCE_END_EVENT:
>                         sequencEnd = true;
>                         break;
>                 case YAML_SEQUENCE_START_EVENT:
>                         break;
>                 default:
>                         return -EINVAL;
>                 }
>
>                 if (sequenceEnd)
>                         break;
> > +
> > +             std::vector<std::string> values = parseArray();
> > +             if (values.size() != 4) {
> > +                     std::cerr << "Error parsing Rectangle: "
> > +                               << "expected array with 4 parameters"
> > +                               << std::endl;
> > +                     return -EINVAL;
> > +             }
> > +
> > +             Rectangle rect = unpackRectangle(values);
> > +             rectangles.push_back(rect);
> > +     }
> > +
> > +     controlValue.set(Span<const Rectangle>(rectangles));
> > +
> > +     return 0;
> > +}
> > +
> > +std::vector<std::string> CaptureScript::parseArray()
> > +{
> > +     std::vector<std::string> values;
> > +
> > +     while (1) {
> > +             EventPtr event = nextEvent();
> > +             if (!event)
> > +                     break;
> > +
> > +             if (event->type != YAML_SCALAR_EVENT) {
> > +                     break;
> > +             }
>
> Similar comments as the ones on the previous function
>

Sure, I will correct these points.

> > +
> > +             std::string value = eventScalarValue(event);
> > +             values.push_back(value);
> > +             if (value.empty())
>
> What is this check for ? Shouldn't it be done before adding the
> element to the vector ?
>

Of course it should... I overlooked that...

> > +                     break;
> > +     }
> > +
> > +     return values;
> > +}
> > +
> >  void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)
> >  {
> >       static const std::map<unsigned int, const char *> typeNames = {
> > @@ -277,7 +355,7 @@ void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)
> >                 << typeName << " control " << id->name() << std::endl;
> >  }
> >
> > -ControlValue CaptureScript::unpackControl(const ControlId *id,
> > +ControlValue CaptureScript::unpackBasicType(const ControlId *id,
> >                                         const std::string &repr)
> >  {
> >       ControlValue value{};
> > @@ -325,12 +403,20 @@ ControlValue CaptureScript::unpackControl(const ControlId *id,
> >               break;
> >       }
> >       case ControlTypeRectangle:
> > -             /* \todo Parse rectangles. */
> > -             break;
> >       case ControlTypeSize:
> > -             /* \todo Parse Sizes. */
> > +             unpackFailure(id, repr);
> >               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);
> > +}
>
> Very happy to see Rectangle supported in the capture script! Are you
> using it to test which functionality ?
>
AfWindows. I am working on AF for rkisp1. I have a very basic AF that
just moves through the full focal range of lens and choose the best sharpness.
I was not pushing it to the upstream as this works only on AfTrigger in
such approach, so no continuous mode. I planned to change the algorithm
to something similar as already implemented in IPU3 and then push it
in the final form. But as I am writing this message now, I am wondering
if maybe it would be better to push it even in this simple form as these
changes also introduce some side-functionalities needed for Af, such as
lens control from ipu. And later push the algorithm with continuous mode.
This would introduce some functionalities earlier but not fully working.
What do you think?

Best regards
Daniel

> Thanks for the patch
>    j
>
> > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h
> > index 8b4f8f62..73b7e101 100644
> > --- a/src/cam/capture_script.h
> > +++ b/src/cam/capture_script.h
> > @@ -52,11 +52,16 @@ private:
> >       int parseFrames();
> >       int parseFrame(EventPtr event);
> >       int parseControl(EventPtr event, libcamera::ControlList &controls);
> > +     int parseBasicType(const libcamera::ControlId *id,
> > +                        libcamera::ControlValue &controlValue);
> > +     int parseRectangles(libcamera::ControlValue &controlValue);
> >
> >       std::string parseScalar();
> > +     std::vector<std::string> parseArray();
> >
> >       void unpackFailure(const libcamera::ControlId *id,
> >                          const std::string &repr);
> > -     libcamera::ControlValue unpackControl(const libcamera::ControlId *id,
> > -                                           const std::string &repr);
> > +     libcamera::ControlValue unpackBasicType(const libcamera::ControlId *id,
> > +                                             const std::string &repr);
> > +     libcamera::Rectangle unpackRectangle(const std::vector<std::string> &strVec);
> >  };
> > --
> > 2.34.1
> >