[libcamera-devel] [PATCH 1/3] controls: Promote NoiseReductionMode to non-draft
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Tue Sep 14 07:08:15 CEST 2021
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 at 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 at 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
>
>
More information about the libcamera-devel
mailing list