[libcamera-devel] [PATCH 21/30] libcamera: pipelines: Explicitly allocate streams
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Dec 15 03:01:47 CET 2019
Hi Niklas,
Thank you for the patch.
On Wed, Nov 27, 2019 at 12:36:11AM +0100, Niklas Söderlund wrote:
> Prepare for sub-classing the Stream class with a V4L2 specific
> implementation which will need to be constructed later when knowledge
> about the V4L2 video device is available.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
As stated in the review of 23/30, I would merge the two patches. The
subject line of this patch should then mention switching to V4L2Stream
instead of explicitly allocating streams and the body of the commit
message should be updated accordingly.
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 64 +++++++++++++-----------
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++---
> src/libcamera/pipeline/uvcvideo.cpp | 13 +++--
> src/libcamera/pipeline/vimc.cpp | 14 ++++--
> 4 files changed, 62 insertions(+), 48 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 8ba08351c950f5e2..094c4db6bc32b683 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -138,8 +138,8 @@ public:
> class IPU3Stream : public Stream
> {
> public:
> - IPU3Stream()
> - : active_(false), device_(nullptr)
> + IPU3Stream(ImgUDevice::ImgUOutput *device, const std::string &name)
> + : active_(false), name_(name), device_(device)
> {
> }
>
> @@ -152,10 +152,16 @@ class IPU3CameraData : public CameraData
> {
> public:
> IPU3CameraData(PipelineHandler *pipe)
> - : CameraData(pipe)
> + : CameraData(pipe), outStream_(nullptr), vfStream_(nullptr)
> {
> }
>
> + ~IPU3CameraData()
> + {
> + delete outStream_;
> + delete vfStream_;
> + }
> +
> void imguOutputBufferReady(Buffer *buffer);
> void imguInputBufferReady(Buffer *buffer);
> void cio2BufferReady(Buffer *buffer);
> @@ -163,8 +169,8 @@ public:
> CIO2Device cio2_;
> ImgUDevice *imgu_;
>
> - IPU3Stream outStream_;
> - IPU3Stream vfStream_;
> + IPU3Stream *outStream_;
> + IPU3Stream *vfStream_;
>
> std::vector<std::unique_ptr<Buffer>> rawBuffers_;
> };
> @@ -343,8 +349,8 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> * stream otherwise.
> */
> std::set<const IPU3Stream *> availableStreams = {
> - &data_->outStream_,
> - &data_->vfStream_,
> + data_->outStream_,
> + data_->vfStream_,
> };
>
> streams_.clear();
> @@ -357,9 +363,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> const IPU3Stream *stream;
>
> if (cfg.size == sensorFormat_.size)
> - stream = &data_->outStream_;
> + stream = data_->outStream_;
> else
> - stream = &data_->vfStream_;
> + stream = data_->vfStream_;
>
> if (availableStreams.find(stream) == availableStreams.end())
> stream = *availableStreams.begin();
> @@ -367,7 +373,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> LOG(IPU3, Debug)
> << "Assigned '" << stream->name_ << "' to stream " << i;
>
> - bool scale = stream == &data_->vfStream_;
> + bool scale = stream == data_->vfStream_;
> adjustStream(config_[i], scale);
>
> if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> @@ -395,8 +401,8 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> IPU3CameraData *data = cameraData(camera);
> IPU3CameraConfiguration *config;
> std::set<IPU3Stream *> streams = {
> - &data->outStream_,
> - &data->vfStream_,
> + data->outStream_,
> + data->vfStream_,
> };
>
> config = new IPU3CameraConfiguration(camera, data);
> @@ -414,10 +420,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> * and VideoRecording roles are not allowed on
> * the output stream.
> */
> - if (streams.find(&data->outStream_) != streams.end()) {
> - stream = &data->outStream_;
> - } else if (streams.find(&data->vfStream_) != streams.end()) {
> - stream = &data->vfStream_;
> + if (streams.find(data->outStream_) != streams.end()) {
> + stream = data->outStream_;
> + } else if (streams.find(data->vfStream_) != streams.end()) {
> + stream = data->vfStream_;
> } else {
> LOG(IPU3, Error)
> << "No stream available for requested role "
> @@ -447,14 +453,14 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> * \todo This is an artificial limitation until we
> * figure out the exact capabilities of the hardware.
> */
> - if (streams.find(&data->vfStream_) == streams.end()) {
> + if (streams.find(data->vfStream_) == streams.end()) {
> LOG(IPU3, Error)
> << "No stream available for requested role "
> << role;
> break;
> }
>
> - stream = &data->vfStream_;
> + stream = data->vfStream_;
>
> /*
> * Align the default viewfinder size to the maximum
> @@ -495,8 +501,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> IPU3CameraConfiguration *config =
> static_cast<IPU3CameraConfiguration *>(c);
> IPU3CameraData *data = cameraData(camera);
> - IPU3Stream *outStream = &data->outStream_;
> - IPU3Stream *vfStream = &data->vfStream_;
> + IPU3Stream *outStream = data->outStream_;
> + IPU3Stream *vfStream = data->vfStream_;
> CIO2Device *cio2 = &data->cio2_;
> ImgUDevice *imgu = data->imgu_;
> int ret;
> @@ -630,8 +636,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams)
> {
> IPU3CameraData *data = cameraData(camera);
> - IPU3Stream *outStream = &data->outStream_;
> - IPU3Stream *vfStream = &data->vfStream_;
> + IPU3Stream *outStream = data->outStream_;
> + IPU3Stream *vfStream = data->vfStream_;
> CIO2Device *cio2 = &data->cio2_;
> ImgUDevice *imgu = data->imgu_;
> unsigned int bufferCount;
> @@ -855,10 +861,6 @@ int PipelineHandlerIPU3::registerCameras()
> for (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {
> std::unique_ptr<IPU3CameraData> data =
> utils::make_unique<IPU3CameraData>(this);
> - std::set<Stream *> streams = {
> - &data->outStream_,
> - &data->vfStream_,
> - };
> CIO2Device *cio2 = &data->cio2_;
>
> ret = cio2->init(cio2MediaDev_, id);
> @@ -872,10 +874,8 @@ int PipelineHandlerIPU3::registerCameras()
> * second.
> */
> data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
> - data->outStream_.device_ = &data->imgu_->output_;
> - data->outStream_.name_ = "output";
> - data->vfStream_.device_ = &data->imgu_->viewfinder_;
> - data->vfStream_.name_ = "viewfinder";
> + data->outStream_ = new IPU3Stream(&data->imgu_->output_, "output");
> + data->vfStream_ = new IPU3Stream(&data->imgu_->viewfinder_, "viewfinder");
>
> /*
> * Connect video devices' 'bufferReady' signals to their
> @@ -895,6 +895,10 @@ int PipelineHandlerIPU3::registerCameras()
> &IPU3CameraData::imguOutputBufferReady);
>
> /* Create and register the Camera instance. */
> + std::set<Stream *> streams = {
> + data->outStream_,
> + data->vfStream_,
> + };
> std::string cameraName = cio2->sensor_->entity()->name() + " "
> + std::to_string(id);
> std::shared_ptr<Camera> camera = Camera::create(this,
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 6ad9b57d8353896c..1ae50bec0d401b3f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -117,19 +117,20 @@ class RkISP1CameraData : public CameraData
> {
> public:
> RkISP1CameraData(PipelineHandler *pipe)
> - : CameraData(pipe), sensor_(nullptr), frame_(0),
> - frameInfo_(pipe)
> + : CameraData(pipe), stream_(nullptr), sensor_(nullptr),
> + frame_(0), frameInfo_(pipe)
> {
> }
>
> ~RkISP1CameraData()
> {
> + delete stream_;
> delete sensor_;
> }
>
> int loadIPA();
>
> - Stream stream_;
> + Stream *stream_;
> CameraSensor *sensor_;
> unsigned int frame_;
> std::vector<IPABuffer> ipaBuffers_;
> @@ -651,7 +652,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> if (ret)
> return ret;
>
> - cfg.setStream(&data->stream_);
> + cfg.setStream(data->stream_);
>
> return 0;
> }
> @@ -785,8 +786,8 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> /* Inform IPA of stream configuration and sensor controls. */
> std::map<unsigned int, IPAStream> streamConfig;
> streamConfig[0] = {
> - .pixelFormat = data->stream_.configuration().pixelFormat,
> - .size = data->stream_.configuration().size,
> + .pixelFormat = data->stream_->configuration().pixelFormat,
> + .size = data->stream_->configuration().size,
> };
>
> std::map<unsigned int, const ControlInfoMap &> entityControls;
> @@ -826,7 +827,7 @@ int PipelineHandlerRkISP1::queueRequestHardware(Camera *camera,
> Request *request)
> {
> RkISP1CameraData *data = cameraData(camera);
> - Stream *stream = &data->stream_;
> + Stream *stream = data->stream_;
>
> RkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,
> stream);
> @@ -887,6 +888,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> std::unique_ptr<RkISP1CameraData> data =
> utils::make_unique<RkISP1CameraData>(this);
>
> + data->stream_ = new Stream();
> +
> ControlInfoMap::Map ctrls;
> ctrls.emplace(std::piecewise_construct,
> std::forward_as_tuple(&controls::AeEnable),
> @@ -903,7 +906,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> if (ret)
> return ret;
>
> - std::set<Stream *> streams{ &data->stream_ };
> + std::set<Stream *> streams{ data->stream_ };
> std::shared_ptr<Camera> camera =
> Camera::create(this, sensor->name(), streams);
> registerCamera(std::move(camera), std::move(data));
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 3a76653ff6dc2b5e..64fc488912e5a82f 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -31,12 +31,13 @@ class UVCCameraData : public CameraData
> {
> public:
> UVCCameraData(PipelineHandler *pipe)
> - : CameraData(pipe), video_(nullptr)
> + : CameraData(pipe), video_(nullptr), stream_(nullptr)
> {
> }
>
> ~UVCCameraData()
> {
> + delete stream_;
> delete video_;
> }
>
> @@ -44,7 +45,7 @@ public:
> void bufferReady(Buffer *buffer);
>
> V4L2VideoDevice *video_;
> - Stream stream_;
> + Stream *stream_;
> };
>
> class UVCCameraConfiguration : public CameraConfiguration
> @@ -187,7 +188,7 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> format.fourcc != data->video_->toV4L2Fourcc(cfg.pixelFormat))
> return -EINVAL;
>
> - cfg.setStream(&data->stream_);
> + cfg.setStream(data->stream_);
>
> return 0;
> }
> @@ -265,7 +266,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> int PipelineHandlerUVC::queueRequestHardware(Camera *camera, Request *request)
> {
> UVCCameraData *data = cameraData(camera);
> - Buffer *buffer = request->findBuffer(&data->stream_);
> + Buffer *buffer = request->findBuffer(data->stream_);
> if (!buffer) {
> LOG(UVC, Error)
> << "Attempt to queue request with invalid stream";
> @@ -310,7 +311,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> }
>
> /* Create and register the camera. */
> - std::set<Stream *> streams{ &data->stream_ };
> + std::set<Stream *> streams{ data->stream_ };
> std::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);
> registerCamera(std::move(camera), std::move(data));
>
> @@ -330,6 +331,8 @@ int UVCCameraData::init(MediaEntity *entity)
> if (ret)
> return ret;
>
> + stream_ = new Stream();
> +
> video_->bufferReady.connect(this, &UVCCameraData::bufferReady);
>
> /* Initialise the supported controls. */
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index f5550a1723668106..3f9e92163642f0c2 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -41,7 +41,8 @@ class VimcCameraData : public CameraData
> public:
> VimcCameraData(PipelineHandler *pipe)
> : CameraData(pipe), sensor_(nullptr), debayer_(nullptr),
> - scaler_(nullptr), video_(nullptr), raw_(nullptr)
> + scaler_(nullptr), video_(nullptr), raw_(nullptr),
> + stream_(nullptr)
> {
> }
>
> @@ -52,6 +53,7 @@ public:
> delete scaler_;
> delete video_;
> delete raw_;
> + delete stream_;
I would delete the stream before the video device that it is constructed
with, otherwise you'll have an invalid pointer that may become an issue
later.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> }
>
> int init(MediaDevice *media);
> @@ -62,7 +64,7 @@ public:
> V4L2Subdevice *scaler_;
> V4L2VideoDevice *video_;
> V4L2VideoDevice *raw_;
> - Stream stream_;
> + Stream *stream_;
> };
>
> class VimcCameraConfiguration : public CameraConfiguration
> @@ -254,7 +256,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> if (ret)
> return ret;
>
> - cfg.setStream(&data->stream_);
> + cfg.setStream(data->stream_);
>
> return 0;
> }
> @@ -326,7 +328,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
> int PipelineHandlerVimc::queueRequestHardware(Camera *camera, Request *request)
> {
> VimcCameraData *data = cameraData(camera);
> - Buffer *buffer = request->findBuffer(&data->stream_);
> + Buffer *buffer = request->findBuffer(data->stream_);
> if (!buffer) {
> LOG(VIMC, Error)
> << "Attempt to queue request with invalid stream";
> @@ -376,7 +378,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> return false;
>
> /* Create and register the camera. */
> - std::set<Stream *> streams{ &data->stream_ };
> + std::set<Stream *> streams{ data->stream_ };
> std::shared_ptr<Camera> camera = Camera::create(this, "VIMC Sensor B",
> streams);
> registerCamera(std::move(camera), std::move(data));
> @@ -418,6 +420,8 @@ int VimcCameraData::init(MediaDevice *media)
> if (video_->open())
> return -ENODEV;
>
> + stream_ = new Stream();
> +
> video_->bufferReady.connect(this, &VimcCameraData::bufferReady);
>
> raw_ = new V4L2VideoDevice(media->getEntityByName("Raw Capture 1"));
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list