[PATCH v3 1/6] libcamera: software_isp: Track frames and requests

[Kieran Bingham]

Quoting Laurent Pinchart (2025-01-27 06:59:36)
> Hi Milan,
> 
> Thank you for the patch.
> 
> On Mon, Jan 13, 2025 at 02:34:00PM +0100, Milan Zamazal wrote:
> > Hardware pipelines track requests and other information related to
> > particular frames.  This hasn't been needed in software ISP so far.  But
> > in order to be able to track metadata corresponding to a given frame,
> > frame-request tracking mechanism starts being useful.  This patch
> > introduces the basic tracking structure, actual metadata handling is
> > added in the following patch.
> > 

I'm also weary that Barnabás made some fixes to other IPA 'frame
handlers' recently.

I think somehow this concept needs to be templated to make sure we don't
have different bugs and different implementation in each IPA...

Not a fault of 'this' patch, but I'm really weary about seeing multiple
implementations of the same object.

> > Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 81 ++++++++++++++++++++++--
> >  1 file changed, 76 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 8ac24e6e..123179b6 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -181,6 +181,64 @@ LOG_DEFINE_CATEGORY(SimplePipeline)
> >  
> >  class SimplePipelineHandler;
> >  
> > +struct SimpleFrameInfo {
> > +     uint32_t frame;
> > +     Request *request;
> > +};
> > +
> > +class SimpleFrames
> > +{
> > +public:
> > +     void create(Request *request);
> > +     void destroy(uint32_t frame);
> > +     void clear();
> > +
> > +     SimpleFrameInfo *find(uint32_t frame);
> > +     SimpleFrameInfo *find(Request *request);
> > +
> > +private:
> > +     std::map<uint32_t, SimpleFrameInfo> frameInfo_;
> > +};
> > +
> > +void SimpleFrames::create(Request *request)
> > +{
> > +     uint32_t frame = request->sequence();
> > +     auto [it, inserted] = frameInfo_.try_emplace(request->sequence());
> > +     ASSERT(inserted);
> > +     it->second.frame = frame;
> > +     it->second.request = request;
> > +}
> > +
> > +void SimpleFrames::destroy(uint32_t frame)
> > +{
> > +     frameInfo_.erase(frame);
> > +}
> > +
> > +void SimpleFrames::clear()
> > +{
> > +     frameInfo_.clear();
> > +}
> > +
> > +SimpleFrameInfo *SimpleFrames::find(uint32_t frame)
> > +{
> > +     auto info = frameInfo_.find(frame);
> > +     if (info == frameInfo_.end())
> > +             return nullptr;
> > +     return &info->second;
> > +}
> > +
> > +SimpleFrameInfo *SimpleFrames::find(Request *request)
> > +{
> > +     for (auto &itInfo : frameInfo_) {
> > +             SimpleFrameInfo *info = &itInfo.second;
> > +
> > +             if (info->request == request)
> > +                     return info;
> > +     }
> > +
> > +     return nullptr;
> > +}
> > +
> >  struct SimplePipelineInfo {
> >       const char *driver;
> >       /*
> > @@ -293,11 +351,13 @@ public:
> >  
> >       std::unique_ptr<Converter> converter_;
> >       std::unique_ptr<SoftwareIsp> swIsp_;
> > +     SimpleFrames frameInfo_;
> >  
> >  private:
> >       void tryPipeline(unsigned int code, const Size &size);
> >       static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);
> >  
> > +     void completeRequest(Request *request);
> >       void conversionInputDone(FrameBuffer *buffer);
> >       void conversionOutputDone(FrameBuffer *buffer);
> >  
> > @@ -799,7 +859,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
> >                       /* No conversion, just complete the request. */
> >                       Request *request = buffer->request();
> >                       pipe->completeBuffer(request, buffer);
> > -                     pipe->completeRequest(request);
> > +                     completeRequest(request);
> >                       return;
> >               }
> >  
> > @@ -817,7 +877,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
> >               const RequestOutputs &outputs = conversionQueue_.front();
> >               for (auto &[stream, buf] : outputs.outputs)
> >                       pipe->completeBuffer(outputs.request, buf);
> > -             pipe->completeRequest(outputs.request);
> > +             completeRequest(outputs.request);
> >               conversionQueue_.pop();
> >  
> >               return;
> > @@ -875,7 +935,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
> >  
> >       /* Otherwise simply complete the request. */
> >       pipe->completeBuffer(request, buffer);
> > -     pipe->completeRequest(request);
> > +     completeRequest(request);
> >  }
> >  
> >  void SimpleCameraData::clearIncompleteRequests()
> > @@ -886,6 +946,14 @@ void SimpleCameraData::clearIncompleteRequests()
> >       }
> >  }
> >  
> > +void SimpleCameraData::completeRequest(Request *request)
> > +{
> > +     SimpleFrameInfo *info = frameInfo_.find(request);
> > +     if (info)
> 
> If there's no frame info something is really wrong. I don't think
> completing the request makes much sense in that case.
> 
> > +             frameInfo_.destroy(info->frame);
> > +     pipe()->completeRequest(request);
> > +}
> > +
> >  void SimpleCameraData::conversionInputDone(FrameBuffer *buffer)
> >  {
> >       /* Queue the input buffer back for capture. */
> > @@ -899,7 +967,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
> >       /* Complete the buffer and the request. */
> >       Request *request = buffer->request();
> >       if (pipe->completeBuffer(request, buffer))
> > -             pipe->completeRequest(request);
> > +             completeRequest(request);
> >  }
> >  
> >  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> > @@ -1412,6 +1480,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
> >  
> >       video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady);
> >  
> > +     data->frameInfo_.clear();
> >       data->clearIncompleteRequests();
> >       data->conversionBuffers_.clear();
> >  
> > @@ -1442,8 +1511,10 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
> >  
> >       if (data->useConversion_) {
> >               data->conversionQueue_.push({ request, std::move(buffers) });
> > -             if (data->swIsp_)
> > +             if (data->swIsp_) {
> > +                     data->frameInfo_.create(request);
> >                       data->swIsp_->queueRequest(request->sequence(), request->controls());
> > +             }
> >       }
> >  
> >       return 0;
> 
> -- 
> Regards,
> 
> Laurent Pinchart