[libcamera-devel] [PATCH v2 3/4] pipeline: rpi: Rename RPi::Stream::setExternalBuffer()

[Naushir Patuck]

Hi Kieran,

Thank you for the review.

On Mon, 4 Sept 2023 at 10:01, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Naushir Patuck via libcamera-devel (2023-07-25 09:55:39)
> > Since we don't distinguish between externally and internally allocated
> > dma bufs, rename this function to setExportedBuffer() to clearer on its
> > function.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +-
> >  src/libcamera/pipeline/rpi/common/rpi_stream.cpp    | 2 +-
> >  src/libcamera/pipeline/rpi/common/rpi_stream.h      | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index f244edc68a85..fc0c0ec3c53c 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -764,7 +764,7 @@ int PipelineHandlerBase::queueRequestDevice(Camera *camera, Request *request)
> >                          * outside the v4l2 device. Store it in the stream buffer list
> >                          * so we can track it.
> >                          */
> > -                       stream->setExternalBuffer(buffer);
> > +                       stream->setExportedBuffer(buffer);
>
> I'm a bit confused.
>
> Aren't these (externally allocated buffers) 'imported' buffers ?

I use "exported" as they are exported from the application.  I guess I
was trying to follow the convention from
PipelineHandler::exportFrameBuffer that gets called through the
FrameBufferAllocator route.  I do kind of agree that for both sound
like "import" might be the better term, but I can live it with :-)

>
> Does this conflict with the 'RPi::Stream::setExportedBuffers() call?

Different prototype, but does exactly the same function - it adds one
or more buffers to the availableBuffers_ vector.

>
> That said - both setExportedBuffer and setExportedBuffers perform the
> same action (on each buffer) - so it seems this is expected behaviour...
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> There's a comment in Stream::prepareBuffers()
>    /* Add these exported buffers to the internal/external buffer list. */
>
> Is that incorrect/redundant now? Is this list really tracking
> internal/external buffers?

No, I think that comment is still valid as availableBuffers_ holds all
the buffers (internal or externally allocated) that the stream may
handle during its lifetime.
If any of that doesn't make sense, please shout!

Cheers,
Naush

>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
>
> >                 }
> >
> >                 /*
> > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > index 74b5abf447c7..e1858c731f57 100644
> > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > @@ -76,7 +76,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const
> >         return it->first;
> >  }
> >
> > -void Stream::setExternalBuffer(FrameBuffer *buffer)
> > +void Stream::setExportedBuffer(FrameBuffer *buffer)
> >  {
> >         bufferMap_.emplace(id_.get(), buffer);
> >  }
> > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h
> > index ca591f99cc45..d1289c4679b9 100644
> > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h
> > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h
> > @@ -76,7 +76,7 @@ public:
> >         const BufferMap &getBuffers() const;
> >         unsigned int getBufferId(FrameBuffer *buffer) const;
> >
> > -       void setExternalBuffer(FrameBuffer *buffer);
> > +       void setExportedBuffer(FrameBuffer *buffer);
> >
> >         int prepareBuffers(unsigned int count);
> >         int queueBuffer(FrameBuffer *buffer);
> > --
> > 2.34.1
> >