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

[Jacopo Mondi]

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

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

and
        AfWindows:
          - [[x, y, w, h], [x1, y1, w1, h1] .. ]

> +
> +	/* 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
>