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

[Kieran Bingham]

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