[libcamera-devel,1/1] cam: Add Rectangle type parsing in capture script
diff mbox series

Message ID 20220622160226.42220-2-dse@thaumatec.com
State Accepted
Headers show
Series
  • cam: Add Rectangle type parsing in capture script
Related show

Commit Message

Daniel Semkowicz June 22, 2022, 4:02 p.m. UTC 
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.
---
 src/cam/capture_script.cpp | 104 +++++++++++++++++++++++++++++++++----
 src/cam/capture_script.h   |   9 +++-
 2 files changed, 102 insertions(+), 11 deletions(-)

Comments

Daniel Semkowicz June 22, 2022, 4:06 p.m. UTC | #1 
On Wed, Jun 22, 2022 at 6:02 PM Daniel Semkowicz <dse@thaumatec.com> 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.
> ---
>  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;
> +       }
> +
>         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;
> +               }
> +
> +               if (event->type != YAML_SEQUENCE_START_EVENT) {
> +                       return -EINVAL;
> +               }
> +
> +               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;
> +               }
> +
> +               std::string value = eventScalarValue(event);
> +               values.push_back(value);
> +               if (value.empty())
> +                       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);
> +}
> 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
>


Sorry, I forgot to sign-off this commit... I will fix this after
review with any additional corrections

Best regards

Daniel
Jacopo Mondi June 23, 2022, 9:01 a.m. UTC | #2 
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 ?

>  	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

> +
> +		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 ?

> +			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 ?

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
>
Daniel Semkowicz June 23, 2022, 10:07 a.m. UTC | #3 
Hi Jacopo,

On Thu, Jun 23, 2022 at 11:01 AM Jacopo Mondi <jacopo@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
> >
Jacopo Mondi June 23, 2022, 10:41 a.m. UTC | #4 
Hi Daniel,

On Thu, Jun 23, 2022 at 12:07:37PM +0200, Daniel Semkowicz wrote:
> Hi Jacopo,
>
> On Thu, Jun 23, 2022 at 11:01 AM Jacopo Mondi <jacopo@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

Awesome!

> 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

FYI there's a port of the same IPU3 algorithm for RPi if it helps
making a comparison
https://patchwork.libcamera.org/project/libcamera/list/?series=3174

+Cc Jean-Michel as he's the author


> 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.

I think gradual steps is indeed better. The trigger-based version can
be submitted first, and continuous later.

The best of the best would be a coordinate effort in having both the
RkISP1 and the RPi AF algorithm merged, so feel free to comment/review
the RPi series if you have comments on it.

> This would introduce some functionalities earlier but not fully working.
> What do you think?

As long as they do not introduce regressions, and there's a plan to
use them, I think it's probably fine..

Thanks
  j

>
> 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
> > >

Patch
diff mbox series 

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;
+	}
+
 	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;
+		}
+
+		if (event->type != YAML_SEQUENCE_START_EVENT) {
+			return -EINVAL;
+		}
+
+		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;
+		}
+
+		std::string value = eventScalarValue(event);
+		values.push_back(value);
+		if (value.empty())
+			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);
+}
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);
 };