| Message ID | 20250113133405.12167-6-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Mon, Jan 13, 2025 at 02:34:04PM +0100, Milan Zamazal wrote: > Provide the requested contrast value, if any, in the metadata to add to > the completed requests. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/lut.cpp | 11 +++++++++++ > src/ipa/simple/algorithms/lut.h | 5 +++++ > 2 files changed, 16 insertions(+) > > diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp > index 0ba2391f..31208bfe 100644 > --- a/src/ipa/simple/algorithms/lut.cpp > +++ b/src/ipa/simple/algorithms/lut.cpp > @@ -116,6 +116,17 @@ void Lut::prepare(IPAContext &context, > } > } > > +void Lut::process(IPAContext &context, > + [[maybe_unused]] const uint32_t frame, > + [[maybe_unused]] IPAFrameContext &frameContext, > + [[maybe_unused]] const SwIspStats *stats, > + ControlList &metadata) > +{ > + const auto contrast = context.activeState.knobs.contrast; Don't copy. > + if (contrast) > + metadata.set(controls::Contrast, contrast.value()); This will not report the right value for the frame, but given that it's not worse than the other patches, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > +} > + > REGISTER_IPA_ALGORITHM(Lut, "Lut") > > } /* namespace ipa::soft::algorithms */ > diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h > index 889f864b..ab4e1094 100644 > --- a/src/ipa/simple/algorithms/lut.h > +++ b/src/ipa/simple/algorithms/lut.h > @@ -30,6 +30,11 @@ public: > const uint32_t frame, > IPAFrameContext &frameContext, > DebayerParams *params) override; > + void process(IPAContext &context, > + const uint32_t frame, > + IPAFrameContext &frameContext, > + const SwIspStats *stats, > + ControlList &metadata) override; > > private: > void updateGammaTable(IPAContext &context);
Quoting Laurent Pinchart (2025-01-27 07:09:35) > On Mon, Jan 13, 2025 at 02:34:04PM +0100, Milan Zamazal wrote: > > Provide the requested contrast value, if any, in the metadata to add to > > the completed requests. > > > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > > --- > > src/ipa/simple/algorithms/lut.cpp | 11 +++++++++++ > > src/ipa/simple/algorithms/lut.h | 5 +++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp > > index 0ba2391f..31208bfe 100644 > > --- a/src/ipa/simple/algorithms/lut.cpp > > +++ b/src/ipa/simple/algorithms/lut.cpp > > @@ -116,6 +116,17 @@ void Lut::prepare(IPAContext &context, > > } > > } > > > > +void Lut::process(IPAContext &context, > > + [[maybe_unused]] const uint32_t frame, > > + [[maybe_unused]] IPAFrameContext &frameContext, > > + [[maybe_unused]] const SwIspStats *stats, > > + ControlList &metadata) > > +{ > > + const auto contrast = context.activeState.knobs.contrast; > > Don't copy. > > > + if (contrast) > > + metadata.set(controls::Contrast, contrast.value()); > > This will not report the right value for the frame, but given that it's > not worse than the other patches, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> But we /have/ a frame context to store the actual contrast value applied on the frame during prepare() ... so this should be done correctly, not reading the active state, but the frameContext. > > > +} > > + > > REGISTER_IPA_ALGORITHM(Lut, "Lut") > > > > } /* namespace ipa::soft::algorithms */ > > diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h > > index 889f864b..ab4e1094 100644 > > --- a/src/ipa/simple/algorithms/lut.h > > +++ b/src/ipa/simple/algorithms/lut.h > > @@ -30,6 +30,11 @@ public: > > const uint32_t frame, > > IPAFrameContext &frameContext, > > DebayerParams *params) override; > > + void process(IPAContext &context, > > + const uint32_t frame, > > + IPAFrameContext &frameContext, > > + const SwIspStats *stats, > > + ControlList &metadata) override; > > > > private: > > void updateGammaTable(IPAContext &context); > > -- > Regards, > > Laurent Pinchart
On Mon, Jan 27, 2025 at 06:37:31PM +0000, Kieran Bingham wrote: > Quoting Laurent Pinchart (2025-01-27 07:09:35) > > On Mon, Jan 13, 2025 at 02:34:04PM +0100, Milan Zamazal wrote: > > > Provide the requested contrast value, if any, in the metadata to add to > > > the completed requests. > > > > > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > > > --- > > > src/ipa/simple/algorithms/lut.cpp | 11 +++++++++++ > > > src/ipa/simple/algorithms/lut.h | 5 +++++ > > > 2 files changed, 16 insertions(+) > > > > > > diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp > > > index 0ba2391f..31208bfe 100644 > > > --- a/src/ipa/simple/algorithms/lut.cpp > > > +++ b/src/ipa/simple/algorithms/lut.cpp > > > @@ -116,6 +116,17 @@ void Lut::prepare(IPAContext &context, > > > } > > > } > > > > > > +void Lut::process(IPAContext &context, > > > + [[maybe_unused]] const uint32_t frame, > > > + [[maybe_unused]] IPAFrameContext &frameContext, > > > + [[maybe_unused]] const SwIspStats *stats, > > > + ControlList &metadata) > > > +{ > > > + const auto contrast = context.activeState.knobs.contrast; > > > > Don't copy. > > > > > + if (contrast) > > > + metadata.set(controls::Contrast, contrast.value()); > > > > This will not report the right value for the frame, but given that it's > > not worse than the other patches, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > But we /have/ a frame context to store the actual contrast value applied > on the frame during prepare() ... so this should be done correctly, not > reading the active state, but the frameContext. I think it can wait until we solve the synchronization issue on rkisp1. I then expect we'll apply the same solution to other IPA modules. > > > +} > > > + > > > REGISTER_IPA_ALGORITHM(Lut, "Lut") > > > > > > } /* namespace ipa::soft::algorithms */ > > > diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h > > > index 889f864b..ab4e1094 100644 > > > --- a/src/ipa/simple/algorithms/lut.h > > > +++ b/src/ipa/simple/algorithms/lut.h > > > @@ -30,6 +30,11 @@ public: > > > const uint32_t frame, > > > IPAFrameContext &frameContext, > > > DebayerParams *params) override; > > > + void process(IPAContext &context, > > > + const uint32_t frame, > > > + IPAFrameContext &frameContext, > > > + const SwIspStats *stats, > > > + ControlList &metadata) override; > > > > > > private: > > > void updateGammaTable(IPAContext &context);
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Mon, Jan 27, 2025 at 06:37:31PM +0000, Kieran Bingham wrote: >> Quoting Laurent Pinchart (2025-01-27 07:09:35) >> > On Mon, Jan 13, 2025 at 02:34:04PM +0100, Milan Zamazal wrote: > >> > > Provide the requested contrast value, if any, in the metadata to add to >> > > the completed requests. >> > > >> > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> > > --- >> > > src/ipa/simple/algorithms/lut.cpp | 11 +++++++++++ >> > > src/ipa/simple/algorithms/lut.h | 5 +++++ >> > > 2 files changed, 16 insertions(+) >> > > >> > > diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp >> > > index 0ba2391f..31208bfe 100644 >> > > --- a/src/ipa/simple/algorithms/lut.cpp >> > > +++ b/src/ipa/simple/algorithms/lut.cpp >> > > @@ -116,6 +116,17 @@ void Lut::prepare(IPAContext &context, >> > > } >> > > } >> > > >> > > +void Lut::process(IPAContext &context, >> > > + [[maybe_unused]] const uint32_t frame, >> > > + [[maybe_unused]] IPAFrameContext &frameContext, >> > > + [[maybe_unused]] const SwIspStats *stats, >> > > + ControlList &metadata) >> > > +{ >> > > + const auto contrast = context.activeState.knobs.contrast; >> > >> > Don't copy. >> > >> > > + if (contrast) >> > > + metadata.set(controls::Contrast, contrast.value()); >> > >> > This will not report the right value for the frame, but given that it's >> > not worse than the other patches, >> > >> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> But we /have/ a frame context to store the actual contrast value applied >> on the frame during prepare() ... so this should be done correctly, not >> reading the active state, but the frameContext. > > I think it can wait until we solve the synchronization issue on rkisp1. > I then expect we'll apply the same solution to other IPA modules. Well, it's easy to use the frame context and have a bit less incorrect stuff around, I'll do it in v4. >> > > +} >> > > + >> > > REGISTER_IPA_ALGORITHM(Lut, "Lut") >> > > >> > > } /* namespace ipa::soft::algorithms */ >> > > diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h >> > > index 889f864b..ab4e1094 100644 >> > > --- a/src/ipa/simple/algorithms/lut.h >> > > +++ b/src/ipa/simple/algorithms/lut.h >> > > @@ -30,6 +30,11 @@ public: >> > > const uint32_t frame, >> > > IPAFrameContext &frameContext, >> > > DebayerParams *params) override; >> > > + void process(IPAContext &context, >> > > + const uint32_t frame, >> > > + IPAFrameContext &frameContext, >> > > + const SwIspStats *stats, >> > > + ControlList &metadata) override; >> > > >> > > private: >> > > void updateGammaTable(IPAContext &context);
diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp index 0ba2391f..31208bfe 100644 --- a/src/ipa/simple/algorithms/lut.cpp +++ b/src/ipa/simple/algorithms/lut.cpp @@ -116,6 +116,17 @@ void Lut::prepare(IPAContext &context, } } +void Lut::process(IPAContext &context, + [[maybe_unused]] const uint32_t frame, + [[maybe_unused]] IPAFrameContext &frameContext, + [[maybe_unused]] const SwIspStats *stats, + ControlList &metadata) +{ + const auto contrast = context.activeState.knobs.contrast; + if (contrast) + metadata.set(controls::Contrast, contrast.value()); +} + REGISTER_IPA_ALGORITHM(Lut, "Lut") } /* namespace ipa::soft::algorithms */ diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h index 889f864b..ab4e1094 100644 --- a/src/ipa/simple/algorithms/lut.h +++ b/src/ipa/simple/algorithms/lut.h @@ -30,6 +30,11 @@ public: const uint32_t frame, IPAFrameContext &frameContext, DebayerParams *params) override; + void process(IPAContext &context, + const uint32_t frame, + IPAFrameContext &frameContext, + const SwIspStats *stats, + ControlList &metadata) override; private: void updateGammaTable(IPAContext &context);
Provide the requested contrast value, if any, in the metadata to add to the completed requests. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/ipa/simple/algorithms/lut.cpp | 11 +++++++++++ src/ipa/simple/algorithms/lut.h | 5 +++++ 2 files changed, 16 insertions(+)