| Message ID | 20190403150735.27580-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Wed, Apr 03, 2019 at 05:07:30PM +0200, Jacopo Mondi wrote: > Pipeline handlers might need to perform allocation of internal buffers, > setup operations, or simple sanity check before going into the > per-stream buffer allocation. > > As of now, PipelineHandler::allocateBuffers() is called once per each s/per/for/ or s/per each/per/ > active stream, leaving no space for stream-independent configuration. > > Change this by providing to the pipeline handler the full set of active s/handler/handlers/ > stream, and ask them to loop over them to perform per-streams s/stream/streams/ > allocations after eventual stream-independent operations. After or before, it doesn't matter too much, I'd drop that part of the sentence. > The same rational applies to PipelineHandler::freeBuffers() and eventual s/rational/rationale/ s/eventual/optional/ (or "possible") Seems that eventual has the same meaning in Italian than in French, but it's not the case in English :-) > stream-independent clean up operations. > > While at it, change the return type of freeBuffers() to void as it was > not checked by the Camera class method calling it. How about checking it instead ? I think it's useful to propagate errors up. Even if there's not too much an application can do to recover, it could for instance warn the user that an error occurred. > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/camera.cpp | 15 +++++++-------- > src/libcamera/include/pipeline_handler.h | 6 ++++-- > src/libcamera/pipeline/ipu3/ipu3.cpp | 15 +++++++++------ > src/libcamera/pipeline/uvcvideo.cpp | 15 ++++++++++----- > src/libcamera/pipeline/vimc.cpp | 15 ++++++++++----- > src/libcamera/pipeline_handler.cpp | 8 ++++---- > 6 files changed, 44 insertions(+), 30 deletions(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 8ee9cc086616..99eb0e6876f0 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -467,13 +467,11 @@ int Camera::allocateBuffers() > return -EINVAL; > } > > - for (Stream *stream : activeStreams_) { > - int ret = pipe_->allocateBuffers(this, stream); > - if (ret) { > - LOG(Camera, Error) << "Failed to allocate buffers"; > - freeBuffers(); > - return ret; > - } > + int ret = pipe_->allocateBuffers(this, activeStreams_); > + if (ret) { > + LOG(Camera, Error) << "Failed to allocate buffers"; > + freeBuffers(); Do pipeline handlers expect freeBuffers() to be called if pipe_->allocatedBuffers() failed ? This should at least be documented. > + return ret; > } > > state_ = CameraPrepared; > @@ -503,9 +501,10 @@ int Camera::freeBuffers() > * by the V4L2 device that has allocated them. > */ > stream->bufferPool().destroyBuffers(); > - pipe_->freeBuffers(this, stream); > } > > + pipe_->freeBuffers(this, activeStreams_); > + > state_ = CameraConfigured; > > return 0; > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > index acb376e07030..920b57609470 100644 > --- a/src/libcamera/include/pipeline_handler.h > +++ b/src/libcamera/include/pipeline_handler.h > @@ -57,8 +57,10 @@ public: > virtual int configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) = 0; > > - virtual int allocateBuffers(Camera *camera, Stream *stream) = 0; > - virtual int freeBuffers(Camera *camera, Stream *stream) = 0; > + virtual int allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) = 0; > + virtual void freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) = 0; > > virtual int start(Camera *camera) = 0; > virtual void stop(Camera *camera); > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index caf1051c58ab..a12e3f9a496d 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -145,8 +145,10 @@ public: > int configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) override; > > - int allocateBuffers(Camera *camera, Stream *stream) override; > - int freeBuffers(Camera *camera, Stream *stream) override; > + int allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) override; > + void freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) override; > > int start(Camera *camera) override; > void stop(Camera *camera) override; > @@ -426,9 +428,11 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > return 0; > } > > -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) > +int PipelineHandlerIPU3::allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) > { > IPU3CameraData *data = cameraData(camera); > + Stream *stream = *streams.begin(); > CIO2Device *cio2 = &data->cio2_; > ImgUDevice *imgu = data->imgu_; > int ret; > @@ -467,14 +471,13 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) > return 0; > } > > -int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream) > +void PipelineHandlerIPU3::freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) > { > IPU3CameraData *data = cameraData(camera); > > data->cio2_.freeBuffers(); > data->imgu_->freeBuffers(); > - > - return 0; > } > > int PipelineHandlerIPU3::start(Camera *camera) > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index cc3e0cd9afab..128f0c49dba3 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -32,8 +32,10 @@ public: > int configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) override; > > - int allocateBuffers(Camera *camera, Stream *stream) override; > - int freeBuffers(Camera *camera, Stream *stream) override; > + int allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) override; > + void freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) override; > > int start(Camera *camera) override; > void stop(Camera *camera) override; > @@ -129,9 +131,11 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > return 0; > } > > -int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > +int PipelineHandlerUVC::allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) > { > UVCCameraData *data = cameraData(camera); > + Stream *stream = *streams.begin(); > const StreamConfiguration &cfg = stream->configuration(); > > LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > @@ -139,10 +143,11 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > return data->video_->exportBuffers(&stream->bufferPool()); > } > > -int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) > +void PipelineHandlerUVC::freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) > { > UVCCameraData *data = cameraData(camera); > - return data->video_->releaseBuffers(); > + data->video_->releaseBuffers(); > } > > int PipelineHandlerUVC::start(Camera *camera) > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index 2e8c26fb7c0b..6735940799d8 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -32,8 +32,10 @@ public: > int configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) override; > > - int allocateBuffers(Camera *camera, Stream *stream) override; > - int freeBuffers(Camera *camera, Stream *stream) override; > + int allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) override; > + void freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) override; > > int start(Camera *camera) override; > void stop(Camera *camera) override; > @@ -129,9 +131,11 @@ int PipelineHandlerVimc::configureStreams(Camera *camera, > return 0; > } > > -int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) > +int PipelineHandlerVimc::allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) > { > VimcCameraData *data = cameraData(camera); > + Stream *stream = *streams.begin(); > const StreamConfiguration &cfg = stream->configuration(); > > LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > @@ -139,10 +143,11 @@ int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) > return data->video_->exportBuffers(&stream->bufferPool()); > } > > -int PipelineHandlerVimc::freeBuffers(Camera *camera, Stream *stream) > +void PipelineHandlerVimc::freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) > { > VimcCameraData *data = cameraData(camera); > - return data->video_->releaseBuffers(); > + data->video_->releaseBuffers(); > } > > int PipelineHandlerVimc::start(Camera *camera) > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 1a858f2638ce..9a8a4fde57e6 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -193,10 +193,10 @@ PipelineHandler::~PipelineHandler() > * \fn PipelineHandler::allocateBuffers() > * \brief Allocate buffers for a stream > * \param[in] camera The camera the \a stream belongs to > - * \param[in] stream The stream to allocate buffers for > + * \param[in] streams The group of streams to allocate buffers for s/group/set/ ? > * > * This method allocates buffers internally in the pipeline handler and > - * associates them with the stream's buffer pool. > + * associates them with each stream's buffer pool. "This method allocates buffers internally in the pipeline handler for each stream in the \a streams set, and associates them with the stream's buffer pool." ? > * > * The intended caller of this method is the Camera class. > * > @@ -207,9 +207,9 @@ PipelineHandler::~PipelineHandler() > * \fn PipelineHandler::freeBuffers() > * \brief Free all buffers associated with a stream > * \param[in] camera The camera the \a stream belongs to > - * \param[in] stream The stream to free buffers from > + * \param[in] streams The group of streams to free buffers from s/group/set/ > * > - * After a capture session has been stopped all buffers associated with the > + * After a capture session has been stopped all buffers associated with each > * stream shall be freed. > * > * The intended caller of this method is the Camera class.
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 8ee9cc086616..99eb0e6876f0 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -467,13 +467,11 @@ int Camera::allocateBuffers() return -EINVAL; } - for (Stream *stream : activeStreams_) { - int ret = pipe_->allocateBuffers(this, stream); - if (ret) { - LOG(Camera, Error) << "Failed to allocate buffers"; - freeBuffers(); - return ret; - } + int ret = pipe_->allocateBuffers(this, activeStreams_); + if (ret) { + LOG(Camera, Error) << "Failed to allocate buffers"; + freeBuffers(); + return ret; } state_ = CameraPrepared; @@ -503,9 +501,10 @@ int Camera::freeBuffers() * by the V4L2 device that has allocated them. */ stream->bufferPool().destroyBuffers(); - pipe_->freeBuffers(this, stream); } + pipe_->freeBuffers(this, activeStreams_); + state_ = CameraConfigured; return 0; diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h index acb376e07030..920b57609470 100644 --- a/src/libcamera/include/pipeline_handler.h +++ b/src/libcamera/include/pipeline_handler.h @@ -57,8 +57,10 @@ public: virtual int configureStreams(Camera *camera, std::map<Stream *, StreamConfiguration> &config) = 0; - virtual int allocateBuffers(Camera *camera, Stream *stream) = 0; - virtual int freeBuffers(Camera *camera, Stream *stream) = 0; + virtual int allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) = 0; + virtual void freeBuffers(Camera *camera, + const std::set<Stream *> &streams) = 0; virtual int start(Camera *camera) = 0; virtual void stop(Camera *camera); diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index caf1051c58ab..a12e3f9a496d 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -145,8 +145,10 @@ public: int configureStreams(Camera *camera, std::map<Stream *, StreamConfiguration> &config) override; - int allocateBuffers(Camera *camera, Stream *stream) override; - int freeBuffers(Camera *camera, Stream *stream) override; + int allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) override; + void freeBuffers(Camera *camera, + const std::set<Stream *> &streams) override; int start(Camera *camera) override; void stop(Camera *camera) override; @@ -426,9 +428,11 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, return 0; } -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) +int PipelineHandlerIPU3::allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) { IPU3CameraData *data = cameraData(camera); + Stream *stream = *streams.begin(); CIO2Device *cio2 = &data->cio2_; ImgUDevice *imgu = data->imgu_; int ret; @@ -467,14 +471,13 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) return 0; } -int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream) +void PipelineHandlerIPU3::freeBuffers(Camera *camera, + const std::set<Stream *> &streams) { IPU3CameraData *data = cameraData(camera); data->cio2_.freeBuffers(); data->imgu_->freeBuffers(); - - return 0; } int PipelineHandlerIPU3::start(Camera *camera) diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index cc3e0cd9afab..128f0c49dba3 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -32,8 +32,10 @@ public: int configureStreams(Camera *camera, std::map<Stream *, StreamConfiguration> &config) override; - int allocateBuffers(Camera *camera, Stream *stream) override; - int freeBuffers(Camera *camera, Stream *stream) override; + int allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) override; + void freeBuffers(Camera *camera, + const std::set<Stream *> &streams) override; int start(Camera *camera) override; void stop(Camera *camera) override; @@ -129,9 +131,11 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, return 0; } -int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) +int PipelineHandlerUVC::allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) { UVCCameraData *data = cameraData(camera); + Stream *stream = *streams.begin(); const StreamConfiguration &cfg = stream->configuration(); LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; @@ -139,10 +143,11 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) return data->video_->exportBuffers(&stream->bufferPool()); } -int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) +void PipelineHandlerUVC::freeBuffers(Camera *camera, + const std::set<Stream *> &streams) { UVCCameraData *data = cameraData(camera); - return data->video_->releaseBuffers(); + data->video_->releaseBuffers(); } int PipelineHandlerUVC::start(Camera *camera) diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index 2e8c26fb7c0b..6735940799d8 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -32,8 +32,10 @@ public: int configureStreams(Camera *camera, std::map<Stream *, StreamConfiguration> &config) override; - int allocateBuffers(Camera *camera, Stream *stream) override; - int freeBuffers(Camera *camera, Stream *stream) override; + int allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) override; + void freeBuffers(Camera *camera, + const std::set<Stream *> &streams) override; int start(Camera *camera) override; void stop(Camera *camera) override; @@ -129,9 +131,11 @@ int PipelineHandlerVimc::configureStreams(Camera *camera, return 0; } -int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) +int PipelineHandlerVimc::allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) { VimcCameraData *data = cameraData(camera); + Stream *stream = *streams.begin(); const StreamConfiguration &cfg = stream->configuration(); LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; @@ -139,10 +143,11 @@ int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) return data->video_->exportBuffers(&stream->bufferPool()); } -int PipelineHandlerVimc::freeBuffers(Camera *camera, Stream *stream) +void PipelineHandlerVimc::freeBuffers(Camera *camera, + const std::set<Stream *> &streams) { VimcCameraData *data = cameraData(camera); - return data->video_->releaseBuffers(); + data->video_->releaseBuffers(); } int PipelineHandlerVimc::start(Camera *camera) diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 1a858f2638ce..9a8a4fde57e6 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -193,10 +193,10 @@ PipelineHandler::~PipelineHandler() * \fn PipelineHandler::allocateBuffers() * \brief Allocate buffers for a stream * \param[in] camera The camera the \a stream belongs to - * \param[in] stream The stream to allocate buffers for + * \param[in] streams The group of streams to allocate buffers for * * This method allocates buffers internally in the pipeline handler and - * associates them with the stream's buffer pool. + * associates them with each stream's buffer pool. * * The intended caller of this method is the Camera class. * @@ -207,9 +207,9 @@ PipelineHandler::~PipelineHandler() * \fn PipelineHandler::freeBuffers() * \brief Free all buffers associated with a stream * \param[in] camera The camera the \a stream belongs to - * \param[in] stream The stream to free buffers from + * \param[in] streams The group of streams to free buffers from * - * After a capture session has been stopped all buffers associated with the + * After a capture session has been stopped all buffers associated with each * stream shall be freed. * * The intended caller of this method is the Camera class.