[libcamera-devel] [PATCH 1/3] controls: Promote NoiseReductionMode to non-draft
Naushir Patuck
naush at raspberrypi.com
Tue Sep 14 09:45:51 CEST 2021
Hi Paul,
On Tue, 14 Sept 2021 at 06:08, <paul.elder at 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 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.
>
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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210914/4256ae1f/attachment-0001.htm>
More information about the libcamera-devel
mailing list