[libcamera-devel] [PATCH 1/1] cam: Add Rectangle type parsing in capture script
Jacopo Mondi
jacopo at jmondi.org
Thu Jun 23 11:01:46 CEST 2022
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
>
More information about the libcamera-devel
mailing list