| Message ID | 20210707134051.2740-1-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hey David, On 07.07.2021 14:40, David Plowman wrote: >The configuration (camera tuning) file used by the Raspberry Pi comes >by default from the sensor name. However, we now allow this to be >overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable. Nice I like this change, this is a great preparation for generalizations! Thank you very much. > >Signed-off-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Sebastian Fricke <sebastian.fricke@posteo.net> Greetings, Sebastian >--- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > >diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >index 082eb1ee..a738770a 100644 >--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >@@ -1232,8 +1232,18 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls); > >- IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"), >- sensor_->model()); >+ /* >+ * The configuration (tuning file) is made from the sensor name unless >+ * the environment variable overrides it. >+ */ >+ std::string configurationFile; >+ char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE"); >+ if (!configFromEnv || *configFromEnv == '\0') >+ configurationFile = ipa_->configurationFile(sensor_->model() + ".json"); >+ else >+ configurationFile = std::string(configFromEnv); >+ >+ IPASettings settings(configurationFile, sensor_->model()); > > return ipa_->init(settings, sensorConfig); > } >-- >2.20.1 >
Hi David, Thank you for your work. On Wed, 7 Jul 2021 at 14:40, David Plowman <david.plowman@raspberrypi.com> wrote: > The configuration (camera tuning) file used by the Raspberry Pi comes > by default from the sensor name. However, we now allow this to be > overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 082eb1ee..a738770a 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1232,8 +1232,18 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig > *sensorConfig) > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > ipa_->setDelayedControls.connect(this, > &RPiCameraData::setDelayedControls); > > - IPASettings settings(ipa_->configurationFile(sensor_->model() + > ".json"), > - sensor_->model()); > + /* > + * The configuration (tuning file) is made from the sensor name > unless > + * the environment variable overrides it. > + */ > + std::string configurationFile; > + char const *configFromEnv = > utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE"); > + if (!configFromEnv || *configFromEnv == '\0') > + configurationFile = > ipa_->configurationFile(sensor_->model() + ".json"); > + else > + configurationFile = std::string(configFromEnv); > + > + IPASettings settings(configurationFile, sensor_->model()); > > return ipa_->init(settings, sensorConfig); > } > -- > 2.20.1 > >
Hi David, On 07/07/2021 14:40, David Plowman wrote: > The configuration (camera tuning) file used by the Raspberry Pi comes > by default from the sensor name. However, we now allow this to be > overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable. Some how I had it in my head that this would be an override occurring in the IPA itself, but I think this is ok too. > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 082eb1ee..a738770a 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1232,8 +1232,18 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls); > > - IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"), > - sensor_->model()); > + /* > + * The configuration (tuning file) is made from the sensor name unless > + * the environment variable overrides it. > + */ > + std::string configurationFile; > + char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE"); We likely need to document this somewhere... We have Documentation/environment_variables.rst But that could be a follow up patch > + if (!configFromEnv || *configFromEnv == '\0') > + configurationFile = ipa_->configurationFile(sensor_->model() + ".json"); I'm not a particular fan of the current ipa_->configurationFile implementation, and I think I see that it should have changes in the future. If this were to be generalised, then the override should go there - but given that this is quite specific to RPi needs right now - I think this is ok. However I could envisage that in the future this environment variable gets deprecated, but I don't yet know what would replace it - so I believe it's worth while to have this for now. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + else > + configurationFile = std::string(configFromEnv); > + > + IPASettings settings(configurationFile, sensor_->model()); > > return ipa_->init(settings, sensorConfig); > } >
Hello, On Fri, Jul 09, 2021 at 04:04:22PM +0100, Kieran Bingham wrote: > On 07/07/2021 14:40, David Plowman wrote: > > The configuration (camera tuning) file used by the Raspberry Pi comes > > by default from the sensor name. However, we now allow this to be > > overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable. > > Some how I had it in my head that this would be an override occurring in > the IPA itself, but I think this is ok too. I like it better in the pipeline handler, as the IPA module isolation mechanisms may limit the ability to pass environmen variables. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 082eb1ee..a738770a 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1232,8 +1232,18 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) > > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > > ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls); > > > > - IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"), > > - sensor_->model()); > > + /* > > + * The configuration (tuning file) is made from the sensor name unless > > + * the environment variable overrides it. > > + */ > > + std::string configurationFile; > > + char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE"); > > We likely need to document this somewhere... > > We have Documentation/environment_variables.rst > > But that could be a follow up patch > > > + if (!configFromEnv || *configFromEnv == '\0') > > + configurationFile = ipa_->configurationFile(sensor_->model() + ".json"); > > I'm not a particular fan of the current ipa_->configurationFile > implementation, and I think I see that it should have changes in the future. > > If this were to be generalised, then the override should go there - but > given that this is quite specific to RPi needs right now - I think this > is ok. I'd like to see this being generalized indeed. We'll have to figure out how to specific a per-camera configuration file though. > However I could envisage that in the future this environment variable > gets deprecated, but I don't yet know what would replace it - so I > believe it's worth while to have this for now. That's the important question: David, if we later create a more generic mechanism to select a different IPA configuration file, will it be fine to drop support for LIBCAMERA_RPI_TUNING_FILE ? If so, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + else > > + configurationFile = std::string(configFromEnv); > > + > > + IPASettings settings(configurationFile, sensor_->model()); > > > > return ipa_->init(settings, sensorConfig); > > }
Hi Laurent, David's away this week, so I'll respond on his behalf. On Sun, 11 Jul 2021 at 21:34, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hello, > > On Fri, Jul 09, 2021 at 04:04:22PM +0100, Kieran Bingham wrote: > > On 07/07/2021 14:40, David Plowman wrote: > > > The configuration (camera tuning) file used by the Raspberry Pi comes > > > by default from the sensor name. However, we now allow this to be > > > overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable. > > > > Some how I had it in my head that this would be an override occurring in > > the IPA itself, but I think this is ok too. > > I like it better in the pipeline handler, as the IPA module isolation > mechanisms may limit the ability to pass environmen variables. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++-- > > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 082eb1ee..a738770a 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -1232,8 +1232,18 @@ int > RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) > > > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > > > ipa_->setDelayedControls.connect(this, > &RPiCameraData::setDelayedControls); > > > > > > - IPASettings settings(ipa_->configurationFile(sensor_->model() + > ".json"), > > > - sensor_->model()); > > > + /* > > > + * The configuration (tuning file) is made from the sensor name > unless > > > + * the environment variable overrides it. > > > + */ > > > + std::string configurationFile; > > > + char const *configFromEnv = > utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE"); > > > > We likely need to document this somewhere... > > > > We have Documentation/environment_variables.rst > > > > But that could be a follow up patch > > > > > + if (!configFromEnv || *configFromEnv == '\0') > > > + configurationFile = > ipa_->configurationFile(sensor_->model() + ".json"); > > > > I'm not a particular fan of the current ipa_->configurationFile > > implementation, and I think I see that it should have changes in the > future. > > > > If this were to be generalised, then the override should go there - but > > given that this is quite specific to RPi needs right now - I think this > > is ok. > > I'd like to see this being generalized indeed. We'll have to figure out > how to specific a per-camera configuration file though. > > > However I could envisage that in the future this environment variable > > gets deprecated, but I don't yet know what would replace it - so I > > believe it's worth while to have this for now. > > That's the important question: David, if we later create a more generic > mechanism to select a different IPA configuration file, will it be fine > to drop support for LIBCAMERA_RPI_TUNING_FILE ? Yes, I think that should be fine. I assume any new mechanism would look like a string parameter passed into the pipeline handler directly through the application, hence able to be set on the command line. Regards, Naush > If so, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > + else > > > + configurationFile = std::string(configFromEnv); > > > + > > > + IPASettings settings(configurationFile, sensor_->model()); > > > > > > return ipa_->init(settings, sensorConfig); > > > } > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Mon, Jul 12, 2021 at 02:46:48PM +0100, Naushir Patuck wrote: > On Sun, 11 Jul 2021 at 21:34, Laurent Pinchart wrote: > > On Fri, Jul 09, 2021 at 04:04:22PM +0100, Kieran Bingham wrote: > > > On 07/07/2021 14:40, David Plowman wrote: > > > > The configuration (camera tuning) file used by the Raspberry Pi comes > > > > by default from the sensor name. However, we now allow this to be > > > > overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable. > > > > > > Some how I had it in my head that this would be an override occurring in > > > the IPA itself, but I think this is ok too. > > > > I like it better in the pipeline handler, as the IPA module isolation > > mechanisms may limit the ability to pass environmen variables. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++-- > > > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index 082eb1ee..a738770a 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -1232,8 +1232,18 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) > > > > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > > > > ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls); > > > > > > > > - IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"), > > > > - sensor_->model()); > > > > + /* > > > > + * The configuration (tuning file) is made from the sensor name unless > > > > + * the environment variable overrides it. > > > > + */ > > > > + std::string configurationFile; > > > > + char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE"); > > > > > > We likely need to document this somewhere... > > > > > > We have Documentation/environment_variables.rst > > > > > > But that could be a follow up patch > > > > > > > + if (!configFromEnv || *configFromEnv == '\0') > > > > + configurationFile = ipa_->configurationFile(sensor_->model() + ".json"); > > > > > > I'm not a particular fan of the current ipa_->configurationFile > > > implementation, and I think I see that it should have changes in the future. > > > > > > If this were to be generalised, then the override should go there - but > > > given that this is quite specific to RPi needs right now - I think this > > > is ok. > > > > I'd like to see this being generalized indeed. We'll have to figure out > > how to specific a per-camera configuration file though. > > > > > However I could envisage that in the future this environment variable > > > gets deprecated, but I don't yet know what would replace it - so I > > > believe it's worth while to have this for now. > > > > That's the important question: David, if we later create a more generic > > mechanism to select a different IPA configuration file, will it be fine > > to drop support for LIBCAMERA_RPI_TUNING_FILE ? > > Yes, I think that should be fine. I assume any new mechanism would look > like a string parameter passed into the pipeline handler directly through > the application, hence able to be set on the command line. I'm not sure yet what form that mechanism will take. An API that can be called by applications is certainly tempting. In any case, it will have to support per-camera configuration files, as a pipeline handler can expose multiple cameras. > > If so, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > + else > > > > + configurationFile = std::string(configFromEnv); > > > > + > > > > + IPASettings settings(configurationFile, sensor_->model()); > > > > > > > > return ipa_->init(settings, sensorConfig); > > > > }
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 082eb1ee..a738770a 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1232,8 +1232,18 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls); - IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"), - sensor_->model()); + /* + * The configuration (tuning file) is made from the sensor name unless + * the environment variable overrides it. + */ + std::string configurationFile; + char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE"); + if (!configFromEnv || *configFromEnv == '\0') + configurationFile = ipa_->configurationFile(sensor_->model() + ".json"); + else + configurationFile = std::string(configFromEnv); + + IPASettings settings(configurationFile, sensor_->model()); return ipa_->init(settings, sensorConfig); }
The configuration (camera tuning) file used by the Raspberry Pi comes by default from the sensor name. However, we now allow this to be overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)