[libcamera-devel] [PATCH v2 3/4] pipeline: rpi: Rename RPi::Stream::setExternalBuffer()
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Sep 4 12:09:49 CEST 2023
Quoting Naushir Patuck (2023-09-04 10:35:06)
> 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!
I think it's clearer now. The list doesn't distinguish between
internally/externally allocated buffers - it's a list of /all/ buffers
used by the stream.
--
Kieran
>
> 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
> > >
More information about the libcamera-devel
mailing list