[libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Advertise ScalerCrop from the pipeline handler

[Naushir Patuck]

Hi Kieran,

Thank you for your feedback.

On Tue, 21 Jun 2022 at 13:23, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Quoting Naushir Patuck via libcamera-devel (2022-06-10 13:25:33)
> > The ScalerCrop control is handled directly by the pipeline handler.
> Remove the
> > control from the IPA's static ControlInfoMap, and let the pipeline
> handler add
> > it to the ControlInfoMap advertised to the application, ensuring the
> limits
> > are set appropriately based on the current sensor mode.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/raspberrypi.cpp                |  1 -
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++++-
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 295f6b735dc0..f46fccdd4177 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -86,7 +86,6 @@ static const ControlInfoMap::Map ipaControls{
> >         { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
> >         { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> >         { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f)
> },
> > -       { &controls::ScalerCrop, ControlInfo(Rectangle{},
> Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> >         { &controls::draft::NoiseReductionMode,
> ControlInfo(controls::draft::NoiseReductionModeValues) }
> >  };
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 4596f2babcea..66a84b1dfb97 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -941,7 +941,15 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >         data->properties_.set(properties::SensorSensitivity,
> result.modeSensitivity);
> >
> >         /* Update the controls that the Raspberry Pi IPA can handle. */
> > -       data->controlInfo_ = result.controlInfo;
> > +       ControlInfoMap::Map ctrlMap;
> > +       for (auto const &c : result.controlInfo)
> > +               ctrlMap.emplace(c.first, c.second);
> > +
> > +       /* Add the ScalerCrop control limits based on the current mode.
> */
> > +       ctrlMap.emplace(&controls::ScalerCrop,
> > +                       ControlInfo(Rectangle(data->ispMinCropSize_),
> Rectangle(data->sensorInfo_.outputSize)));
>
> I don't think this works with emplace.
>
> Reading: https://en.cppreference.com/w/cpp/container/map/emplace
>
>  "The element may be constructed even if there already is an element
>  with the key in the container, in which case the newly constructed
>  element will be destroyed immediately."
>

I see what you are getting at (I think), but the code is correct by an
unfortunate
accident.  I cannot see a way to add a single element to an existing
ControlInfoMap
(it is privately inherited from an unordered_map). So I have to to the
following
(slightly hideous) sequence:

1) Populate a ControlInfoMap::Map from the IPA's ControlInfo
2) emplace the "controls::ScalerCrop" to this new ControlInfoMap::Map
3) Convert the new  ControlInfoMap::Map back to a new ControlInfo for
subsequent use.

Because 1) is creating a new ControlInfoMap::Map, the
emplace(controls::ScalerCrop)
will not fail as the key will not be present in the map. Does that make
sense?

I would dearly love to have a ControlInfo::add() or ControlINfo::emplace()
so I can
avoid doing the above sequence to add a new key/value to the map!

Regards,
Naush


>
>
> So taking their example code, and extending with an emplace after
> already emplaced (which is the same as this ctrlMap being updated on any
> second or consecutive call to PipelineHandlerRPi::configure() ...
>
> $ cat snippets/map-emplace.cpp
>
> #include <iostream>
> #include <utility>
> #include <string>
> #include <map>
>
> int main()
> {
>     std::map<std::string, std::string> m;
>
>     // uses pair's move constructor
>     m.emplace(std::make_pair(std::string("a"), std::string("a")));
>
>     // uses pair's converting move constructor
>     m.emplace(std::make_pair("b", "abcd"));
>
>     // uses pair's template constructor
>     m.emplace("d", "ddd");
>
>     m.emplace("d", "a new ddd");
>
>     // uses pair's piecewise constructor
>     m.emplace(std::piecewise_construct,
>               std::forward_as_tuple("c"),
>               std::forward_as_tuple(10, 'c'));
>     // as of C++17, m.try_emplace("c", 10, 'c'); can be used
>
>     for (const auto &p : m) {
>         std::cout << p.first << " => " << p.second << '\n';
>     }
> }
>
>
> kbingham at Monstersaurus:~/iob/libcamera/libcamera$ make
> snippets/map-emplace
> g++     snippets/map-emplace.cpp   -o snippets/map-emplace
> kbingham at Monstersaurus:~/iob/libcamera/libcamera$ ./snippets/map-emplace
> a => a
> b => abcd
> c => cccccccccc
> d => ddd
>
>
> Perhaps we need a helper on the ControlInfoMap to support updates.
>
>
> > +
> > +       data->controlInfo_ = ControlInfoMap(std::move(ctrlMap),
> result.controlInfo.idmap());
> >
> >         /* Setup the Video Mux/Bridge entities. */
> >         for (auto &[device, link] : data->bridgeDevices_) {
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220621/f3252d37/attachment.htm>