<div dir="ltr"><div dir="ltr">Hi Paul,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 14 Sept 2021 at 06:08, <<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">Hi Naush,<br>
<br>
Thank you for the feedback.<br>
<br>
On Mon, Sep 13, 2021 at 11:31:39AM +0100, Naushir Patuck wrote:<br>
> Hi Paul,<br>
> <br>
> Thank you for your patch.<br>
> <br>
> On Mon, 13 Sept 2021 at 11:20, Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>> wrote:<br>
> <br>
> 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/<br>
> 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,<br>
> 16.0f) },<br>
> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle<br>
> (65535, 65535, 65535, 65535), Rectangle{}) },<br>
> { &controls::FrameDurationLimits, ControlInfo(INT64_C<br>
> (1000), INT64_C(1000000000)) },<br>
> - { &controls::draft::NoiseReductionMode, ControlInfo<br>
> (controls::draft::NoiseReductionModeValues) }<br>
> + { &controls::NoiseReductionMode, ControlInfo<br>
> (controls::NoiseReductionModeValues) }<br>
> }, controls::controls);<br>
> <br>
> } /* namespace RPi */<br>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/<br>
> 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(&<br>
> controls::draft::NoiseReductionMode);<br>
> + const auto &infoMap = controlsInfo.find(&<br>
> 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/<br>
> 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><br>
> AwbModeTable = {<br>
> };<br>
> <br>
> static const std::map<int32_t, RPiController::DenoiseMode><br>
> DenoiseModeTable = {<br>
> - { controls::draft::NoiseReductionModeOff,<br>
> RPiController::DenoiseMode::Off },<br>
> - { controls::draft::NoiseReductionModeFast,<br>
> RPiController::DenoiseMode::ColourFast },<br>
> - { controls::draft::NoiseReductionModeHighQuality,<br>
> RPiController::DenoiseMode::ColourHighQuality },<br>
> - { controls::draft::NoiseReductionModeMinimal,<br>
> RPiController::DenoiseMode::ColourOff },<br>
> - { controls::draft::NoiseReductionModeZSL,<br>
> RPiController::DenoiseMode::ColourHighQuality },<br>
> + { controls::NoiseReductionModeOff, RPiController::DenoiseMode::Off<br>
> },<br>
> + { controls::NoiseReductionModeFast,<br>
> RPiController::DenoiseMode::ColourFast },<br>
> + { controls::NoiseReductionModeHighQuality,<br>
> RPiController::DenoiseMode::ColourHighQuality },<br>
> + { controls::NoiseReductionModeMinimal,<br>
> RPiController::DenoiseMode::ColourOff },<br>
> + { controls::NoiseReductionModeZSL,<br>
> 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<br>
> mismatch, we should<br>
> * report the status correctly in the<br>
> metadata.<br>
> */<br>
> - libcameraMetadata_.set<br>
> (controls::draft::NoiseReductionMode, idx);<br>
> + libcameraMetadata_.set<br>
> (controls::NoiseReductionMode, idx);<br>
> } else {<br>
> LOG(IPARPI, Error) << "Noise reduction mode<br>
> " << idx<br>
> << " not recognised";<br>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/<br>
> 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<br>
> 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<br>
> 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<br>
> Off if<br>
> + any noise reduction will slow down the frame rate.<br>
> <br>
> <br>
> I wonder if the stated requirement of "without reducing the framerate" is a bit<br>
> strict here?<br>
> There are certainly going to be cases, e.g. 60/120fps recording, where our<br>
> implementation will<br>
> drop frames and be unable to keep up. Perhaps this should be worded as:<br>
> "Noise reduction is applied with minimal possible impact on the frame rate."<br>
<br>
On one hand, I think the application will want to select whether:<br>
- it wants to prioritize fps over nr (if you can't suply nr fast enough,<br>
then just disable it)<br>
- or vice versa (I want nr, it's okay to slow it down as long as I get<br>
my nr)<br>
<br>
On the other hand, I see what you mean, that normally the camera would<br>
be able to provide normal quality nr, except when the frame rate is too<br>
high.<br>
<br>
Since we don't have to copy android exactly, we could have some sort of<br>
mapping from frame rate to available nr modes, or how much the frame<br>
rate will fall at what quality level.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Though I think that's overkill. The way that I read the android docs<br>
(which I've also copied into the definition above) is that when the<br>
application selects fast mode, it prioritizes the frame rate, so if the<br>
camera can't provide it without dropping the frame rate, then just<br>
disable the nr. The application has specified that it'd rather have the<br>
frame rate than the nr. If it wanted the nr over the frame rate, it<br>
would've chosen hq instead of fast.<br></blockquote><div><br></div><div>Agree, I don't think we need to take any steps to link framerate with this control.</div><div>The only thing I would suggest is a small change in the wording to remove</div><div>any frame rate guarantees.</div><div><br></div><div>Thanks,</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>
Or perhaps we could simply have an in-between mode? But I don't see much<br>
point in it unless there's some way to report to the application how<br>
much the frame reduction is (which I suppose we could report in a new<br>
control?). Is there any value in this? I feel like fast and hq is enough<br>
to cover the two anticipated "desires" of applications. Do you know any<br>
examples of use cases for an application that's okay with lowered frame<br>
rate "but only up to a certain amount"?<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
> <br>
> <br>
> + - 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<br>
> remove<br>
> + demosaicing or other processing artifacts. Frame rate will not<br>
> be<br>
> + reduced.<br>
> <br>
> <br>
> Similar to the above comment, perhaps there may be an impact on the framerate<br>
> as well.<br>
> <br>
> <br>
> <br>
> + - name: NoiseReductionModeZSL<br>
> + value: 4<br>
> + description: |<br>
> + Noise reduction is applied at different levels to different<br>
> streams.<br>
> +<br>
> + ZSL is meant to be used by applications that maintain a<br>
> 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<br>
> recording<br>
> + resolution) to maximize preview quality, but does not apply<br>
> noise<br>
> + reduction to high-resolution streams, since those will be<br>
> + reprocessed later if necessary.<br>
> <br>
> <br>
> I know that this is following the Android API, but given that libcamera does<br>
> not<br>
> have reprocessing capabilities just yet, should this mode be left out for now?<br>
<br>
Yeah, probably.<br>
<br>
<br>
Thanks,<br>
<br>
Paul<br>
<br>
> <br>
> <br>
> <br>
> +<br>
> #<br>
> ----------------------------------------------------------------------------<br>
> # Draft controls section<br>
> <br>
> @@ -427,35 +471,6 @@ controls:<br>
> The camera will cancel any active trigger and the AF routine<br>
> 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<br>
> rate.<br>
> - - name: NoiseReductionModeZSL<br>
> - value: 4<br>
> - description: |<br>
> - Noise reduction is applied at different levels to different<br>
> streams.<br>
> -<br>
> - ColorCorrectionAberrationMode:<br>
> type: int32_t<br>
> draft: true<br>
> --<br>
> 2.27.0<br>
> <br>
> <br>
</blockquote></div></div>