<div dir="ltr"><div dir="ltr">Hi Paul,<div><br></div><div>Thank you for your patch.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 13 Sept 2021 at 11:20, Paul Elder <<a href="mailto:paul.elder@ideasonboard.com">paul.elder@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Promote NoiseReductionMode to a non-draft control. Upgrade the value<br>
descriptions. Update the raspberrypi IPA accordingly, as it is the only<br>
current user of this control.<br>
<br>
Signed-off-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>><br>
---<br>
include/libcamera/ipa/raspberrypi.h | 2 +-<br>
src/android/camera_capabilities.cpp | 2 +-<br>
src/ipa/raspberrypi/raspberrypi.cpp | 12 ++---<br>
src/libcamera/control_ids.yaml | 73 +++++++++++++++++------------<br>
4 files changed, 52 insertions(+), 37 deletions(-)<br>
<br>
diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h<br>
index 521eaecd..e0dc6f5e 100644<br>
--- a/include/libcamera/ipa/raspberrypi.h<br>
+++ b/include/libcamera/ipa/raspberrypi.h<br>
@@ -45,7 +45,7 @@ static const ControlInfoMap Controls({<br>
{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },<br>
{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },<br>
{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },<br>
- { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }<br>
+ { &controls::NoiseReductionMode, ControlInfo(controls::NoiseReductionModeValues) }<br>
}, controls::controls);<br>
<br>
} /* namespace RPi */<br>
diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp<br>
index e92bca42..08e44a1a 100644<br>
--- a/src/android/camera_capabilities.cpp<br>
+++ b/src/android/camera_capabilities.cpp<br>
@@ -1175,7 +1175,7 @@ int CameraCapabilities::initializeStaticMetadata()<br>
{<br>
std::vector<uint8_t> data;<br>
data.reserve(5);<br>
- const auto &infoMap = controlsInfo.find(&controls::draft::NoiseReductionMode);<br>
+ const auto &infoMap = controlsInfo.find(&controls::NoiseReductionMode);<br>
if (infoMap != controlsInfo.end()) {<br>
for (const auto &value : infoMap->second.values())<br>
data.push_back(value.get<int32_t>());<br>
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
index 047123ab..8d44ab0a 100644<br>
--- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
+++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
@@ -605,11 +605,11 @@ static const std::map<int32_t, std::string> AwbModeTable = {<br>
};<br>
<br>
static const std::map<int32_t, RPiController::DenoiseMode> DenoiseModeTable = {<br>
- { controls::draft::NoiseReductionModeOff, RPiController::DenoiseMode::Off },<br>
- { controls::draft::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast },<br>
- { controls::draft::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality },<br>
- { controls::draft::NoiseReductionModeMinimal, RPiController::DenoiseMode::ColourOff },<br>
- { controls::draft::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality },<br>
+ { controls::NoiseReductionModeOff, RPiController::DenoiseMode::Off },<br>
+ { controls::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast },<br>
+ { controls::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality },<br>
+ { controls::NoiseReductionModeMinimal, RPiController::DenoiseMode::ColourOff },<br>
+ { controls::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality },<br>
};<br>
<br>
void IPARPi::queueRequest(const ControlList &controls)<br>
@@ -898,7 +898,7 @@ void IPARPi::queueRequest(const ControlList &controls)<br>
* analysis image resolution or format mismatch, we should<br>
* report the status correctly in the metadata.<br>
*/<br>
- libcameraMetadata_.set(controls::draft::NoiseReductionMode, idx);<br>
+ libcameraMetadata_.set(controls::NoiseReductionMode, idx);<br>
} else {<br>
LOG(IPARPI, Error) << "Noise reduction mode " << idx<br>
<< " not recognised";<br>
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml<br>
index 9d4638ae..fffcca2d 100644<br>
--- a/src/libcamera/control_ids.yaml<br>
+++ b/src/libcamera/control_ids.yaml<br>
@@ -381,6 +381,50 @@ controls:<br>
\todo Define how the sensor timestamp has to be used in the reprocessing<br>
use case.<br>
<br>
+ - NoiseReductionMode:<br>
+ type: int32_t<br>
+ description: |<br>
+ Mode of operation for the noise reduction algorithm.<br>
+<br>
+ The noise reduction algorithm attempts to improve image quality by<br>
+ removing excessive noise added by the capture process, especially in<br>
+ dark conditions.<br>
+ enum:<br>
+ - name: NoiseReductionModeOff<br>
+ value: 0<br>
+ description: |<br>
+ No noise reduction will be applied by the camera device, for<br>
+ both the raw and YUV domains.<br>
+ - name: NoiseReductionModeFast<br>
+ value: 1<br>
+ description: |<br>
+ Noise reduction is applied without reducing the frame rate.<br>
+ This may be the same as Raw if it is listed, or the same as Off if<br>
+ any noise reduction will slow down the frame rate.<br></blockquote><div><br></div><div>I wonder if the stated requirement of "without reducing the framerate" is a bit strict here?</div><div>There are certainly going to be cases, e.g. 60/120fps recording, where our implementation will</div><div>drop frames and be unable to keep up. Perhaps this should be worded as:</div><div>"Noise reduction is applied with minimal possible impact on the frame rate."<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ - name: NoiseReductionModeHighQuality<br>
+ value: 2<br>
+ description: |<br>
+ High quality noise reduction at the expense of frame rate.<br>
+ - name: NoiseReductionModeRaw<br>
+ value: 3<br>
+ description: |<br>
+ Only sensor raw domain basic noise reduction is enabled, to remove<br>
+ demosaicing or other processing artifacts. Frame rate will not be<br>
+ reduced.<br></blockquote><div><br></div><div>Similar to the above comment, perhaps there may be an impact on the framerate as well.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ - name: NoiseReductionModeZSL<br>
+ value: 4<br>
+ description: |<br>
+ Noise reduction is applied at different levels to different streams.<br>
+<br>
+ ZSL is meant to be used by applications that maintain a continuous<br>
+ circular buffer of high-resolution images during preview and<br>
+ reprocess image(s) from that buffer into a final capture when<br>
+ triggered by the user. In this mode, the camera device applies<br>
+ noise reduction to low-resolution streams (below maximum recording<br>
+ resolution) to maximize preview quality, but does not apply noise<br>
+ reduction to high-resolution streams, since those will be<br>
+ reprocessed later if necessary.<br></blockquote><div><br></div><div>I know that this is following the Android API, but given that libcamera does not</div><div>have reprocessing capabilities just yet, should this mode be left out for now?</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
# ----------------------------------------------------------------------------<br>
# Draft controls section<br>
<br>
@@ -427,35 +471,6 @@ controls:<br>
The camera will cancel any active trigger and the AF routine is<br>
reset to its initial state.<br>
<br>
- - NoiseReductionMode:<br>
- type: int32_t<br>
- draft: true<br>
- description: |<br>
- Control to select the noise reduction algorithm mode. Currently<br>
- identical to ANDROID_NOISE_REDUCTION_MODE.<br>
-<br>
- Mode of operation for the noise reduction algorithm.<br>
- enum:<br>
- - name: NoiseReductionModeOff<br>
- value: 0<br>
- description: No noise reduction is applied<br>
- - name: NoiseReductionModeFast<br>
- value: 1<br>
- description: |<br>
- Noise reduction is applied without reducing the frame rate.<br>
- - name: NoiseReductionModeHighQuality<br>
- value: 2<br>
- description: |<br>
- High quality noise reduction at the expense of frame rate.<br>
- - name: NoiseReductionModeMinimal<br>
- value: 3<br>
- description: |<br>
- Minimal noise reduction is applied without reducing the frame rate.<br>
- - name: NoiseReductionModeZSL<br>
- value: 4<br>
- description: |<br>
- Noise reduction is applied at different levels to different streams.<br>
-<br>
- ColorCorrectionAberrationMode:<br>
type: int32_t<br>
draft: true<br>
-- <br>
2.27.0<br>
<br>
</blockquote></div></div>