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