| Message ID | 20230426131057.21550-6-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Wed, Apr 26, 2023 at 02:10:49PM +0100, Naushir Patuck via libcamera-devel wrote: > At present, the RPiStream buffer ids == -1 indicates an invalid value. > As a simplification, use id == 0 to indicate an invalid value. This > allows for better code readability. > > As a consequence of this, use unsigned int for the buffer id values. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > .../pipeline/rpi/common/rpi_stream.cpp | 10 +++++----- > src/libcamera/pipeline/rpi/common/rpi_stream.h | 18 +++++++++--------- > src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp | 6 +++--- > 3 files changed, 17 insertions(+), 17 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > index 2bb10f25d6ca..3690667e9aa6 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > @@ -55,17 +55,17 @@ const BufferMap &Stream::getBuffers() const > return bufferMap_; > } > > -int Stream::getBufferId(FrameBuffer *buffer) const > +unsigned int Stream::getBufferId(FrameBuffer *buffer) const > { > if (importOnly_) > - return -1; > + return 0; > > /* Find the buffer in the map, and return the buffer id. */ > auto it = std::find_if(bufferMap_.begin(), bufferMap_.end(), > [&buffer](auto const &p) { return p.second == buffer; }); > > if (it == bufferMap_.end()) > - return -1; > + return 0; > > return it->first; > } > @@ -77,10 +77,10 @@ void Stream::setExternalBuffer(FrameBuffer *buffer) > > void Stream::removeExternalBuffer(FrameBuffer *buffer) > { > - int id = getBufferId(buffer); > + unsigned int id = getBufferId(buffer); > > /* Ensure we have this buffer in the stream, and it is marked external. */ > - ASSERT(id != -1 && (id & BufferMask::MaskExternalBuffer)); > + ASSERT(id & BufferMask::MaskExternalBuffer); > bufferMap_.erase(id); > } > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > index b8bd79cf1535..1aae674967e1 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > @@ -58,7 +58,7 @@ public: > > void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers); > const BufferMap &getBuffers() const; > - int getBufferId(FrameBuffer *buffer) const; > + unsigned int getBufferId(FrameBuffer *buffer) const; > > void setExternalBuffer(FrameBuffer *buffer); > void removeExternalBuffer(FrameBuffer *buffer); > @@ -74,25 +74,25 @@ private: > class IdGenerator > { > public: > - IdGenerator(int max) > + IdGenerator(unsigned int max) > : max_(max), id_(0) > { > } > > - int get() > + unsigned int get() > { > - int id; > + unsigned int id; > if (!recycle_.empty()) { > id = recycle_.front(); > recycle_.pop(); > } else { > - id = id_++; > + id = ++id_; > ASSERT(id_ <= max_); > } > return id; > } > > - void release(int id) > + void release(unsigned int id) > { > recycle_.push(id); > } > @@ -104,9 +104,9 @@ private: > } > > private: > - int max_; > - int id_; > - std::queue<int> recycle_; > + unsigned int max_; > + unsigned int id_; > + std::queue<unsigned int> recycle_; > }; > > void clearBuffers(); > diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp > index cfde7f2e2229..e54bf1ef2c17 100644 > --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp > @@ -1210,7 +1210,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request) > continue; > > FrameBuffer *buffer = request->findBuffer(stream); > - if (buffer && stream->getBufferId(buffer) == -1) { > + if (buffer && !stream->getBufferId(buffer)) { > /* > * This buffer is not recognised, so it must have been allocated > * outside the v4l2 device. Store it in the stream buffer list > @@ -2042,7 +2042,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > for (RPi::Stream &s : unicam_) { > index = s.getBufferId(buffer); > - if (index != -1) { > + if (index) { > stream = &s; > break; > } > @@ -2098,7 +2098,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) > > for (RPi::Stream &s : isp_) { > index = s.getBufferId(buffer); > - if (index != -1) { > + if (index) { > stream = &s; > break; > } > -- > 2.34.1 >
Hi Naush, Thank you for the patch. On Wed, Apr 26, 2023 at 02:10:49PM +0100, Naushir Patuck via libcamera-devel wrote: > At present, the RPiStream buffer ids == -1 indicates an invalid value. > As a simplification, use id == 0 to indicate an invalid value. This > allows for better code readability. > > As a consequence of this, use unsigned int for the buffer id values. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../pipeline/rpi/common/rpi_stream.cpp | 10 +++++----- > src/libcamera/pipeline/rpi/common/rpi_stream.h | 18 +++++++++--------- > src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp | 6 +++--- > 3 files changed, 17 insertions(+), 17 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > index 2bb10f25d6ca..3690667e9aa6 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > @@ -55,17 +55,17 @@ const BufferMap &Stream::getBuffers() const > return bufferMap_; > } > > -int Stream::getBufferId(FrameBuffer *buffer) const > +unsigned int Stream::getBufferId(FrameBuffer *buffer) const > { > if (importOnly_) > - return -1; > + return 0; > > /* Find the buffer in the map, and return the buffer id. */ > auto it = std::find_if(bufferMap_.begin(), bufferMap_.end(), > [&buffer](auto const &p) { return p.second == buffer; }); > > if (it == bufferMap_.end()) > - return -1; > + return 0; > > return it->first; > } > @@ -77,10 +77,10 @@ void Stream::setExternalBuffer(FrameBuffer *buffer) > > void Stream::removeExternalBuffer(FrameBuffer *buffer) > { > - int id = getBufferId(buffer); > + unsigned int id = getBufferId(buffer); > > /* Ensure we have this buffer in the stream, and it is marked external. */ > - ASSERT(id != -1 && (id & BufferMask::MaskExternalBuffer)); > + ASSERT(id & BufferMask::MaskExternalBuffer); > bufferMap_.erase(id); > } > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > index b8bd79cf1535..1aae674967e1 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > @@ -58,7 +58,7 @@ public: > > void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers); > const BufferMap &getBuffers() const; > - int getBufferId(FrameBuffer *buffer) const; > + unsigned int getBufferId(FrameBuffer *buffer) const; > > void setExternalBuffer(FrameBuffer *buffer); > void removeExternalBuffer(FrameBuffer *buffer); > @@ -74,25 +74,25 @@ private: > class IdGenerator > { > public: > - IdGenerator(int max) > + IdGenerator(unsigned int max) > : max_(max), id_(0) > { > } > > - int get() > + unsigned int get() > { > - int id; > + unsigned int id; > if (!recycle_.empty()) { > id = recycle_.front(); > recycle_.pop(); > } else { > - id = id_++; > + id = ++id_; > ASSERT(id_ <= max_); > } > return id; > } > > - void release(int id) > + void release(unsigned int id) > { > recycle_.push(id); > } > @@ -104,9 +104,9 @@ private: > } > > private: > - int max_; > - int id_; > - std::queue<int> recycle_; > + unsigned int max_; > + unsigned int id_; > + std::queue<unsigned int> recycle_; > }; > > void clearBuffers(); > diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp > index cfde7f2e2229..e54bf1ef2c17 100644 > --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp > @@ -1210,7 +1210,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request) > continue; > > FrameBuffer *buffer = request->findBuffer(stream); > - if (buffer && stream->getBufferId(buffer) == -1) { > + if (buffer && !stream->getBufferId(buffer)) { > /* > * This buffer is not recognised, so it must have been allocated > * outside the v4l2 device. Store it in the stream buffer list > @@ -2042,7 +2042,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > for (RPi::Stream &s : unicam_) { > index = s.getBufferId(buffer); > - if (index != -1) { > + if (index) { > stream = &s; > break; > } > @@ -2098,7 +2098,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) > > for (RPi::Stream &s : isp_) { > index = s.getBufferId(buffer); > - if (index != -1) { > + if (index) { > stream = &s; > break; > }
diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp index 2bb10f25d6ca..3690667e9aa6 100644 --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp @@ -55,17 +55,17 @@ const BufferMap &Stream::getBuffers() const return bufferMap_; } -int Stream::getBufferId(FrameBuffer *buffer) const +unsigned int Stream::getBufferId(FrameBuffer *buffer) const { if (importOnly_) - return -1; + return 0; /* Find the buffer in the map, and return the buffer id. */ auto it = std::find_if(bufferMap_.begin(), bufferMap_.end(), [&buffer](auto const &p) { return p.second == buffer; }); if (it == bufferMap_.end()) - return -1; + return 0; return it->first; } @@ -77,10 +77,10 @@ void Stream::setExternalBuffer(FrameBuffer *buffer) void Stream::removeExternalBuffer(FrameBuffer *buffer) { - int id = getBufferId(buffer); + unsigned int id = getBufferId(buffer); /* Ensure we have this buffer in the stream, and it is marked external. */ - ASSERT(id != -1 && (id & BufferMask::MaskExternalBuffer)); + ASSERT(id & BufferMask::MaskExternalBuffer); bufferMap_.erase(id); } diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h index b8bd79cf1535..1aae674967e1 100644 --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h @@ -58,7 +58,7 @@ public: void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers); const BufferMap &getBuffers() const; - int getBufferId(FrameBuffer *buffer) const; + unsigned int getBufferId(FrameBuffer *buffer) const; void setExternalBuffer(FrameBuffer *buffer); void removeExternalBuffer(FrameBuffer *buffer); @@ -74,25 +74,25 @@ private: class IdGenerator { public: - IdGenerator(int max) + IdGenerator(unsigned int max) : max_(max), id_(0) { } - int get() + unsigned int get() { - int id; + unsigned int id; if (!recycle_.empty()) { id = recycle_.front(); recycle_.pop(); } else { - id = id_++; + id = ++id_; ASSERT(id_ <= max_); } return id; } - void release(int id) + void release(unsigned int id) { recycle_.push(id); } @@ -104,9 +104,9 @@ private: } private: - int max_; - int id_; - std::queue<int> recycle_; + unsigned int max_; + unsigned int id_; + std::queue<unsigned int> recycle_; }; void clearBuffers(); diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp index cfde7f2e2229..e54bf1ef2c17 100644 --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp @@ -1210,7 +1210,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request) continue; FrameBuffer *buffer = request->findBuffer(stream); - if (buffer && stream->getBufferId(buffer) == -1) { + if (buffer && !stream->getBufferId(buffer)) { /* * This buffer is not recognised, so it must have been allocated * outside the v4l2 device. Store it in the stream buffer list @@ -2042,7 +2042,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) for (RPi::Stream &s : unicam_) { index = s.getBufferId(buffer); - if (index != -1) { + if (index) { stream = &s; break; } @@ -2098,7 +2098,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) for (RPi::Stream &s : isp_) { index = s.getBufferId(buffer); - if (index != -1) { + if (index) { stream = &s; break; }
At present, the RPiStream buffer ids == -1 indicates an invalid value. As a simplification, use id == 0 to indicate an invalid value. This allows for better code readability. As a consequence of this, use unsigned int for the buffer id values. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/rpi/common/rpi_stream.cpp | 10 +++++----- src/libcamera/pipeline/rpi/common/rpi_stream.h | 18 +++++++++--------- src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp | 6 +++--- 3 files changed, 17 insertions(+), 17 deletions(-)