| Message ID | 20210913102007.2303225-1-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for your patch. On Mon, 13 Sept 2021 at 11:20, Paul Elder <paul.elder@ideasonboard.com> wrote: > Promote NoiseReductionMode to a non-draft control. Upgrade the value > descriptions. Update the raspberrypi IPA accordingly, as it is the only > current user of this control. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/ipa/raspberrypi.h | 2 +- > src/android/camera_capabilities.cpp | 2 +- > src/ipa/raspberrypi/raspberrypi.cpp | 12 ++--- > src/libcamera/control_ids.yaml | 73 +++++++++++++++++------------ > 4 files changed, 52 insertions(+), 37 deletions(-) > > diff --git a/include/libcamera/ipa/raspberrypi.h > b/include/libcamera/ipa/raspberrypi.h > index 521eaecd..e0dc6f5e 100644 > --- a/include/libcamera/ipa/raspberrypi.h > +++ b/include/libcamera/ipa/raspberrypi.h > @@ -45,7 +45,7 @@ static const ControlInfoMap Controls({ > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, > 16.0f) }, > { &controls::ScalerCrop, ControlInfo(Rectangle{}, > Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > { &controls::FrameDurationLimits, > ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, > - { &controls::draft::NoiseReductionMode, > ControlInfo(controls::draft::NoiseReductionModeValues) } > + { &controls::NoiseReductionMode, > ControlInfo(controls::NoiseReductionModeValues) } > }, controls::controls); > > } /* namespace RPi */ > diff --git a/src/android/camera_capabilities.cpp > b/src/android/camera_capabilities.cpp > index e92bca42..08e44a1a 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -1175,7 +1175,7 @@ int CameraCapabilities::initializeStaticMetadata() > { > std::vector<uint8_t> data; > data.reserve(5); > - const auto &infoMap = > controlsInfo.find(&controls::draft::NoiseReductionMode); > + const auto &infoMap = > controlsInfo.find(&controls::NoiseReductionMode); > if (infoMap != controlsInfo.end()) { > for (const auto &value : infoMap->second.values()) > data.push_back(value.get<int32_t>()); > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > index 047123ab..8d44ab0a 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -605,11 +605,11 @@ static const std::map<int32_t, std::string> > AwbModeTable = { > }; > > static const std::map<int32_t, RPiController::DenoiseMode> > DenoiseModeTable = { > - { controls::draft::NoiseReductionModeOff, > RPiController::DenoiseMode::Off }, > - { controls::draft::NoiseReductionModeFast, > RPiController::DenoiseMode::ColourFast }, > - { controls::draft::NoiseReductionModeHighQuality, > RPiController::DenoiseMode::ColourHighQuality }, > - { controls::draft::NoiseReductionModeMinimal, > RPiController::DenoiseMode::ColourOff }, > - { controls::draft::NoiseReductionModeZSL, > RPiController::DenoiseMode::ColourHighQuality }, > + { controls::NoiseReductionModeOff, RPiController::DenoiseMode::Off > }, > + { controls::NoiseReductionModeFast, > RPiController::DenoiseMode::ColourFast }, > + { controls::NoiseReductionModeHighQuality, > RPiController::DenoiseMode::ColourHighQuality }, > + { controls::NoiseReductionModeMinimal, > RPiController::DenoiseMode::ColourOff }, > + { controls::NoiseReductionModeZSL, > RPiController::DenoiseMode::ColourHighQuality }, > }; > > void IPARPi::queueRequest(const ControlList &controls) > @@ -898,7 +898,7 @@ void IPARPi::queueRequest(const ControlList &controls) > * analysis image resolution or format > mismatch, we should > * report the status correctly in the > metadata. > */ > - > libcameraMetadata_.set(controls::draft::NoiseReductionMode, idx); > + > libcameraMetadata_.set(controls::NoiseReductionMode, idx); > } else { > LOG(IPARPI, Error) << "Noise reduction > mode " << idx > << " not recognised"; > diff --git a/src/libcamera/control_ids.yaml > b/src/libcamera/control_ids.yaml > index 9d4638ae..fffcca2d 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -381,6 +381,50 @@ controls: > \todo Define how the sensor timestamp has to be used in the > reprocessing > use case. > > + - NoiseReductionMode: > + type: int32_t > + description: | > + Mode of operation for the noise reduction algorithm. > + > + The noise reduction algorithm attempts to improve image quality by > + removing excessive noise added by the capture process, especially > in > + dark conditions. > + enum: > + - name: NoiseReductionModeOff > + value: 0 > + description: | > + No noise reduction will be applied by the camera device, for > + both the raw and YUV domains. > + - name: NoiseReductionModeFast > + value: 1 > + description: | > + Noise reduction is applied without reducing the frame rate. > + This may be the same as Raw if it is listed, or the same as > Off if > + any noise reduction will slow down the frame rate. > I wonder if the stated requirement of "without reducing the framerate" is a bit strict here? There are certainly going to be cases, e.g. 60/120fps recording, where our implementation will drop frames and be unable to keep up. Perhaps this should be worded as: "Noise reduction is applied with minimal possible impact on the frame rate." > + - name: NoiseReductionModeHighQuality > + value: 2 > + description: | > + High quality noise reduction at the expense of frame rate. > + - name: NoiseReductionModeRaw > + value: 3 > + description: | > + Only sensor raw domain basic noise reduction is enabled, to > remove > + demosaicing or other processing artifacts. Frame rate will > not be > + reduced. > Similar to the above comment, perhaps there may be an impact on the framerate as well. > + - name: NoiseReductionModeZSL > + value: 4 > + description: | > + Noise reduction is applied at different levels to different > streams. > + > + ZSL is meant to be used by applications that maintain a > continuous > + circular buffer of high-resolution images during preview and > + reprocess image(s) from that buffer into a final capture when > + triggered by the user. In this mode, the camera device applies > + noise reduction to low-resolution streams (below maximum > recording > + resolution) to maximize preview quality, but does not apply > noise > + reduction to high-resolution streams, since those will be > + reprocessed later if necessary. > I know that this is following the Android API, but given that libcamera does not have reprocessing capabilities just yet, should this mode be left out for now? Regards, Naush > + > # > ---------------------------------------------------------------------------- > # Draft controls section > > @@ -427,35 +471,6 @@ controls: > The camera will cancel any active trigger and the AF routine > is > reset to its initial state. > > - - NoiseReductionMode: > - type: int32_t > - draft: true > - description: | > - Control to select the noise reduction algorithm mode. Currently > - identical to ANDROID_NOISE_REDUCTION_MODE. > - > - Mode of operation for the noise reduction algorithm. > - enum: > - - name: NoiseReductionModeOff > - value: 0 > - description: No noise reduction is applied > - - name: NoiseReductionModeFast > - value: 1 > - description: | > - Noise reduction is applied without reducing the frame rate. > - - name: NoiseReductionModeHighQuality > - value: 2 > - description: | > - High quality noise reduction at the expense of frame rate. > - - name: NoiseReductionModeMinimal > - value: 3 > - description: | > - Minimal noise reduction is applied without reducing the frame > rate. > - - name: NoiseReductionModeZSL > - value: 4 > - description: | > - Noise reduction is applied at different levels to different > streams. > - > - ColorCorrectionAberrationMode: > type: int32_t > draft: true > -- > 2.27.0 > >
Hi Naush, Thank you for the feedback. On Mon, Sep 13, 2021 at 11:31:39AM +0100, Naushir Patuck wrote: > Hi Paul, > > Thank you for your patch. > > On Mon, 13 Sept 2021 at 11:20, Paul Elder <paul.elder@ideasonboard.com> wrote: > > Promote NoiseReductionMode to a non-draft control. Upgrade the value > descriptions. Update the raspberrypi IPA accordingly, as it is the only > current user of this control. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/ipa/raspberrypi.h | 2 +- > src/android/camera_capabilities.cpp | 2 +- > src/ipa/raspberrypi/raspberrypi.cpp | 12 ++--- > src/libcamera/control_ids.yaml | 73 +++++++++++++++++------------ > 4 files changed, 52 insertions(+), 37 deletions(-) > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/ > raspberrypi.h > index 521eaecd..e0dc6f5e 100644 > --- a/include/libcamera/ipa/raspberrypi.h > +++ b/include/libcamera/ipa/raspberrypi.h > @@ -45,7 +45,7 @@ static const ControlInfoMap Controls({ > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, > 16.0f) }, > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle > (65535, 65535, 65535, 65535), Rectangle{}) }, > { &controls::FrameDurationLimits, ControlInfo(INT64_C > (1000), INT64_C(1000000000)) }, > - { &controls::draft::NoiseReductionMode, ControlInfo > (controls::draft::NoiseReductionModeValues) } > + { &controls::NoiseReductionMode, ControlInfo > (controls::NoiseReductionModeValues) } > }, controls::controls); > > } /* namespace RPi */ > diff --git a/src/android/camera_capabilities.cpp b/src/android/ > camera_capabilities.cpp > index e92bca42..08e44a1a 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -1175,7 +1175,7 @@ int CameraCapabilities::initializeStaticMetadata() > { > std::vector<uint8_t> data; > data.reserve(5); > - const auto &infoMap = controlsInfo.find(& > controls::draft::NoiseReductionMode); > + const auto &infoMap = controlsInfo.find(& > controls::NoiseReductionMode); > if (infoMap != controlsInfo.end()) { > for (const auto &value : infoMap->second.values()) > data.push_back(value.get<int32_t>()); > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/ > raspberrypi.cpp > index 047123ab..8d44ab0a 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -605,11 +605,11 @@ static const std::map<int32_t, std::string> > AwbModeTable = { > }; > > static const std::map<int32_t, RPiController::DenoiseMode> > DenoiseModeTable = { > - { controls::draft::NoiseReductionModeOff, > RPiController::DenoiseMode::Off }, > - { controls::draft::NoiseReductionModeFast, > RPiController::DenoiseMode::ColourFast }, > - { controls::draft::NoiseReductionModeHighQuality, > RPiController::DenoiseMode::ColourHighQuality }, > - { controls::draft::NoiseReductionModeMinimal, > RPiController::DenoiseMode::ColourOff }, > - { controls::draft::NoiseReductionModeZSL, > RPiController::DenoiseMode::ColourHighQuality }, > + { controls::NoiseReductionModeOff, RPiController::DenoiseMode::Off > }, > + { controls::NoiseReductionModeFast, > RPiController::DenoiseMode::ColourFast }, > + { controls::NoiseReductionModeHighQuality, > RPiController::DenoiseMode::ColourHighQuality }, > + { controls::NoiseReductionModeMinimal, > RPiController::DenoiseMode::ColourOff }, > + { controls::NoiseReductionModeZSL, > RPiController::DenoiseMode::ColourHighQuality }, > }; > > void IPARPi::queueRequest(const ControlList &controls) > @@ -898,7 +898,7 @@ void IPARPi::queueRequest(const ControlList &controls) > * analysis image resolution or format > mismatch, we should > * report the status correctly in the > metadata. > */ > - libcameraMetadata_.set > (controls::draft::NoiseReductionMode, idx); > + libcameraMetadata_.set > (controls::NoiseReductionMode, idx); > } else { > LOG(IPARPI, Error) << "Noise reduction mode > " << idx > << " not recognised"; > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/ > control_ids.yaml > index 9d4638ae..fffcca2d 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -381,6 +381,50 @@ controls: > \todo Define how the sensor timestamp has to be used in the > reprocessing > use case. > > + - NoiseReductionMode: > + type: int32_t > + description: | > + Mode of operation for the noise reduction algorithm. > + > + The noise reduction algorithm attempts to improve image quality by > + removing excessive noise added by the capture process, especially > in > + dark conditions. > + enum: > + - name: NoiseReductionModeOff > + value: 0 > + description: | > + No noise reduction will be applied by the camera device, for > + both the raw and YUV domains. > + - name: NoiseReductionModeFast > + value: 1 > + description: | > + Noise reduction is applied without reducing the frame rate. > + This may be the same as Raw if it is listed, or the same as > Off if > + any noise reduction will slow down the frame rate. > > > I wonder if the stated requirement of "without reducing the framerate" is a bit > strict here? > There are certainly going to be cases, e.g. 60/120fps recording, where our > implementation will > drop frames and be unable to keep up. Perhaps this should be worded as: > "Noise reduction is applied with minimal possible impact on the frame rate." On one hand, I think the application will want to select whether: - it wants to prioritize fps over nr (if you can't suply nr fast enough, then just disable it) - or vice versa (I want nr, it's okay to slow it down as long as I get my nr) On the other hand, I see what you mean, that normally the camera would be able to provide normal quality nr, except when the frame rate is too high. Since we don't have to copy android exactly, we could have some sort of mapping from frame rate to available nr modes, or how much the frame rate will fall at what quality level. Though I think that's overkill. The way that I read the android docs (which I've also copied into the definition above) is that when the application selects fast mode, it prioritizes the frame rate, so if the camera can't provide it without dropping the frame rate, then just disable the nr. The application has specified that it'd rather have the frame rate than the nr. If it wanted the nr over the frame rate, it would've chosen hq instead of fast. Or perhaps we could simply have an in-between mode? But I don't see much point in it unless there's some way to report to the application how much the frame reduction is (which I suppose we could report in a new control?). Is there any value in this? I feel like fast and hq is enough to cover the two anticipated "desires" of applications. Do you know any examples of use cases for an application that's okay with lowered frame rate "but only up to a certain amount"? > > > + - name: NoiseReductionModeHighQuality > + value: 2 > + description: | > + High quality noise reduction at the expense of frame rate. > + - name: NoiseReductionModeRaw > + value: 3 > + description: | > + Only sensor raw domain basic noise reduction is enabled, to > remove > + demosaicing or other processing artifacts. Frame rate will not > be > + reduced. > > > Similar to the above comment, perhaps there may be an impact on the framerate > as well. > > > > + - name: NoiseReductionModeZSL > + value: 4 > + description: | > + Noise reduction is applied at different levels to different > streams. > + > + ZSL is meant to be used by applications that maintain a > continuous > + circular buffer of high-resolution images during preview and > + reprocess image(s) from that buffer into a final capture when > + triggered by the user. In this mode, the camera device applies > + noise reduction to low-resolution streams (below maximum > recording > + resolution) to maximize preview quality, but does not apply > noise > + reduction to high-resolution streams, since those will be > + reprocessed later if necessary. > > > I know that this is following the Android API, but given that libcamera does > not > have reprocessing capabilities just yet, should this mode be left out for now? Yeah, probably. Thanks, Paul > > > > + > # > ---------------------------------------------------------------------------- > # Draft controls section > > @@ -427,35 +471,6 @@ controls: > The camera will cancel any active trigger and the AF routine > is > reset to its initial state. > > - - NoiseReductionMode: > - type: int32_t > - draft: true > - description: | > - Control to select the noise reduction algorithm mode. Currently > - identical to ANDROID_NOISE_REDUCTION_MODE. > - > - Mode of operation for the noise reduction algorithm. > - enum: > - - name: NoiseReductionModeOff > - value: 0 > - description: No noise reduction is applied > - - name: NoiseReductionModeFast > - value: 1 > - description: | > - Noise reduction is applied without reducing the frame rate. > - - name: NoiseReductionModeHighQuality > - value: 2 > - description: | > - High quality noise reduction at the expense of frame rate. > - - name: NoiseReductionModeMinimal > - value: 3 > - description: | > - Minimal noise reduction is applied without reducing the frame > rate. > - - name: NoiseReductionModeZSL > - value: 4 > - description: | > - Noise reduction is applied at different levels to different > streams. > - > - ColorCorrectionAberrationMode: > type: int32_t > draft: true > -- > 2.27.0 > >
Hi Paul, On Tue, 14 Sept 2021 at 06:08, <paul.elder@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the feedback. > > On Mon, Sep 13, 2021 at 11:31:39AM +0100, Naushir Patuck wrote: > > Hi Paul, > > > > Thank you for your patch. > > > > On Mon, 13 Sept 2021 at 11:20, Paul Elder <paul.elder@ideasonboard.com> > wrote: > > > > Promote NoiseReductionMode to a non-draft control. Upgrade the value > > descriptions. Update the raspberrypi IPA accordingly, as it is the > only > > current user of this control. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > include/libcamera/ipa/raspberrypi.h | 2 +- > > src/android/camera_capabilities.cpp | 2 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 12 ++--- > > src/libcamera/control_ids.yaml | 73 > +++++++++++++++++------------ > > 4 files changed, 52 insertions(+), 37 deletions(-) > > > > diff --git a/include/libcamera/ipa/raspberrypi.h > b/include/libcamera/ipa/ > > raspberrypi.h > > index 521eaecd..e0dc6f5e 100644 > > --- a/include/libcamera/ipa/raspberrypi.h > > +++ b/include/libcamera/ipa/raspberrypi.h > > @@ -45,7 +45,7 @@ static const ControlInfoMap Controls({ > > { &controls::ColourCorrectionMatrix, > ControlInfo(-16.0f, > > 16.0f) }, > > { &controls::ScalerCrop, ControlInfo(Rectangle{}, > Rectangle > > (65535, 65535, 65535, 65535), Rectangle{}) }, > > { &controls::FrameDurationLimits, ControlInfo(INT64_C > > (1000), INT64_C(1000000000)) }, > > - { &controls::draft::NoiseReductionMode, ControlInfo > > (controls::draft::NoiseReductionModeValues) } > > + { &controls::NoiseReductionMode, ControlInfo > > (controls::NoiseReductionModeValues) } > > }, controls::controls); > > > > } /* namespace RPi */ > > diff --git a/src/android/camera_capabilities.cpp b/src/android/ > > camera_capabilities.cpp > > index e92bca42..08e44a1a 100644 > > --- a/src/android/camera_capabilities.cpp > > +++ b/src/android/camera_capabilities.cpp > > @@ -1175,7 +1175,7 @@ int > CameraCapabilities::initializeStaticMetadata() > > { > > std::vector<uint8_t> data; > > data.reserve(5); > > - const auto &infoMap = controlsInfo.find(& > > controls::draft::NoiseReductionMode); > > + const auto &infoMap = controlsInfo.find(& > > controls::NoiseReductionMode); > > if (infoMap != controlsInfo.end()) { > > for (const auto &value : > infoMap->second.values()) > > data.push_back(value.get<int32_t>()); > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/ > > raspberrypi.cpp > > index 047123ab..8d44ab0a 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -605,11 +605,11 @@ static const std::map<int32_t, std::string> > > AwbModeTable = { > > }; > > > > static const std::map<int32_t, RPiController::DenoiseMode> > > DenoiseModeTable = { > > - { controls::draft::NoiseReductionModeOff, > > RPiController::DenoiseMode::Off }, > > - { controls::draft::NoiseReductionModeFast, > > RPiController::DenoiseMode::ColourFast }, > > - { controls::draft::NoiseReductionModeHighQuality, > > RPiController::DenoiseMode::ColourHighQuality }, > > - { controls::draft::NoiseReductionModeMinimal, > > RPiController::DenoiseMode::ColourOff }, > > - { controls::draft::NoiseReductionModeZSL, > > RPiController::DenoiseMode::ColourHighQuality }, > > + { controls::NoiseReductionModeOff, > RPiController::DenoiseMode::Off > > }, > > + { controls::NoiseReductionModeFast, > > RPiController::DenoiseMode::ColourFast }, > > + { controls::NoiseReductionModeHighQuality, > > RPiController::DenoiseMode::ColourHighQuality }, > > + { controls::NoiseReductionModeMinimal, > > RPiController::DenoiseMode::ColourOff }, > > + { controls::NoiseReductionModeZSL, > > RPiController::DenoiseMode::ColourHighQuality }, > > }; > > > > void IPARPi::queueRequest(const ControlList &controls) > > @@ -898,7 +898,7 @@ void IPARPi::queueRequest(const ControlList > &controls) > > * analysis image resolution or > format > > mismatch, we should > > * report the status correctly in the > > metadata. > > */ > > - libcameraMetadata_.set > > (controls::draft::NoiseReductionMode, idx); > > + libcameraMetadata_.set > > (controls::NoiseReductionMode, idx); > > } else { > > LOG(IPARPI, Error) << "Noise > reduction mode > > " << idx > > << " not > recognised"; > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/ > > control_ids.yaml > > index 9d4638ae..fffcca2d 100644 > > --- a/src/libcamera/control_ids.yaml > > +++ b/src/libcamera/control_ids.yaml > > @@ -381,6 +381,50 @@ controls: > > \todo Define how the sensor timestamp has to be used in the > > reprocessing > > use case. > > > > + - NoiseReductionMode: > > + type: int32_t > > + description: | > > + Mode of operation for the noise reduction algorithm. > > + > > + The noise reduction algorithm attempts to improve image > quality by > > + removing excessive noise added by the capture process, > especially > > in > > + dark conditions. > > + enum: > > + - name: NoiseReductionModeOff > > + value: 0 > > + description: | > > + No noise reduction will be applied by the camera > device, for > > + both the raw and YUV domains. > > + - name: NoiseReductionModeFast > > + value: 1 > > + description: | > > + Noise reduction is applied without reducing the frame > rate. > > + This may be the same as Raw if it is listed, or the > same as > > Off if > > + any noise reduction will slow down the frame rate. > > > > > > I wonder if the stated requirement of "without reducing the framerate" > is a bit > > strict here? > > There are certainly going to be cases, e.g. 60/120fps recording, where > our > > implementation will > > drop frames and be unable to keep up. Perhaps this should be worded as: > > "Noise reduction is applied with minimal possible impact on the frame > rate." > > On one hand, I think the application will want to select whether: > - it wants to prioritize fps over nr (if you can't suply nr fast enough, > then just disable it) > - or vice versa (I want nr, it's okay to slow it down as long as I get > my nr) > > On the other hand, I see what you mean, that normally the camera would > be able to provide normal quality nr, except when the frame rate is too > high. > > Since we don't have to copy android exactly, we could have some sort of > mapping from frame rate to available nr modes, or how much the frame > rate will fall at what quality level. > > Though I think that's overkill. The way that I read the android docs > (which I've also copied into the definition above) is that when the > application selects fast mode, it prioritizes the frame rate, so if the > camera can't provide it without dropping the frame rate, then just > disable the nr. The application has specified that it'd rather have the > frame rate than the nr. If it wanted the nr over the frame rate, it > would've chosen hq instead of fast. > Agree, I don't think we need to take any steps to link framerate with this control. The only thing I would suggest is a small change in the wording to remove any frame rate guarantees. Thanks, Naush > > Or perhaps we could simply have an in-between mode? But I don't see much > point in it unless there's some way to report to the application how > much the frame reduction is (which I suppose we could report in a new > control?). Is there any value in this? I feel like fast and hq is enough > to cover the two anticipated "desires" of applications. Do you know any > examples of use cases for an application that's okay with lowered frame > rate "but only up to a certain amount"? > > > > > > > + - name: NoiseReductionModeHighQuality > > + value: 2 > > + description: | > > + High quality noise reduction at the expense of frame > rate. > > + - name: NoiseReductionModeRaw > > + value: 3 > > + description: | > > + Only sensor raw domain basic noise reduction is > enabled, to > > remove > > + demosaicing or other processing artifacts. Frame rate > will not > > be > > + reduced. > > > > > > Similar to the above comment, perhaps there may be an impact on the > framerate > > as well. > > > > > > > > + - name: NoiseReductionModeZSL > > + value: 4 > > + description: | > > + Noise reduction is applied at different levels to > different > > streams. > > + > > + ZSL is meant to be used by applications that maintain a > > continuous > > + circular buffer of high-resolution images during > preview and > > + reprocess image(s) from that buffer into a final > capture when > > + triggered by the user. In this mode, the camera device > applies > > + noise reduction to low-resolution streams (below maximum > > recording > > + resolution) to maximize preview quality, but does not > apply > > noise > > + reduction to high-resolution streams, since those will > be > > + reprocessed later if necessary. > > > > > > I know that this is following the Android API, but given that libcamera > does > > not > > have reprocessing capabilities just yet, should this mode be left out > for now? > > Yeah, probably. > > > Thanks, > > Paul > > > > > > > > > + > > # > > > ---------------------------------------------------------------------------- > > # Draft controls section > > > > @@ -427,35 +471,6 @@ controls: > > The camera will cancel any active trigger and the AF > routine > > is > > reset to its initial state. > > > > - - NoiseReductionMode: > > - type: int32_t > > - draft: true > > - description: | > > - Control to select the noise reduction algorithm mode. > Currently > > - identical to ANDROID_NOISE_REDUCTION_MODE. > > - > > - Mode of operation for the noise reduction algorithm. > > - enum: > > - - name: NoiseReductionModeOff > > - value: 0 > > - description: No noise reduction is applied > > - - name: NoiseReductionModeFast > > - value: 1 > > - description: | > > - Noise reduction is applied without reducing the frame > rate. > > - - name: NoiseReductionModeHighQuality > > - value: 2 > > - description: | > > - High quality noise reduction at the expense of frame > rate. > > - - name: NoiseReductionModeMinimal > > - value: 3 > > - description: | > > - Minimal noise reduction is applied without reducing the > frame > > rate. > > - - name: NoiseReductionModeZSL > > - value: 4 > > - description: | > > - Noise reduction is applied at different levels to > different > > streams. > > - > > - ColorCorrectionAberrationMode: > > type: int32_t > > draft: true > > -- > > 2.27.0 > > > > >
diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h index 521eaecd..e0dc6f5e 100644 --- a/include/libcamera/ipa/raspberrypi.h +++ b/include/libcamera/ipa/raspberrypi.h @@ -45,7 +45,7 @@ static const ControlInfoMap Controls({ { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, - { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) } + { &controls::NoiseReductionMode, ControlInfo(controls::NoiseReductionModeValues) } }, controls::controls); } /* namespace RPi */ diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp index e92bca42..08e44a1a 100644 --- a/src/android/camera_capabilities.cpp +++ b/src/android/camera_capabilities.cpp @@ -1175,7 +1175,7 @@ int CameraCapabilities::initializeStaticMetadata() { std::vector<uint8_t> data; data.reserve(5); - const auto &infoMap = controlsInfo.find(&controls::draft::NoiseReductionMode); + const auto &infoMap = controlsInfo.find(&controls::NoiseReductionMode); if (infoMap != controlsInfo.end()) { for (const auto &value : infoMap->second.values()) data.push_back(value.get<int32_t>()); diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 047123ab..8d44ab0a 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -605,11 +605,11 @@ static const std::map<int32_t, std::string> AwbModeTable = { }; static const std::map<int32_t, RPiController::DenoiseMode> DenoiseModeTable = { - { controls::draft::NoiseReductionModeOff, RPiController::DenoiseMode::Off }, - { controls::draft::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast }, - { controls::draft::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality }, - { controls::draft::NoiseReductionModeMinimal, RPiController::DenoiseMode::ColourOff }, - { controls::draft::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality }, + { controls::NoiseReductionModeOff, RPiController::DenoiseMode::Off }, + { controls::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast }, + { controls::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality }, + { controls::NoiseReductionModeMinimal, RPiController::DenoiseMode::ColourOff }, + { controls::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality }, }; void IPARPi::queueRequest(const ControlList &controls) @@ -898,7 +898,7 @@ void IPARPi::queueRequest(const ControlList &controls) * analysis image resolution or format mismatch, we should * report the status correctly in the metadata. */ - libcameraMetadata_.set(controls::draft::NoiseReductionMode, idx); + libcameraMetadata_.set(controls::NoiseReductionMode, idx); } else { LOG(IPARPI, Error) << "Noise reduction mode " << idx << " not recognised"; diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml index 9d4638ae..fffcca2d 100644 --- a/src/libcamera/control_ids.yaml +++ b/src/libcamera/control_ids.yaml @@ -381,6 +381,50 @@ controls: \todo Define how the sensor timestamp has to be used in the reprocessing use case. + - NoiseReductionMode: + type: int32_t + description: | + Mode of operation for the noise reduction algorithm. + + The noise reduction algorithm attempts to improve image quality by + removing excessive noise added by the capture process, especially in + dark conditions. + enum: + - name: NoiseReductionModeOff + value: 0 + description: | + No noise reduction will be applied by the camera device, for + both the raw and YUV domains. + - name: NoiseReductionModeFast + value: 1 + description: | + Noise reduction is applied without reducing the frame rate. + This may be the same as Raw if it is listed, or the same as Off if + any noise reduction will slow down the frame rate. + - name: NoiseReductionModeHighQuality + value: 2 + description: | + High quality noise reduction at the expense of frame rate. + - name: NoiseReductionModeRaw + value: 3 + description: | + Only sensor raw domain basic noise reduction is enabled, to remove + demosaicing or other processing artifacts. Frame rate will not be + reduced. + - name: NoiseReductionModeZSL + value: 4 + description: | + Noise reduction is applied at different levels to different streams. + + ZSL is meant to be used by applications that maintain a continuous + circular buffer of high-resolution images during preview and + reprocess image(s) from that buffer into a final capture when + triggered by the user. In this mode, the camera device applies + noise reduction to low-resolution streams (below maximum recording + resolution) to maximize preview quality, but does not apply noise + reduction to high-resolution streams, since those will be + reprocessed later if necessary. + # ---------------------------------------------------------------------------- # Draft controls section @@ -427,35 +471,6 @@ controls: The camera will cancel any active trigger and the AF routine is reset to its initial state. - - NoiseReductionMode: - type: int32_t - draft: true - description: | - Control to select the noise reduction algorithm mode. Currently - identical to ANDROID_NOISE_REDUCTION_MODE. - - Mode of operation for the noise reduction algorithm. - enum: - - name: NoiseReductionModeOff - value: 0 - description: No noise reduction is applied - - name: NoiseReductionModeFast - value: 1 - description: | - Noise reduction is applied without reducing the frame rate. - - name: NoiseReductionModeHighQuality - value: 2 - description: | - High quality noise reduction at the expense of frame rate. - - name: NoiseReductionModeMinimal - value: 3 - description: | - Minimal noise reduction is applied without reducing the frame rate. - - name: NoiseReductionModeZSL - value: 4 - description: | - Noise reduction is applied at different levels to different streams. - - ColorCorrectionAberrationMode: type: int32_t draft: true
Promote NoiseReductionMode to a non-draft control. Upgrade the value descriptions. Update the raspberrypi IPA accordingly, as it is the only current user of this control. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- include/libcamera/ipa/raspberrypi.h | 2 +- src/android/camera_capabilities.cpp | 2 +- src/ipa/raspberrypi/raspberrypi.cpp | 12 ++--- src/libcamera/control_ids.yaml | 73 +++++++++++++++++------------ 4 files changed, 52 insertions(+), 37 deletions(-)