| Message ID | 20230607100054.4576-4-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2023-06-07 11:00:54) > If a metering/exposure/constraint mode is not listed in the sensor > tuning file, and a control for the missing mode is set on the agc, we > terminate the application with a fatal log message. > > Instead of this fatal termination, log a warning message and switch to > the appropriate default mode so that the application continues running. I think this will improve the user experience better, at least it goes from "Oh ... the program crashed" to "Oh the program didn't give me the image settings I expected" ... As long as the actually applied configuration is reported in the metadata, I think that's fine and reasonable - and the warnings here will help someone when they investigate the 'why'. > > Reported-on: https://github.com/raspberrypi/libcamera/issues/59 > Reported-on: https://github.com/ayufan/camera-streamer/issues/67 > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++++++++++++++++++--------- > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp > index b611157af1f0..1b05d478818e 100644 > --- a/src/ipa/rpi/controller/rpi/agc.cpp > +++ b/src/ipa/rpi/controller/rpi/agc.cpp > @@ -540,24 +540,36 @@ void Agc::housekeepConfig() > */ > if (meteringModeName_ != status_.meteringMode) { > auto it = config_.meteringModes.find(meteringModeName_); > - if (it == config_.meteringModes.end()) > - LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_; > - meteringMode_ = &it->second; > + if (it == config_.meteringModes.end()) { > + LOG(RPiAgc, Warning) << "No metering mode " << meteringModeName_ > + << ", defaulting to " << config_.defaultMeteringMode; Taking the default here seems like a good fallback. I also wonder if we should instead take the settings from uncalibrated.json. But that may be more tricky to get the data in - and would still need a fallback in case *that* couldn't be found!. So this seems like a good step forwards to me. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + meteringModeName_ = config_.defaultMeteringMode; > + meteringMode_ = &config_.meteringModes[meteringModeName_]; > + } else > + meteringMode_ = &it->second; > status_.meteringMode = meteringModeName_; > } > if (exposureModeName_ != status_.exposureMode) { > auto it = config_.exposureModes.find(exposureModeName_); > - if (it == config_.exposureModes.end()) > - LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_; > - exposureMode_ = &it->second; > + if (it == config_.exposureModes.end()) { > + LOG(RPiAgc, Warning) << "No exposure profile " << exposureModeName_ > + << ", defaulting to " << config_.defaultExposureMode; > + exposureModeName_ = config_.defaultExposureMode; > + exposureMode_ = &config_.exposureModes[exposureModeName_]; > + } else > + exposureMode_ = &it->second; > status_.exposureMode = exposureModeName_; > } > if (constraintModeName_ != status_.constraintMode) { > auto it = > config_.constraintModes.find(constraintModeName_); > - if (it == config_.constraintModes.end()) > - LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_; > - constraintMode_ = &it->second; > + if (it == config_.constraintModes.end()) { > + LOG(RPiAgc, Warning) << "No constraint list " << constraintModeName_ > + << ", defaulting to " << config_.defaultConstraintMode; > + constraintModeName_ = config_.defaultConstraintMode; > + constraintMode_ = &config_.constraintModes[constraintModeName_]; > + } else > + constraintMode_ = &it->second; > status_.constraintMode = constraintModeName_; > } > LOG(RPiAgc, Debug) << "exposureMode " > -- > 2.34.1 >
Hi Naush, Thank you for the patch. On Wed, Jun 07, 2023 at 11:00:54AM +0100, Naushir Patuck via libcamera-devel wrote: > If a metering/exposure/constraint mode is not listed in the sensor > tuning file, and a control for the missing mode is set on the agc, we > terminate the application with a fatal log message. > > Instead of this fatal termination, log a warning message and switch to > the appropriate default mode so that the application continues running. > > Reported-on: https://github.com/raspberrypi/libcamera/issues/59 > Reported-on: https://github.com/ayufan/camera-streamer/issues/67 We haven't used "Reported-on" before, and grepping in the whole Linux kernel history there's a single occurrence. The tags used in the kernel with github issues links are 1 Ref. 1 regression 1 Reported-on 1 see 1 URL 2 Issue 2 link 2 Ref 3 Buglink 3 Related 3 Reported-by 3 Resolves 5 Bug 5 References 6 Fixes 7 Reference 17 Addresses-KSPP-ID 43 See 89 Closes 158 BugLink 1426 Link We don't use BugLink and have used Link once only to refer to a mailing list entry, while we use Bug for bugzilla entries. I would vote for Bug or Link in this case. > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++++++++++++++++++--------- > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp > index b611157af1f0..1b05d478818e 100644 > --- a/src/ipa/rpi/controller/rpi/agc.cpp > +++ b/src/ipa/rpi/controller/rpi/agc.cpp > @@ -540,24 +540,36 @@ void Agc::housekeepConfig() > */ > if (meteringModeName_ != status_.meteringMode) { > auto it = config_.meteringModes.find(meteringModeName_); > - if (it == config_.meteringModes.end()) > - LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_; > - meteringMode_ = &it->second; > + if (it == config_.meteringModes.end()) { > + LOG(RPiAgc, Warning) << "No metering mode " << meteringModeName_ > + << ", defaulting to " << config_.defaultMeteringMode; > + meteringModeName_ = config_.defaultMeteringMode; > + meteringMode_ = &config_.meteringModes[meteringModeName_]; > + } else > + meteringMode_ = &it->second; Please use curly braces here and below. This can be updated when applying the patch, no need to resubmit. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > status_.meteringMode = meteringModeName_; > } > if (exposureModeName_ != status_.exposureMode) { > auto it = config_.exposureModes.find(exposureModeName_); > - if (it == config_.exposureModes.end()) > - LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_; > - exposureMode_ = &it->second; > + if (it == config_.exposureModes.end()) { > + LOG(RPiAgc, Warning) << "No exposure profile " << exposureModeName_ > + << ", defaulting to " << config_.defaultExposureMode; > + exposureModeName_ = config_.defaultExposureMode; > + exposureMode_ = &config_.exposureModes[exposureModeName_]; > + } else > + exposureMode_ = &it->second; > status_.exposureMode = exposureModeName_; > } > if (constraintModeName_ != status_.constraintMode) { > auto it = > config_.constraintModes.find(constraintModeName_); > - if (it == config_.constraintModes.end()) > - LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_; > - constraintMode_ = &it->second; > + if (it == config_.constraintModes.end()) { > + LOG(RPiAgc, Warning) << "No constraint list " << constraintModeName_ > + << ", defaulting to " << config_.defaultConstraintMode; > + constraintModeName_ = config_.defaultConstraintMode; > + constraintMode_ = &config_.constraintModes[constraintModeName_]; > + } else > + constraintMode_ = &it->second; > status_.constraintMode = constraintModeName_; > } > LOG(RPiAgc, Debug) << "exposureMode "
Quoting Laurent Pinchart via libcamera-devel (2023-06-07 16:10:59) > Hi Naush, > > Thank you for the patch. > > On Wed, Jun 07, 2023 at 11:00:54AM +0100, Naushir Patuck via libcamera-devel wrote: > > If a metering/exposure/constraint mode is not listed in the sensor > > tuning file, and a control for the missing mode is set on the agc, we > > terminate the application with a fatal log message. > > > > Instead of this fatal termination, log a warning message and switch to > > the appropriate default mode so that the application continues running. > > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/59 > > Reported-on: https://github.com/ayufan/camera-streamer/issues/67 > > We haven't used "Reported-on" before, and grepping in the whole Linux Err ... we have: 2a261d911f50 ("pipeline: raspberrypi: Iterate over all Unicam instances in match()") Other references we've used for github issues include: Reported-on: https://github.com/raspberrypi/libcamera/issues/59 Reported-on: https://github.com/ayufan/camera-streamer/issues/67 Reported-on: https://github.com/raspberrypi/libcamera/issues/44 Bug: https://github.com/raspberrypi/libcamera/issues/29 Reported-by: https://github.com/kralo Bug: https://github.com/raspberrypi/libcamera-apps/issues/217 Bug: https://github.com/raspberrypi/libcamera-apps/issues/236 Bug: https://github.com/raspberrypi/libcamera-apps/issues/238 Bug: https://github.com/raspberrypi/libcamera-apps/issues/240 See https://github.com/PyCQA/pycodestyle/issues/466 I think Bug: is the most appropriate still, so I'll use that. > kernel history there's a single occurrence. The tags used in the kernel > with github issues links are > > 1 Ref. > 1 regression > 1 Reported-on > 1 see > 1 URL > 2 Issue > 2 link > 2 Ref > 3 Buglink > 3 Related > 3 Reported-by > 3 Resolves > 5 Bug > 5 References > 6 Fixes > 7 Reference > 17 Addresses-KSPP-ID > 43 See > 89 Closes > 158 BugLink > 1426 Link > > We don't use BugLink and have used Link once only to refer to a mailing > list entry, while we use Bug for bugzilla entries. I would vote for Bug > or Link in this case. > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++++++++++++++++++--------- > > 1 file changed, 21 insertions(+), 9 deletions(-) > > > > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp > > index b611157af1f0..1b05d478818e 100644 > > --- a/src/ipa/rpi/controller/rpi/agc.cpp > > +++ b/src/ipa/rpi/controller/rpi/agc.cpp > > @@ -540,24 +540,36 @@ void Agc::housekeepConfig() > > */ > > if (meteringModeName_ != status_.meteringMode) { > > auto it = config_.meteringModes.find(meteringModeName_); > > - if (it == config_.meteringModes.end()) > > - LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_; > > - meteringMode_ = &it->second; > > + if (it == config_.meteringModes.end()) { > > + LOG(RPiAgc, Warning) << "No metering mode " << meteringModeName_ > > + << ", defaulting to " << config_.defaultMeteringMode; > > + meteringModeName_ = config_.defaultMeteringMode; > > + meteringMode_ = &config_.meteringModes[meteringModeName_]; > > + } else > > + meteringMode_ = &it->second; > > Please use curly braces here and below. This can be updated when > applying the patch, no need to resubmit. Updated, and running through the integration tests. -- Kieran > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > status_.meteringMode = meteringModeName_; > > } > > if (exposureModeName_ != status_.exposureMode) { > > auto it = config_.exposureModes.find(exposureModeName_); > > - if (it == config_.exposureModes.end()) > > - LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_; > > - exposureMode_ = &it->second; > > + if (it == config_.exposureModes.end()) { > > + LOG(RPiAgc, Warning) << "No exposure profile " << exposureModeName_ > > + << ", defaulting to " << config_.defaultExposureMode; > > + exposureModeName_ = config_.defaultExposureMode; > > + exposureMode_ = &config_.exposureModes[exposureModeName_]; > > + } else > > + exposureMode_ = &it->second; > > status_.exposureMode = exposureModeName_; > > } > > if (constraintModeName_ != status_.constraintMode) { > > auto it = > > config_.constraintModes.find(constraintModeName_); > > - if (it == config_.constraintModes.end()) > > - LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_; > > - constraintMode_ = &it->second; > > + if (it == config_.constraintModes.end()) { > > + LOG(RPiAgc, Warning) << "No constraint list " << constraintModeName_ > > + << ", defaulting to " << config_.defaultConstraintMode; > > + constraintModeName_ = config_.defaultConstraintMode; > > + constraintMode_ = &config_.constraintModes[constraintModeName_]; > > + } else > > + constraintMode_ = &it->second; > > status_.constraintMode = constraintModeName_; > > } > > LOG(RPiAgc, Debug) << "exposureMode " > > -- > Regards, > > Laurent Pinchart
Hi everyone I think this is a good idea. The only question I have is whether we might simply print a warning and do nothing, rather than change to the default mode. I think this is a slightly less surprising behaviour - if you make a mistake it has no effect, rather than change you to some possibly unrelated mode that you might even have to undo again. It's also a general rule that could be applied to all controls: ask for something that can't be done and it tells you and does nothing. Thoughts? Thanks! David On Wed, 7 Jun 2023 at 20:56, Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Quoting Laurent Pinchart via libcamera-devel (2023-06-07 16:10:59) > > Hi Naush, > > > > Thank you for the patch. > > > > On Wed, Jun 07, 2023 at 11:00:54AM +0100, Naushir Patuck via libcamera-devel wrote: > > > If a metering/exposure/constraint mode is not listed in the sensor > > > tuning file, and a control for the missing mode is set on the agc, we > > > terminate the application with a fatal log message. > > > > > > Instead of this fatal termination, log a warning message and switch to > > > the appropriate default mode so that the application continues running. > > > > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/59 > > > Reported-on: https://github.com/ayufan/camera-streamer/issues/67 > > > > We haven't used "Reported-on" before, and grepping in the whole Linux > > Err ... we have: > > 2a261d911f50 ("pipeline: raspberrypi: Iterate over all Unicam instances in match()") > > Other references we've used for github issues include: > > Reported-on: https://github.com/raspberrypi/libcamera/issues/59 > Reported-on: https://github.com/ayufan/camera-streamer/issues/67 > Reported-on: https://github.com/raspberrypi/libcamera/issues/44 > Bug: https://github.com/raspberrypi/libcamera/issues/29 > Reported-by: https://github.com/kralo > Bug: https://github.com/raspberrypi/libcamera-apps/issues/217 > Bug: https://github.com/raspberrypi/libcamera-apps/issues/236 > Bug: https://github.com/raspberrypi/libcamera-apps/issues/238 > Bug: https://github.com/raspberrypi/libcamera-apps/issues/240 > See https://github.com/PyCQA/pycodestyle/issues/466 > > I think Bug: is the most appropriate still, so I'll use that. > > > > kernel history there's a single occurrence. The tags used in the kernel > > with github issues links are > > > > 1 Ref. > > 1 regression > > 1 Reported-on > > 1 see > > 1 URL > > 2 Issue > > 2 link > > 2 Ref > > 3 Buglink > > 3 Related > > 3 Reported-by > > 3 Resolves > > 5 Bug > > 5 References > > 6 Fixes > > 7 Reference > > 17 Addresses-KSPP-ID > > 43 See > > 89 Closes > > 158 BugLink > > 1426 Link > > > > We don't use BugLink and have used Link once only to refer to a mailing > > list entry, while we use Bug for bugzilla entries. I would vote for Bug > > or Link in this case. > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++++++++++++++++++--------- > > > 1 file changed, 21 insertions(+), 9 deletions(-) > > > > > > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp > > > index b611157af1f0..1b05d478818e 100644 > > > --- a/src/ipa/rpi/controller/rpi/agc.cpp > > > +++ b/src/ipa/rpi/controller/rpi/agc.cpp > > > @@ -540,24 +540,36 @@ void Agc::housekeepConfig() > > > */ > > > if (meteringModeName_ != status_.meteringMode) { > > > auto it = config_.meteringModes.find(meteringModeName_); > > > - if (it == config_.meteringModes.end()) > > > - LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_; > > > - meteringMode_ = &it->second; > > > + if (it == config_.meteringModes.end()) { > > > + LOG(RPiAgc, Warning) << "No metering mode " << meteringModeName_ > > > + << ", defaulting to " << config_.defaultMeteringMode; > > > + meteringModeName_ = config_.defaultMeteringMode; > > > + meteringMode_ = &config_.meteringModes[meteringModeName_]; > > > + } else > > > + meteringMode_ = &it->second; > > > > Please use curly braces here and below. This can be updated when > > applying the patch, no need to resubmit. > > Updated, and running through the integration tests. > > -- > Kieran > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > status_.meteringMode = meteringModeName_; > > > } > > > if (exposureModeName_ != status_.exposureMode) { > > > auto it = config_.exposureModes.find(exposureModeName_); > > > - if (it == config_.exposureModes.end()) > > > - LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_; > > > - exposureMode_ = &it->second; > > > + if (it == config_.exposureModes.end()) { > > > + LOG(RPiAgc, Warning) << "No exposure profile " << exposureModeName_ > > > + << ", defaulting to " << config_.defaultExposureMode; > > > + exposureModeName_ = config_.defaultExposureMode; > > > + exposureMode_ = &config_.exposureModes[exposureModeName_]; > > > + } else > > > + exposureMode_ = &it->second; > > > status_.exposureMode = exposureModeName_; > > > } > > > if (constraintModeName_ != status_.constraintMode) { > > > auto it = > > > config_.constraintModes.find(constraintModeName_); > > > - if (it == config_.constraintModes.end()) > > > - LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_; > > > - constraintMode_ = &it->second; > > > + if (it == config_.constraintModes.end()) { > > > + LOG(RPiAgc, Warning) << "No constraint list " << constraintModeName_ > > > + << ", defaulting to " << config_.defaultConstraintMode; > > > + constraintModeName_ = config_.defaultConstraintMode; > > > + constraintMode_ = &config_.constraintModes[constraintModeName_]; > > > + } else > > > + constraintMode_ = &it->second; > > > status_.constraintMode = constraintModeName_; > > > } > > > LOG(RPiAgc, Debug) << "exposureMode " > > > > -- > > Regards, > > > > Laurent Pinchart
Hi David, On Mon, 12 Jun 2023 at 09:54, David Plowman <david.plowman@raspberrypi.com> wrote: > > Hi everyone > > I think this is a good idea. The only question I have is whether we > might simply print a warning and do nothing, rather than change to the > default mode. > > I think this is a slightly less surprising behaviour - if you make a > mistake it has no effect, rather than change you to some possibly > unrelated mode that you might even have to undo again. It's also a > general rule that could be applied to all controls: ask for something > that can't be done and it tells you and does nothing. > > Thoughts? I agree, it's slightly less surprising if we don't change the mode (instead of changing to default) on the error condition. This also matches the behaviour of various other control handling that we have in the IPA. I'll make the change and post it shortly. Regards, Naush > > Thanks! > David > > On Wed, 7 Jun 2023 at 20:56, Kieran Bingham via libcamera-devel > <libcamera-devel@lists.libcamera.org> wrote: > > > > Quoting Laurent Pinchart via libcamera-devel (2023-06-07 16:10:59) > > > Hi Naush, > > > > > > Thank you for the patch. > > > > > > On Wed, Jun 07, 2023 at 11:00:54AM +0100, Naushir Patuck via libcamera-devel wrote: > > > > If a metering/exposure/constraint mode is not listed in the sensor > > > > tuning file, and a control for the missing mode is set on the agc, we > > > > terminate the application with a fatal log message. > > > > > > > > Instead of this fatal termination, log a warning message and switch to > > > > the appropriate default mode so that the application continues running. > > > > > > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/59 > > > > Reported-on: https://github.com/ayufan/camera-streamer/issues/67 > > > > > > We haven't used "Reported-on" before, and grepping in the whole Linux > > > > Err ... we have: > > > > 2a261d911f50 ("pipeline: raspberrypi: Iterate over all Unicam instances in match()") > > > > Other references we've used for github issues include: > > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/59 > > Reported-on: https://github.com/ayufan/camera-streamer/issues/67 > > Reported-on: https://github.com/raspberrypi/libcamera/issues/44 > > Bug: https://github.com/raspberrypi/libcamera/issues/29 > > Reported-by: https://github.com/kralo > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/217 > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/236 > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/238 > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/240 > > See https://github.com/PyCQA/pycodestyle/issues/466 > > > > I think Bug: is the most appropriate still, so I'll use that. > > > > > > > kernel history there's a single occurrence. The tags used in the kernel > > > with github issues links are > > > > > > 1 Ref. > > > 1 regression > > > 1 Reported-on > > > 1 see > > > 1 URL > > > 2 Issue > > > 2 link > > > 2 Ref > > > 3 Buglink > > > 3 Related > > > 3 Reported-by > > > 3 Resolves > > > 5 Bug > > > 5 References > > > 6 Fixes > > > 7 Reference > > > 17 Addresses-KSPP-ID > > > 43 See > > > 89 Closes > > > 158 BugLink > > > 1426 Link > > > > > > We don't use BugLink and have used Link once only to refer to a mailing > > > list entry, while we use Bug for bugzilla entries. I would vote for Bug > > > or Link in this case. > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++++++++++++++++++--------- > > > > 1 file changed, 21 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp > > > > index b611157af1f0..1b05d478818e 100644 > > > > --- a/src/ipa/rpi/controller/rpi/agc.cpp > > > > +++ b/src/ipa/rpi/controller/rpi/agc.cpp > > > > @@ -540,24 +540,36 @@ void Agc::housekeepConfig() > > > > */ > > > > if (meteringModeName_ != status_.meteringMode) { > > > > auto it = config_.meteringModes.find(meteringModeName_); > > > > - if (it == config_.meteringModes.end()) > > > > - LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_; > > > > - meteringMode_ = &it->second; > > > > + if (it == config_.meteringModes.end()) { > > > > + LOG(RPiAgc, Warning) << "No metering mode " << meteringModeName_ > > > > + << ", defaulting to " << config_.defaultMeteringMode; > > > > + meteringModeName_ = config_.defaultMeteringMode; > > > > + meteringMode_ = &config_.meteringModes[meteringModeName_]; > > > > + } else > > > > + meteringMode_ = &it->second; > > > > > > Please use curly braces here and below. This can be updated when > > > applying the patch, no need to resubmit. > > > > Updated, and running through the integration tests. > > > > -- > > Kieran > > > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > status_.meteringMode = meteringModeName_; > > > > } > > > > if (exposureModeName_ != status_.exposureMode) { > > > > auto it = config_.exposureModes.find(exposureModeName_); > > > > - if (it == config_.exposureModes.end()) > > > > - LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_; > > > > - exposureMode_ = &it->second; > > > > + if (it == config_.exposureModes.end()) { > > > > + LOG(RPiAgc, Warning) << "No exposure profile " << exposureModeName_ > > > > + << ", defaulting to " << config_.defaultExposureMode; > > > > + exposureModeName_ = config_.defaultExposureMode; > > > > + exposureMode_ = &config_.exposureModes[exposureModeName_]; > > > > + } else > > > > + exposureMode_ = &it->second; > > > > status_.exposureMode = exposureModeName_; > > > > } > > > > if (constraintModeName_ != status_.constraintMode) { > > > > auto it = > > > > config_.constraintModes.find(constraintModeName_); > > > > - if (it == config_.constraintModes.end()) > > > > - LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_; > > > > - constraintMode_ = &it->second; > > > > + if (it == config_.constraintModes.end()) { > > > > + LOG(RPiAgc, Warning) << "No constraint list " << constraintModeName_ > > > > + << ", defaulting to " << config_.defaultConstraintMode; > > > > + constraintModeName_ = config_.defaultConstraintMode; > > > > + constraintMode_ = &config_.constraintModes[constraintModeName_]; > > > > + } else > > > > + constraintMode_ = &it->second; > > > > status_.constraintMode = constraintModeName_; > > > > } > > > > LOG(RPiAgc, Debug) << "exposureMode " > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart
On Wed, Jun 07, 2023 at 08:56:50PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart via libcamera-devel (2023-06-07 16:10:59) > > Hi Naush, > > > > Thank you for the patch. > > > > On Wed, Jun 07, 2023 at 11:00:54AM +0100, Naushir Patuck via libcamera-devel wrote: > > > If a metering/exposure/constraint mode is not listed in the sensor > > > tuning file, and a control for the missing mode is set on the agc, we > > > terminate the application with a fatal log message. > > > > > > Instead of this fatal termination, log a warning message and switch to > > > the appropriate default mode so that the application continues running. > > > > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/59 > > > Reported-on: https://github.com/ayufan/camera-streamer/issues/67 > > > > We haven't used "Reported-on" before, and grepping in the whole Linux > > Err ... we have: > > 2a261d911f50 ("pipeline: raspberrypi: Iterate over all Unicam instances in match()") Oops, indeed :-/ > Other references we've used for github issues include: > > Reported-on: https://github.com/raspberrypi/libcamera/issues/59 > Reported-on: https://github.com/ayufan/camera-streamer/issues/67 > Reported-on: https://github.com/raspberrypi/libcamera/issues/44 > Bug: https://github.com/raspberrypi/libcamera/issues/29 > Reported-by: https://github.com/kralo > Bug: https://github.com/raspberrypi/libcamera-apps/issues/217 > Bug: https://github.com/raspberrypi/libcamera-apps/issues/236 > Bug: https://github.com/raspberrypi/libcamera-apps/issues/238 > Bug: https://github.com/raspberrypi/libcamera-apps/issues/240 > See https://github.com/PyCQA/pycodestyle/issues/466 > > I think Bug: is the most appropriate still, so I'll use that. In order to avoid manual handling if similar issues in the future, I've implemented a git commit message trailers checker for checkstyle.py. I'll post patches shortly. > > kernel history there's a single occurrence. The tags used in the kernel > > with github issues links are > > > > 1 Ref. > > 1 regression > > 1 Reported-on > > 1 see > > 1 URL > > 2 Issue > > 2 link > > 2 Ref > > 3 Buglink > > 3 Related > > 3 Reported-by > > 3 Resolves > > 5 Bug > > 5 References > > 6 Fixes > > 7 Reference > > 17 Addresses-KSPP-ID > > 43 See > > 89 Closes > > 158 BugLink > > 1426 Link > > > > We don't use BugLink and have used Link once only to refer to a mailing > > list entry, while we use Bug for bugzilla entries. I would vote for Bug > > or Link in this case. > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++++++++++++++++++--------- > > > 1 file changed, 21 insertions(+), 9 deletions(-) > > > > > > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp > > > index b611157af1f0..1b05d478818e 100644 > > > --- a/src/ipa/rpi/controller/rpi/agc.cpp > > > +++ b/src/ipa/rpi/controller/rpi/agc.cpp > > > @@ -540,24 +540,36 @@ void Agc::housekeepConfig() > > > */ > > > if (meteringModeName_ != status_.meteringMode) { > > > auto it = config_.meteringModes.find(meteringModeName_); > > > - if (it == config_.meteringModes.end()) > > > - LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_; > > > - meteringMode_ = &it->second; > > > + if (it == config_.meteringModes.end()) { > > > + LOG(RPiAgc, Warning) << "No metering mode " << meteringModeName_ > > > + << ", defaulting to " << config_.defaultMeteringMode; > > > + meteringModeName_ = config_.defaultMeteringMode; > > > + meteringMode_ = &config_.meteringModes[meteringModeName_]; > > > + } else > > > + meteringMode_ = &it->second; > > > > Please use curly braces here and below. This can be updated when > > applying the patch, no need to resubmit. > > Updated, and running through the integration tests. > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > status_.meteringMode = meteringModeName_; > > > } > > > if (exposureModeName_ != status_.exposureMode) { > > > auto it = config_.exposureModes.find(exposureModeName_); > > > - if (it == config_.exposureModes.end()) > > > - LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_; > > > - exposureMode_ = &it->second; > > > + if (it == config_.exposureModes.end()) { > > > + LOG(RPiAgc, Warning) << "No exposure profile " << exposureModeName_ > > > + << ", defaulting to " << config_.defaultExposureMode; > > > + exposureModeName_ = config_.defaultExposureMode; > > > + exposureMode_ = &config_.exposureModes[exposureModeName_]; > > > + } else > > > + exposureMode_ = &it->second; > > > status_.exposureMode = exposureModeName_; > > > } > > > if (constraintModeName_ != status_.constraintMode) { > > > auto it = > > > config_.constraintModes.find(constraintModeName_); > > > - if (it == config_.constraintModes.end()) > > > - LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_; > > > - constraintMode_ = &it->second; > > > + if (it == config_.constraintModes.end()) { > > > + LOG(RPiAgc, Warning) << "No constraint list " << constraintModeName_ > > > + << ", defaulting to " << config_.defaultConstraintMode; > > > + constraintModeName_ = config_.defaultConstraintMode; > > > + constraintMode_ = &config_.constraintModes[constraintModeName_]; > > > + } else > > > + constraintMode_ = &it->second; > > > status_.constraintMode = constraintModeName_; > > > } > > > LOG(RPiAgc, Debug) << "exposureMode "
Hello, On Mon, Jun 12, 2023 at 10:15:42AM +0100, Naushir Patuck wrote: > On Mon, 12 Jun 2023 at 09:54, David Plowman wrote: > > > > Hi everyone > > > > I think this is a good idea. The only question I have is whether we > > might simply print a warning and do nothing, rather than change to the > > default mode. > > > > I think this is a slightly less surprising behaviour - if you make a > > mistake it has no effect, rather than change you to some possibly > > unrelated mode that you might even have to undo again. It's also a > > general rule that could be applied to all controls: ask for something > > that can't be done and it tells you and does nothing. > > > > Thoughts? > > I agree, it's slightly less surprising if we don't change the mode (instead of > changing to default) on the error condition. This also matches the behaviour of > various other control handling that we have in the IPA. Sounds good to me too. Thanks David for the suggestion. > I'll make the change and post it shortly. > > > On Wed, 7 Jun 2023 at 20:56, Kieran Bingham via libcamera-devel wrote: > > > Quoting Laurent Pinchart via libcamera-devel (2023-06-07 16:10:59) > > > > Hi Naush, > > > > > > > > Thank you for the patch. > > > > > > > > On Wed, Jun 07, 2023 at 11:00:54AM +0100, Naushir Patuck via libcamera-devel wrote: > > > > > If a metering/exposure/constraint mode is not listed in the sensor > > > > > tuning file, and a control for the missing mode is set on the agc, we > > > > > terminate the application with a fatal log message. > > > > > > > > > > Instead of this fatal termination, log a warning message and switch to > > > > > the appropriate default mode so that the application continues running. > > > > > > > > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/59 > > > > > Reported-on: https://github.com/ayufan/camera-streamer/issues/67 > > > > > > > > We haven't used "Reported-on" before, and grepping in the whole Linux > > > > > > Err ... we have: > > > > > > 2a261d911f50 ("pipeline: raspberrypi: Iterate over all Unicam instances in match()") > > > > > > Other references we've used for github issues include: > > > > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/59 > > > Reported-on: https://github.com/ayufan/camera-streamer/issues/67 > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/44 > > > Bug: https://github.com/raspberrypi/libcamera/issues/29 > > > Reported-by: https://github.com/kralo > > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/217 > > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/236 > > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/238 > > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/240 > > > See https://github.com/PyCQA/pycodestyle/issues/466 > > > > > > I think Bug: is the most appropriate still, so I'll use that. > > > > > > > > > > kernel history there's a single occurrence. The tags used in the kernel > > > > with github issues links are > > > > > > > > 1 Ref. > > > > 1 regression > > > > 1 Reported-on > > > > 1 see > > > > 1 URL > > > > 2 Issue > > > > 2 link > > > > 2 Ref > > > > 3 Buglink > > > > 3 Related > > > > 3 Reported-by > > > > 3 Resolves > > > > 5 Bug > > > > 5 References > > > > 6 Fixes > > > > 7 Reference > > > > 17 Addresses-KSPP-ID > > > > 43 See > > > > 89 Closes > > > > 158 BugLink > > > > 1426 Link > > > > > > > > We don't use BugLink and have used Link once only to refer to a mailing > > > > list entry, while we use Bug for bugzilla entries. I would vote for Bug > > > > or Link in this case. > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > --- > > > > > src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++++++++++++++++++--------- > > > > > 1 file changed, 21 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp > > > > > index b611157af1f0..1b05d478818e 100644 > > > > > --- a/src/ipa/rpi/controller/rpi/agc.cpp > > > > > +++ b/src/ipa/rpi/controller/rpi/agc.cpp > > > > > @@ -540,24 +540,36 @@ void Agc::housekeepConfig() > > > > > */ > > > > > if (meteringModeName_ != status_.meteringMode) { > > > > > auto it = config_.meteringModes.find(meteringModeName_); > > > > > - if (it == config_.meteringModes.end()) > > > > > - LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_; > > > > > - meteringMode_ = &it->second; > > > > > + if (it == config_.meteringModes.end()) { > > > > > + LOG(RPiAgc, Warning) << "No metering mode " << meteringModeName_ > > > > > + << ", defaulting to " << config_.defaultMeteringMode; > > > > > + meteringModeName_ = config_.defaultMeteringMode; > > > > > + meteringMode_ = &config_.meteringModes[meteringModeName_]; > > > > > + } else > > > > > + meteringMode_ = &it->second; > > > > > > > > Please use curly braces here and below. This can be updated when > > > > applying the patch, no need to resubmit. > > > > > > Updated, and running through the integration tests. > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > status_.meteringMode = meteringModeName_; > > > > > } > > > > > if (exposureModeName_ != status_.exposureMode) { > > > > > auto it = config_.exposureModes.find(exposureModeName_); > > > > > - if (it == config_.exposureModes.end()) > > > > > - LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_; > > > > > - exposureMode_ = &it->second; > > > > > + if (it == config_.exposureModes.end()) { > > > > > + LOG(RPiAgc, Warning) << "No exposure profile " << exposureModeName_ > > > > > + << ", defaulting to " << config_.defaultExposureMode; > > > > > + exposureModeName_ = config_.defaultExposureMode; > > > > > + exposureMode_ = &config_.exposureModes[exposureModeName_]; > > > > > + } else > > > > > + exposureMode_ = &it->second; > > > > > status_.exposureMode = exposureModeName_; > > > > > } > > > > > if (constraintModeName_ != status_.constraintMode) { > > > > > auto it = > > > > > config_.constraintModes.find(constraintModeName_); > > > > > - if (it == config_.constraintModes.end()) > > > > > - LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_; > > > > > - constraintMode_ = &it->second; > > > > > + if (it == config_.constraintModes.end()) { > > > > > + LOG(RPiAgc, Warning) << "No constraint list " << constraintModeName_ > > > > > + << ", defaulting to " << config_.defaultConstraintMode; > > > > > + constraintModeName_ = config_.defaultConstraintMode; > > > > > + constraintMode_ = &config_.constraintModes[constraintModeName_]; > > > > > + } else > > > > > + constraintMode_ = &it->second; > > > > > status_.constraintMode = constraintModeName_; > > > > > } > > > > > LOG(RPiAgc, Debug) << "exposureMode "
diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp index b611157af1f0..1b05d478818e 100644 --- a/src/ipa/rpi/controller/rpi/agc.cpp +++ b/src/ipa/rpi/controller/rpi/agc.cpp @@ -540,24 +540,36 @@ void Agc::housekeepConfig() */ if (meteringModeName_ != status_.meteringMode) { auto it = config_.meteringModes.find(meteringModeName_); - if (it == config_.meteringModes.end()) - LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_; - meteringMode_ = &it->second; + if (it == config_.meteringModes.end()) { + LOG(RPiAgc, Warning) << "No metering mode " << meteringModeName_ + << ", defaulting to " << config_.defaultMeteringMode; + meteringModeName_ = config_.defaultMeteringMode; + meteringMode_ = &config_.meteringModes[meteringModeName_]; + } else + meteringMode_ = &it->second; status_.meteringMode = meteringModeName_; } if (exposureModeName_ != status_.exposureMode) { auto it = config_.exposureModes.find(exposureModeName_); - if (it == config_.exposureModes.end()) - LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_; - exposureMode_ = &it->second; + if (it == config_.exposureModes.end()) { + LOG(RPiAgc, Warning) << "No exposure profile " << exposureModeName_ + << ", defaulting to " << config_.defaultExposureMode; + exposureModeName_ = config_.defaultExposureMode; + exposureMode_ = &config_.exposureModes[exposureModeName_]; + } else + exposureMode_ = &it->second; status_.exposureMode = exposureModeName_; } if (constraintModeName_ != status_.constraintMode) { auto it = config_.constraintModes.find(constraintModeName_); - if (it == config_.constraintModes.end()) - LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_; - constraintMode_ = &it->second; + if (it == config_.constraintModes.end()) { + LOG(RPiAgc, Warning) << "No constraint list " << constraintModeName_ + << ", defaulting to " << config_.defaultConstraintMode; + constraintModeName_ = config_.defaultConstraintMode; + constraintMode_ = &config_.constraintModes[constraintModeName_]; + } else + constraintMode_ = &it->second; status_.constraintMode = constraintModeName_; } LOG(RPiAgc, Debug) << "exposureMode "
If a metering/exposure/constraint mode is not listed in the sensor tuning file, and a control for the missing mode is set on the agc, we terminate the application with a fatal log message. Instead of this fatal termination, log a warning message and switch to the appropriate default mode so that the application continues running. Reported-on: https://github.com/raspberrypi/libcamera/issues/59 Reported-on: https://github.com/ayufan/camera-streamer/issues/67 Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)