| Message ID | 20220908014200.28728-10-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:37) > Inherit from the base FrameContext class in the IPU3 IPAFrameContext. > This allows dropping the frame member, which is now stored in the base > class. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/ipu3/ipa_context.cpp | 24 ++++-------------------- > src/ipa/ipu3/ipa_context.h | 7 ++++--- > src/ipa/ipu3/ipu3.cpp | 5 +---- > 3 files changed, 9 insertions(+), 27 deletions(-) > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp > index 13cdb835ef7f..9cfca0db3a0d 100644 > --- a/src/ipa/ipu3/ipa_context.cpp > +++ b/src/ipa/ipu3/ipa_context.cpp > @@ -35,22 +35,6 @@ namespace libcamera::ipa::ipu3 { > * most recently computed by the IPA algorithms. > */ > > -/** > - * \struct IPAFrameContext > - * \brief Context for a frame > - * > - * The frame context stores data specific to a single frame processed by the > - * IPA. Each frame processed by the IPA has a context associated with it, > - * accessible through the IPAContext structure. > - * > - * Fields in the frame context should reflect values and controls > - * associated with the specific frame as requested by the application, and > - * as configured by the hardware. Fields can be read by algorithms to > - * determine if they should update any specific action for this frame, and > - * finally to update the metadata control lists when the frame is fully > - * completed. > - */ > - > /** > * \struct IPAContext > * \brief Global IPA context data shared between all algorithms > @@ -188,15 +172,15 @@ IPAFrameContext::IPAFrameContext() = default; > /** > * \brief Construct a IPAFrameContext instance > */ > -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls) > - : frame(id), frameControls(reqControls) > +IPAFrameContext::IPAFrameContext(const ControlList &reqControls) > + : frameControls(reqControls) > { > sensor = {}; > } > > /** > - * \var IPAFrameContext::frame > - * \brief The frame number > + * \struct IPAFrameContext > + * \brief IPU3-specific FrameContext > * > * \var IPAFrameContext::frameControls > * \brief Controls sent in by the application while queuing the request > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h > index 42e11141d3a1..e8fc42769075 100644 > --- a/src/ipa/ipu3/ipa_context.h > +++ b/src/ipa/ipu3/ipa_context.h > @@ -17,6 +17,8 @@ > #include <libcamera/controls.h> > #include <libcamera/geometry.h> > > +#include <libipa/fc_queue.h> > + > namespace libcamera { > > namespace ipa::ipu3 { > @@ -76,16 +78,15 @@ struct IPAActiveState { > } toneMapping; > }; > > -struct IPAFrameContext { > +struct IPAFrameContext : public FrameContext { > IPAFrameContext(); > - IPAFrameContext(uint32_t id, const ControlList &reqControls); > + IPAFrameContext(const ControlList &reqControls); > > struct { > uint32_t exposure; > double gain; > } sensor; > > - uint32_t frame; > ControlList frameControls; > }; > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index e5a763fd2b08..b1b23fd8f927 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -607,9 +607,6 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > > IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts]; > > - if (frameContext.frame != frame) > - LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context"; > - (almost) unrelated change. Is this intentional? It could be (but the frameContext.frame will still find the derived frame member right?), so an updated commit message, or split to a separate patch and : Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); > > @@ -654,7 +651,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > { > /* \todo Start processing for 'frame' based on 'controls'. */ > - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls }; > + context_.frameContexts[frame % kMaxFrameContexts] = { controls }; > } > > /** > -- > Regards, > > Laurent Pinchart >
Hi Laurent On Thu, Sep 08, 2022 at 04:41:37AM +0300, Laurent Pinchart via libcamera-devel wrote: > Inherit from the base FrameContext class in the IPU3 IPAFrameContext. > This allows dropping the frame member, which is now stored in the base > class. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/ipu3/ipa_context.cpp | 24 ++++-------------------- > src/ipa/ipu3/ipa_context.h | 7 ++++--- > src/ipa/ipu3/ipu3.cpp | 5 +---- > 3 files changed, 9 insertions(+), 27 deletions(-) > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp > index 13cdb835ef7f..9cfca0db3a0d 100644 > --- a/src/ipa/ipu3/ipa_context.cpp > +++ b/src/ipa/ipu3/ipa_context.cpp > @@ -35,22 +35,6 @@ namespace libcamera::ipa::ipu3 { > * most recently computed by the IPA algorithms. > */ > > -/** > - * \struct IPAFrameContext > - * \brief Context for a frame > - * > - * The frame context stores data specific to a single frame processed by the > - * IPA. Each frame processed by the IPA has a context associated with it, > - * accessible through the IPAContext structure. > - * > - * Fields in the frame context should reflect values and controls > - * associated with the specific frame as requested by the application, and > - * as configured by the hardware. Fields can be read by algorithms to > - * determine if they should update any specific action for this frame, and > - * finally to update the metadata control lists when the frame is fully > - * completed. > - */ > - > /** > * \struct IPAContext > * \brief Global IPA context data shared between all algorithms > @@ -188,15 +172,15 @@ IPAFrameContext::IPAFrameContext() = default; > /** > * \brief Construct a IPAFrameContext instance > */ > -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls) > - : frame(id), frameControls(reqControls) > +IPAFrameContext::IPAFrameContext(const ControlList &reqControls) > + : frameControls(reqControls) > { > sensor = {}; > } > > /** > - * \var IPAFrameContext::frame > - * \brief The frame number > + * \struct IPAFrameContext > + * \brief IPU3-specific FrameContext > * > * \var IPAFrameContext::frameControls > * \brief Controls sent in by the application while queuing the request > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h > index 42e11141d3a1..e8fc42769075 100644 > --- a/src/ipa/ipu3/ipa_context.h > +++ b/src/ipa/ipu3/ipa_context.h > @@ -17,6 +17,8 @@ > #include <libcamera/controls.h> > #include <libcamera/geometry.h> > > +#include <libipa/fc_queue.h> > + > namespace libcamera { > > namespace ipa::ipu3 { > @@ -76,16 +78,15 @@ struct IPAActiveState { > } toneMapping; > }; > > -struct IPAFrameContext { > +struct IPAFrameContext : public FrameContext { This could now be called IPU3FrameContext :) > IPAFrameContext(); > - IPAFrameContext(uint32_t id, const ControlList &reqControls); > + IPAFrameContext(const ControlList &reqControls); > > struct { > uint32_t exposure; > double gain; > } sensor; > > - uint32_t frame; > ControlList frameControls; > }; > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index e5a763fd2b08..b1b23fd8f927 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -607,9 +607,6 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > > IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts]; > > - if (frameContext.frame != frame) > - LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context"; > - > frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); > > @@ -654,7 +651,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > { > /* \todo Start processing for 'frame' based on 'controls'. */ > - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls }; > + context_.frameContexts[frame % kMaxFrameContexts] = { controls }; I understand this right that once moving to the FCQ the frame number initialization and the check above will be there performed ? Should we allow then to have a constructor for the IPA-specific class that accepts a frame number ? Nits apart Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > } > > /** > -- > Regards, > > Laurent Pinchart >
Hi Kieran, On Tue, Sep 20, 2022 at 03:09:53PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:37) > > Inherit from the base FrameContext class in the IPU3 IPAFrameContext. > > This allows dropping the frame member, which is now stored in the base > > class. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/ipu3/ipa_context.cpp | 24 ++++-------------------- > > src/ipa/ipu3/ipa_context.h | 7 ++++--- > > src/ipa/ipu3/ipu3.cpp | 5 +---- > > 3 files changed, 9 insertions(+), 27 deletions(-) > > > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp > > index 13cdb835ef7f..9cfca0db3a0d 100644 > > --- a/src/ipa/ipu3/ipa_context.cpp > > +++ b/src/ipa/ipu3/ipa_context.cpp > > @@ -35,22 +35,6 @@ namespace libcamera::ipa::ipu3 { > > * most recently computed by the IPA algorithms. > > */ > > > > -/** > > - * \struct IPAFrameContext > > - * \brief Context for a frame > > - * > > - * The frame context stores data specific to a single frame processed by the > > - * IPA. Each frame processed by the IPA has a context associated with it, > > - * accessible through the IPAContext structure. > > - * > > - * Fields in the frame context should reflect values and controls > > - * associated with the specific frame as requested by the application, and > > - * as configured by the hardware. Fields can be read by algorithms to > > - * determine if they should update any specific action for this frame, and > > - * finally to update the metadata control lists when the frame is fully > > - * completed. > > - */ > > - > > /** > > * \struct IPAContext > > * \brief Global IPA context data shared between all algorithms > > @@ -188,15 +172,15 @@ IPAFrameContext::IPAFrameContext() = default; > > /** > > * \brief Construct a IPAFrameContext instance > > */ > > -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls) > > - : frame(id), frameControls(reqControls) > > +IPAFrameContext::IPAFrameContext(const ControlList &reqControls) > > + : frameControls(reqControls) > > { > > sensor = {}; > > } > > > > /** > > - * \var IPAFrameContext::frame > > - * \brief The frame number > > + * \struct IPAFrameContext > > + * \brief IPU3-specific FrameContext > > * > > * \var IPAFrameContext::frameControls > > * \brief Controls sent in by the application while queuing the request > > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h > > index 42e11141d3a1..e8fc42769075 100644 > > --- a/src/ipa/ipu3/ipa_context.h > > +++ b/src/ipa/ipu3/ipa_context.h > > @@ -17,6 +17,8 @@ > > #include <libcamera/controls.h> > > #include <libcamera/geometry.h> > > > > +#include <libipa/fc_queue.h> > > + > > namespace libcamera { > > > > namespace ipa::ipu3 { > > @@ -76,16 +78,15 @@ struct IPAActiveState { > > } toneMapping; > > }; > > > > -struct IPAFrameContext { > > +struct IPAFrameContext : public FrameContext { > > IPAFrameContext(); > > - IPAFrameContext(uint32_t id, const ControlList &reqControls); > > + IPAFrameContext(const ControlList &reqControls); > > > > struct { > > uint32_t exposure; > > double gain; > > } sensor; > > > > - uint32_t frame; > > ControlList frameControls; > > }; > > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > index e5a763fd2b08..b1b23fd8f927 100644 > > --- a/src/ipa/ipu3/ipu3.cpp > > +++ b/src/ipa/ipu3/ipu3.cpp > > @@ -607,9 +607,6 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > > > > IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts]; > > > > - if (frameContext.frame != frame) > > - LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context"; > > - > > (almost) unrelated change. Is this intentional? The frame member of the base class is private, so this didn't compile anymore. I didn't think the check was useful, given the upcoming switch to FCQueue, so I dropped it. I'll mention that in the commit message. > It could be (but the frameContext.frame will still find the derived > frame member right?), so an updated commit message, or split to a > separate patch and : > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > > frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); > > > > @@ -654,7 +651,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > > { > > /* \todo Start processing for 'frame' based on 'controls'. */ > > - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls }; > > + context_.frameContexts[frame % kMaxFrameContexts] = { controls }; > > } > > > > /**
Hi Jacopo, On Wed, Sep 21, 2022 at 08:10:30PM +0200, Jacopo Mondi wrote: > On Thu, Sep 08, 2022 at 04:41:37AM +0300, Laurent Pinchart via libcamera-devel wrote: > > Inherit from the base FrameContext class in the IPU3 IPAFrameContext. > > This allows dropping the frame member, which is now stored in the base > > class. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/ipu3/ipa_context.cpp | 24 ++++-------------------- > > src/ipa/ipu3/ipa_context.h | 7 ++++--- > > src/ipa/ipu3/ipu3.cpp | 5 +---- > > 3 files changed, 9 insertions(+), 27 deletions(-) > > > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp > > index 13cdb835ef7f..9cfca0db3a0d 100644 > > --- a/src/ipa/ipu3/ipa_context.cpp > > +++ b/src/ipa/ipu3/ipa_context.cpp > > @@ -35,22 +35,6 @@ namespace libcamera::ipa::ipu3 { > > * most recently computed by the IPA algorithms. > > */ > > > > -/** > > - * \struct IPAFrameContext > > - * \brief Context for a frame > > - * > > - * The frame context stores data specific to a single frame processed by the > > - * IPA. Each frame processed by the IPA has a context associated with it, > > - * accessible through the IPAContext structure. > > - * > > - * Fields in the frame context should reflect values and controls > > - * associated with the specific frame as requested by the application, and > > - * as configured by the hardware. Fields can be read by algorithms to > > - * determine if they should update any specific action for this frame, and > > - * finally to update the metadata control lists when the frame is fully > > - * completed. > > - */ > > - > > /** > > * \struct IPAContext > > * \brief Global IPA context data shared between all algorithms > > @@ -188,15 +172,15 @@ IPAFrameContext::IPAFrameContext() = default; > > /** > > * \brief Construct a IPAFrameContext instance > > */ > > -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls) > > - : frame(id), frameControls(reqControls) > > +IPAFrameContext::IPAFrameContext(const ControlList &reqControls) > > + : frameControls(reqControls) > > { > > sensor = {}; > > } > > > > /** > > - * \var IPAFrameContext::frame > > - * \brief The frame number > > + * \struct IPAFrameContext > > + * \brief IPU3-specific FrameContext > > * > > * \var IPAFrameContext::frameControls > > * \brief Controls sent in by the application while queuing the request > > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h > > index 42e11141d3a1..e8fc42769075 100644 > > --- a/src/ipa/ipu3/ipa_context.h > > +++ b/src/ipa/ipu3/ipa_context.h > > @@ -17,6 +17,8 @@ > > #include <libcamera/controls.h> > > #include <libcamera/geometry.h> > > > > +#include <libipa/fc_queue.h> > > + > > namespace libcamera { > > > > namespace ipa::ipu3 { > > @@ -76,16 +78,15 @@ struct IPAActiveState { > > } toneMapping; > > }; > > > > -struct IPAFrameContext { > > +struct IPAFrameContext : public FrameContext { > > This could now be called IPU3FrameContext :) There's a subsequent patch in the series that handles renames :-) > > IPAFrameContext(); > > - IPAFrameContext(uint32_t id, const ControlList &reqControls); > > + IPAFrameContext(const ControlList &reqControls); > > > > struct { > > uint32_t exposure; > > double gain; > > } sensor; > > > > - uint32_t frame; > > ControlList frameControls; > > }; > > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > index e5a763fd2b08..b1b23fd8f927 100644 > > --- a/src/ipa/ipu3/ipu3.cpp > > +++ b/src/ipa/ipu3/ipu3.cpp > > @@ -607,9 +607,6 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > > > > IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts]; > > > > - if (frameContext.frame != frame) > > - LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context"; > > - > > frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > > frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); > > > > @@ -654,7 +651,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > > { > > /* \todo Start processing for 'frame' based on 'controls'. */ > > - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls }; > > + context_.frameContexts[frame % kMaxFrameContexts] = { controls }; > > I understand this right that once moving to the FCQ the frame number > initialization and the check above will be there performed ? That's right. > Should we allow then to have a constructor for the IPA-specific class > that accepts a frame number ? What would you use that for ? > Nits apart > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > } > > > > /**
Hi Laurent, On Thu, Sep 22, 2022 at 10:54:54PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Wed, Sep 21, 2022 at 08:10:30PM +0200, Jacopo Mondi wrote: > > On Thu, Sep 08, 2022 at 04:41:37AM +0300, Laurent Pinchart via libcamera-devel wrote: > > > Inherit from the base FrameContext class in the IPU3 IPAFrameContext. > > > This allows dropping the frame member, which is now stored in the base > > > class. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/ipa/ipu3/ipa_context.cpp | 24 ++++-------------------- > > > src/ipa/ipu3/ipa_context.h | 7 ++++--- > > > src/ipa/ipu3/ipu3.cpp | 5 +---- > > > 3 files changed, 9 insertions(+), 27 deletions(-) > > > > > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp > > > index 13cdb835ef7f..9cfca0db3a0d 100644 > > > --- a/src/ipa/ipu3/ipa_context.cpp > > > +++ b/src/ipa/ipu3/ipa_context.cpp > > > @@ -35,22 +35,6 @@ namespace libcamera::ipa::ipu3 { > > > * most recently computed by the IPA algorithms. > > > */ > > > > > > -/** > > > - * \struct IPAFrameContext > > > - * \brief Context for a frame > > > - * > > > - * The frame context stores data specific to a single frame processed by the > > > - * IPA. Each frame processed by the IPA has a context associated with it, > > > - * accessible through the IPAContext structure. > > > - * > > > - * Fields in the frame context should reflect values and controls > > > - * associated with the specific frame as requested by the application, and > > > - * as configured by the hardware. Fields can be read by algorithms to > > > - * determine if they should update any specific action for this frame, and > > > - * finally to update the metadata control lists when the frame is fully > > > - * completed. > > > - */ > > > - > > > /** > > > * \struct IPAContext > > > * \brief Global IPA context data shared between all algorithms > > > @@ -188,15 +172,15 @@ IPAFrameContext::IPAFrameContext() = default; > > > /** > > > * \brief Construct a IPAFrameContext instance > > > */ > > > -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls) > > > - : frame(id), frameControls(reqControls) > > > +IPAFrameContext::IPAFrameContext(const ControlList &reqControls) > > > + : frameControls(reqControls) > > > { > > > sensor = {}; > > > } > > > > > > /** > > > - * \var IPAFrameContext::frame > > > - * \brief The frame number > > > + * \struct IPAFrameContext > > > + * \brief IPU3-specific FrameContext > > > * > > > * \var IPAFrameContext::frameControls > > > * \brief Controls sent in by the application while queuing the request > > > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h > > > index 42e11141d3a1..e8fc42769075 100644 > > > --- a/src/ipa/ipu3/ipa_context.h > > > +++ b/src/ipa/ipu3/ipa_context.h > > > @@ -17,6 +17,8 @@ > > > #include <libcamera/controls.h> > > > #include <libcamera/geometry.h> > > > > > > +#include <libipa/fc_queue.h> > > > + > > > namespace libcamera { > > > > > > namespace ipa::ipu3 { > > > @@ -76,16 +78,15 @@ struct IPAActiveState { > > > } toneMapping; > > > }; > > > > > > -struct IPAFrameContext { > > > +struct IPAFrameContext : public FrameContext { > > > > This could now be called IPU3FrameContext :) > > There's a subsequent patch in the series that handles renames :-) > > > > IPAFrameContext(); > > > - IPAFrameContext(uint32_t id, const ControlList &reqControls); > > > + IPAFrameContext(const ControlList &reqControls); > > > > > > struct { > > > uint32_t exposure; > > > double gain; > > > } sensor; > > > > > > - uint32_t frame; > > > ControlList frameControls; > > > }; > > > > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > > index e5a763fd2b08..b1b23fd8f927 100644 > > > --- a/src/ipa/ipu3/ipu3.cpp > > > +++ b/src/ipa/ipu3/ipu3.cpp > > > @@ -607,9 +607,6 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > > > > > > IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts]; > > > > > > - if (frameContext.frame != frame) > > > - LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context"; > > > - > > > frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > > > frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); > > > > > > @@ -654,7 +651,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > > > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > > > { > > > /* \todo Start processing for 'frame' based on 'controls'. */ > > > - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls }; > > > + context_.frameContexts[frame % kMaxFrameContexts] = { controls }; > > > > I understand this right that once moving to the FCQ the frame number > > initialization and the check above will be there performed ? > > That's right. > > > Should we allow then to have a constructor for the IPA-specific class > > that accepts a frame number ? > > What would you use that for ? Not advocating for it, I was pointing out it is there > > > > IPAFrameContext(); > > > - IPAFrameContext(uint32_t id, const ControlList &reqControls); > > > + IPAFrameContext(const ControlList &reqControls); But I realize now the line has a '-' sign in front. One day I'll learn how to read patches > > > Nits apart > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > } > > > > > > /** > > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp index 13cdb835ef7f..9cfca0db3a0d 100644 --- a/src/ipa/ipu3/ipa_context.cpp +++ b/src/ipa/ipu3/ipa_context.cpp @@ -35,22 +35,6 @@ namespace libcamera::ipa::ipu3 { * most recently computed by the IPA algorithms. */ -/** - * \struct IPAFrameContext - * \brief Context for a frame - * - * The frame context stores data specific to a single frame processed by the - * IPA. Each frame processed by the IPA has a context associated with it, - * accessible through the IPAContext structure. - * - * Fields in the frame context should reflect values and controls - * associated with the specific frame as requested by the application, and - * as configured by the hardware. Fields can be read by algorithms to - * determine if they should update any specific action for this frame, and - * finally to update the metadata control lists when the frame is fully - * completed. - */ - /** * \struct IPAContext * \brief Global IPA context data shared between all algorithms @@ -188,15 +172,15 @@ IPAFrameContext::IPAFrameContext() = default; /** * \brief Construct a IPAFrameContext instance */ -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls) - : frame(id), frameControls(reqControls) +IPAFrameContext::IPAFrameContext(const ControlList &reqControls) + : frameControls(reqControls) { sensor = {}; } /** - * \var IPAFrameContext::frame - * \brief The frame number + * \struct IPAFrameContext + * \brief IPU3-specific FrameContext * * \var IPAFrameContext::frameControls * \brief Controls sent in by the application while queuing the request diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h index 42e11141d3a1..e8fc42769075 100644 --- a/src/ipa/ipu3/ipa_context.h +++ b/src/ipa/ipu3/ipa_context.h @@ -17,6 +17,8 @@ #include <libcamera/controls.h> #include <libcamera/geometry.h> +#include <libipa/fc_queue.h> + namespace libcamera { namespace ipa::ipu3 { @@ -76,16 +78,15 @@ struct IPAActiveState { } toneMapping; }; -struct IPAFrameContext { +struct IPAFrameContext : public FrameContext { IPAFrameContext(); - IPAFrameContext(uint32_t id, const ControlList &reqControls); + IPAFrameContext(const ControlList &reqControls); struct { uint32_t exposure; double gain; } sensor; - uint32_t frame; ControlList frameControls; }; diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index e5a763fd2b08..b1b23fd8f927 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -607,9 +607,6 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts]; - if (frameContext.frame != frame) - LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context"; - frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); @@ -654,7 +651,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) { /* \todo Start processing for 'frame' based on 'controls'. */ - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls }; + context_.frameContexts[frame % kMaxFrameContexts] = { controls }; } /**
Inherit from the base FrameContext class in the IPU3 IPAFrameContext. This allows dropping the frame member, which is now stored in the base class. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/ipa/ipu3/ipa_context.cpp | 24 ++++-------------------- src/ipa/ipu3/ipa_context.h | 7 ++++--- src/ipa/ipu3/ipu3.cpp | 5 +---- 3 files changed, 9 insertions(+), 27 deletions(-)