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

[Kieran Bingham]

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."


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
>