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

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback.

On Sun, 22 Jan 2023 at 21:57, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.

Ack.

>
> > >         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

Since we don't support YUV sensors in the pipeline handler (just yet), the
behavior here is identical.  The reason for the change is to be more explicit
with the stream identification, which becomes important when we add a test for
Isp::Output0 in a subsequent change.

.
>
> > >                         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.

The overrides are needed to ensure we can correctly drop frames by using at
least one internal Unicam buffer. I will add a note the explain this behavior
in the config file.

Regards,
Naush


>
> > > +                       }
> > > +
> > >                         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