<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 21 Jun 2022 at 13:23, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Naushir Patuck via libcamera-devel (2022-06-10 13:25:33)<br>
> The ScalerCrop control is handled directly by the pipeline handler. Remove the<br>
> control from the IPA's static ControlInfoMap, and let the pipeline handler add<br>
> it to the ControlInfoMap advertised to the application, ensuring the limits<br>
> are set appropriately based on the current sensor mode.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  src/ipa/raspberrypi/raspberrypi.cpp                |  1 -<br>
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++++-<br>
>  2 files changed, 9 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 295f6b735dc0..f46fccdd4177 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -86,7 +86,6 @@ static const ControlInfoMap::Map ipaControls{<br>
>         { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },<br>
>         { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },<br>
>         { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },<br>
> -       { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },<br>
>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }<br>
>  };<br>
>  <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 4596f2babcea..66a84b1dfb97 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -941,7 +941,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
>         data->properties_.set(properties::SensorSensitivity, result.modeSensitivity);<br>
>  <br>
>         /* Update the controls that the Raspberry Pi IPA can handle. */<br>
> -       data->controlInfo_ = result.controlInfo;<br>
> +       ControlInfoMap::Map ctrlMap;<br>
> +       for (auto const &c : result.controlInfo)<br>
> +               ctrlMap.emplace(c.first, c.second);<br>
> +<br>
> +       /* Add the ScalerCrop control limits based on the current mode. */<br>
> +       ctrlMap.emplace(&controls::ScalerCrop,<br>
> +                       ControlInfo(Rectangle(data->ispMinCropSize_), Rectangle(data->sensorInfo_.outputSize)));<br>
<br>
I don't think this works with emplace.<br>
<br>
Reading: <a href="https://en.cppreference.com/w/cpp/container/map/emplace" rel="noreferrer" target="_blank">https://en.cppreference.com/w/cpp/container/map/emplace</a><br>
<br>
 "The element may be constructed even if there already is an element<br>
 with the key in the container, in which case the newly constructed<br>
 element will be destroyed immediately."<br></blockquote><div><br></div><div>I see what you are getting at (I think), but the code is correct by an unfortunate</div><div>accident.  I cannot see a way to add a single element to an existing ControlInfoMap</div><div>(it is privately inherited from an unordered_map). So I have to to the following </div><div>(slightly hideous) sequence:</div><div class="gmail_quote"><br></div>1) Populate a ControlInfoMap::Map from the IPA's ControlInfo</div><div class="gmail_quote">2) emplace the "controls::ScalerCrop" to this new ControlInfoMap::Map</div><div class="gmail_quote">3) Convert the new 

ControlInfoMap::Map back to a new ControlInfo for subsequent use.</div><div class="gmail_quote"><br></div><div class="gmail_quote">Because 1) is creating a new ControlInfoMap::Map, the emplace(controls::ScalerCrop)</div><div class="gmail_quote">will not fail as the key will not be present in the map. Does that make sense?</div><div class="gmail_quote"><br></div><div class="gmail_quote">I would dearly love to have a ControlInfo::add() or ControlINfo::emplace() so I can</div><div class="gmail_quote">avoid doing the above sequence to add a new key/value to the map!</div><div class="gmail_quote"><br></div><div class="gmail_quote">Regards,</div><div class="gmail_quote">Naush</div><div class="gmail_quote"> <br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
So taking their example code, and extending with an emplace after<br>
already emplaced (which is the same as this ctrlMap being updated on any<br>
second or consecutive call to PipelineHandlerRPi::configure() ...<br>
<br>
$ cat snippets/map-emplace.cpp<br>
<br>
#include <iostream><br>
#include <utility><br>
#include <string><br>
#include <map><br>
<br>
int main()<br>
{<br>
    std::map<std::string, std::string> m;<br>
<br>
    // uses pair's move constructor<br>
    m.emplace(std::make_pair(std::string("a"), std::string("a")));<br>
<br>
    // uses pair's converting move constructor<br>
    m.emplace(std::make_pair("b", "abcd"));<br>
<br>
    // uses pair's template constructor<br>
    m.emplace("d", "ddd");<br>
<br>
    m.emplace("d", "a new ddd");<br>
<br>
    // uses pair's piecewise constructor<br>
    m.emplace(std::piecewise_construct,<br>
              std::forward_as_tuple("c"),<br>
              std::forward_as_tuple(10, 'c'));<br>
    // as of C++17, m.try_emplace("c", 10, 'c'); can be used<br>
<br>
    for (const auto &p : m) {<br>
        std::cout << p.first << " => " << p.second << '\n';<br>
    }<br>
}<br>
<br>
<br>
kbingham@Monstersaurus:~/iob/libcamera/libcamera$ make snippets/map-emplace<br>
g++     snippets/map-emplace.cpp   -o snippets/map-emplace<br>
kbingham@Monstersaurus:~/iob/libcamera/libcamera$ ./snippets/map-emplace <br>
a => a<br>
b => abcd<br>
c => cccccccccc<br>
d => ddd<br>
<br>
<br>
Perhaps we need a helper on the ControlInfoMap to support updates.<br>
<br>
<br>
> +<br>
> +       data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());<br>
>  <br>
>         /* Setup the Video Mux/Bridge entities. */<br>
>         for (auto &[device, link] : data->bridgeDevices_) {<br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div>