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

[Naushir Patuck]

Hi Laurent,

(cc David)


On Wed, 5 Oct 2022 at 11:07, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> 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?

Naush



>
> > > >
> > > >       return ipa_->init(settings, result);
> > > >  }
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221005/200f918e/attachment.htm>