[libcamera-devel] [PATCH v5 05/12] pipeline: raspberrypi: Handle OptionalStream hints for Unicam Image

[Laurent Pinchart]

Hello,

On Fri, Jan 20, 2023 at 10:53:22AM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:46)
> > Look for OptionalStream flag in the hints field of the Unicam Image
> > StreamConfiguration structure. If this flag is not set, it guarantees that the
> > application will provide buffers for Unicam Image, so override the
> > minUnicamBuffers and minTotalUnicamBuffers config parameters in the following
> > way:
> > 
> > - If startup drop frames are required, allocate at least 1 internal buffer.
> > - If no startup drop frames are required, do not allocate any internal buffers.
> 
> As highlighted somewhere else, the only part I hadn't considered when
> suggesting this way around is the raw sensor mode configuration. But
> that might still be ok.
> 
> And probably means we really need a helper or something to improve how
> we handle configuring the sensor modes/sizes separately from the output
> stream, as here we would have to make sure that configuring a RAW stream
> when the application doesn't provide buffers will have to report it's an
> OptionalStream in the hint....
> 
> > All other buffer allocations remain unchanged.
> > 
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 ++++++++++++++-----
> >  1 file changed, 28 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 8d325c7a18c5..6ed4cc2c7ba7 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1448,12 +1448,33 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> >  int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >  {
> >         RPiCameraData *data = cameraData(camera);
> > -       unsigned int numRawBuffers = 0;
> > +       unsigned int minUnicamBuffers = data->config_.minUnicamBuffers,
> > +                    minTotalUnicamBuffers = data->config_.minTotalUnicamBuffers,
> > +                    numRawBuffers = 0;

One variable per line please.

> >         int ret;
> >  
> >         for (Stream *s : camera->streams()) {
> > -               if (isRaw(s->configuration().pixelFormat)) {
> > +               if (s == &data->unicam_[Unicam::Image]) {

Is this a functional change or do you expect it to preserve the existing
behaviour ? I'm wondering how it affects use cases with YUV sensors.

> >                         numRawBuffers = s->configuration().bufferCount;
> > +                       /*
> > +                        * If the application provides a guarantees that Unicam
> > +                        * image buffers will always be provided for the RAW stream
> > +                        * in a Request, we need:
> > +                        * - at least 1 internal Unicam buffer to handle startup frame drops,
> > +                        * - no internal Unicam buffers if there are no startup frame drops.
> > +                        */
> > +                       if (!(s->configuration().hints &
> > +                             StreamConfiguration::Hint::OptionalStream)) {
> > +                               if (data->dropFrameCount_) {
> > +                                       minUnicamBuffers = std::max(1u, minUnicamBuffers);
> > +                                       minTotalUnicamBuffers =
> > +                                               std::max(minUnicamBuffers, minTotalUnicamBuffers);
> > +                               } else {
> > +                                       minUnicamBuffers = 0;
> > +                                       minTotalUnicamBuffers = 0;
> > +                               }

Why do you override the minimums from the configuration only when not
dropping startup frames ? I'm not saying this is wrong, but I think
better documentation is needed (likely in the example configuration file
in patch 07/12) to explain the behaviour and how the configuration
parameters influence it.

> > +                       }
> > +
> >                         break;
> >                 }
> >         }
> > @@ -1465,7 +1486,6 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >                  * For Unicam, allocate a minimum number of buffers for internal
> >                  * use as we want to avoid any frame drops.
> >                  */
> > -               const unsigned int minBuffers = data->config_.minTotalUnicamBuffers;
> >                 if (stream == &data->unicam_[Unicam::Image]) {
> >                         /*
> >                          * If an application has configured a RAW stream, allocate
> > @@ -1473,8 +1493,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >                          * we have at least minUnicamBuffers of internal buffers
> >                          * to use to minimise frame drops.
> >                          */
> > -                       numBuffers = std::max<int>(data->config_.minUnicamBuffers,
> > -                                                  minBuffers - numRawBuffers);
> > +                       numBuffers = std::max<int>(minUnicamBuffers,
> > +                                                  minTotalUnicamBuffers - numRawBuffers);
> >                 } else if (stream == &data->isp_[Isp::Input]) {
> >                         /*
> >                          * ISP input buffers are imported from Unicam, so follow
> > @@ -1482,15 +1502,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >                          * available.
> >                          */
> >                         numBuffers = numRawBuffers +
> > -                                    std::max<int>(data->config_.minUnicamBuffers,
> > -                                                  minBuffers - numRawBuffers);
> > +                                    std::max<int>(minUnicamBuffers,
> > +                                                  minTotalUnicamBuffers - numRawBuffers);
> >  
> >                 } else if (stream == &data->unicam_[Unicam::Embedded]) {
> >                         /*
> >                          * Embedded data buffers are (currently) for internal use,
> >                          * so allocate the minimum required to avoid frame drops.
> >                          */
> > -                       numBuffers = minBuffers;
> > +                       numBuffers = data->config_.minTotalUnicamBuffers;
> >                 } else {
> >                         /*
> >                          * Since the ISP runs synchronous with the IPA and requests,

-- 
Regards,

Laurent Pinchart