[libcamera-devel] [PATCH v3 2/4] pipeline: raspberrypi: Update naming convention for tuning files

[David Plowman]

Hi everyone!

On Wed, 5 Oct 2022 at 11:38, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Wed, Oct 05, 2022 at 11:17:20AM +0100, Naushir Patuck wrote:
> > On Wed, 5 Oct 2022 at 11:07, Laurent Pinchart wrote:
> > > On Wed, Oct 05, 2022 at 10:50:19AM +0100, Naushir Patuck wrote:
> > > > On Tue, 4 Oct 2022 at 20:43, Laurent Pinchart wrote:
> > > > > On Mon, Oct 03, 2022 at 10:55:56AM +0100, Naushir Patuck wrote:
> > > > > > Append "_mono" to the sensor name when generating the tuning filename for
> > > > > > monochrome sensor variants. So the new naming convention is as follows:
> > > > > >
> > > > > > <sensor_name>.json - Standard colour sensor variant
> > > > > > <sensor_name>_mono.json - Monochrom sensor variant
> > > > > >
> > > > > > Rename the existing imx296.json file to imx296_mono.json as this tuning file
> > > > > > is based on the monochrome variant of the IMX296.
> > > > >
> > > > > We'll still have to figure out how to identify camera module, but that's
> > > > > fine, we don't have to solve all issues in one go.
> > > > >
> > > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > > ---
> > > > > >  .../data/{imx296.json => imx296_mono.json}    |  0
> > > > > >  src/ipa/raspberrypi/data/meson.build          |  2 +-
> > > > > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 20 +++++++++++++++----
> > > > > >  3 files changed, 17 insertions(+), 5 deletions(-)
> > > > > >  rename src/ipa/raspberrypi/data/{imx296.json => imx296_mono.json} (100%)
> > > > > >
> > > > > > diff --git a/src/ipa/raspberrypi/data/imx296.json b/src/ipa/raspberrypi/data/imx296_mono.json
> > > > > > similarity index 100%
> > > > > > rename from src/ipa/raspberrypi/data/imx296.json
> > > > > > rename to src/ipa/raspberrypi/data/imx296_mono.json
> > > > > > diff --git a/src/ipa/raspberrypi/data/meson.build b/src/ipa/raspberrypi/data/meson.build
> > > > > > index 211811cfa915..465a7a17ce6f 100644
> > > > > > --- a/src/ipa/raspberrypi/data/meson.build
> > > > > > +++ b/src/ipa/raspberrypi/data/meson.build
> > > > > > @@ -4,7 +4,7 @@ conf_files = files([
> > > > > >      'imx219.json',
> > > > > >      'imx219_noir.json',
> > > > > >      'imx290.json',
> > > > > > -    'imx296.json',
> > > > > > +    'imx296_mono.json',
> > > > > >      'imx378.json',
> > > > > >      'imx477.json',
> > > > > >      'imx477_noir.json',
> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > index dcd81650c32d..39a1c798df2e 100644
> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > @@ -67,6 +67,14 @@ SensorFormats populateSensorFormats(std::unique_ptr<CameraSensor> &sensor)
> > > > > >       return formats;
> > > > > >  }
> > > > > >
> > > > > > +bool isMonoSensor(std::unique_ptr<CameraSensor> &sensor)
> > > > > > +{
> > > > > > +     unsigned int mbusCode = sensor->mbusCodes()[0];
> > > > > > +     const BayerFormat &bayer = BayerFormat::fromMbusCode(mbusCode);
> > > > > > +
> > > > > > +     return bayer.order == BayerFormat::Order::MONO;
> > > > > > +}
> > > > > > +
> > > > > >  PixelFormat mbusCodeToPixelFormat(unsigned int mbus_code,
> > > > > >                                 BayerFormat::Packing packingReq)
> > > > > >  {
> > > > > > @@ -1551,12 +1559,16 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
> > > > > >        */
> > > > > >       std::string configurationFile;
> > > > > >       char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE");
> > > > > > -     if (!configFromEnv || *configFromEnv == '\0')
> > > > > > -             configurationFile = ipa_->configurationFile(sensor_->model() + ".json");
> > > > > > -     else
> > > > > > +     std::string model = sensor_->model();
> > > > > > +     if (!configFromEnv || *configFromEnv == '\0') {
> > > > > > +             if (isMonoSensor(sensor_))
> > > > > > +                     model += "_mono";
> > > > > > +             configurationFile = ipa_->configurationFile(model + ".json");
> > > > > > +     } else {
> > > > > >               configurationFile = std::string(configFromEnv);
> > > > > > +     }
> > > > > >
> > > > > > -     IPASettings settings(configurationFile, sensor_->model());
> > > > > > +     IPASettings settings(configurationFile, model);
> > > > >
> > > > > The model will be suffixed by "_mono" only when the
> > > > > LIBCAMERA_RPI_TUNING_FILE environment variable isn't set. Is that on
> > > > > purpose ?
> > > >
> > > > Yes, this is intentional, Jacopo queried the same thing :)
> > > >
> > > > If a user specifies the LIBCAMERA_RPI_TUNING_FILE env argument,
> > > > we expect them to provide the exact filename that must be used including
> > > > path and all. This user provided filename may have not resemblance to the
> > > > sensor model either.  So it will never be modified with variant suffixes.
> > >
> > > For the configuration file name, that's fine, but note how the model is
> > > passed as the second argument to the IPASettings constructor. It's
> > > currently used by the IPA module to instantiate the camera sensor
> > > helper. If a custom configuration file is specified, the sensor model
> > > won't get the "_mono" suffix. It should work fine for now I think, as
> > > the helper matching logic performs a partial string match, but it seems
> > > a bit inconsistent. Shouldn't the IPA module get the "_mono" model name,
> > > even if a custom tuning file is used ?
> >
> > Sorry, I misunderstood your point!
> >
> > Yes, you are right, sensor model will not get the _mono suffix in the
> > env variable case.  But by luck, the cam helper does a partial match
> > and it all works.
> >
> > IMHO sensor model should not get the _mono suffix as the model
> > string is only needed to match the CamHelper instance.  Apart from
> > the config file itself, the IPA does not need to know about sensor variant
> > differences.
> >
> > What do you and David think?

To be honest, I'm struggling to get too excited about this! I think if
the CamHelper really doesn't need anything other than the sensor name,
and has no dependencies on other things (like "mono-ness"), then we
shouldn't add anything. If we later on discover something that does
matter... well then maybe we need to change our minds!

David

>
> The IPA module should be able to detect that the sensor is monochrome
> from the pixel format, so I'm fine not adding a "_mono" suffix to the
> model here. I'm equally fine adding it :-) My only concern is that we
> should be consistent and do the same in both cases.
>
> > > > > >
> > > > > >       return ipa_->init(settings, result);
> > > > > >  }
>
> --
> Regards,
>
> Laurent Pinchart