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

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Tue, Jun 21, 2022 at 01:45:29PM +0100, Naushir Patuck via libcamera-devel wrote:
> On Tue, 21 Jun 2022 at 13:23, Kieran Bingham 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!

ControlInfoMap was meant to be immutable, as controls exposed by a
camera were not meant to change, but as we've blown up that design
principle, I'm fine with adding a new function to the ControlInfoMap
class to add/update an element until we redesign all this.

> > 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_) {

-- 
Regards,

Laurent Pinchart