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

[Jacopo Mondi]

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
>