[{"id":2998,"web_url":"https://patchwork.libcamera.org/comment/2998/","msgid":"<20191106084927.vfkdx6tmtr4jukga@uno.localdomain>","date":"2019-11-06T08:49:27","subject":"Re: [libcamera-devel] [RFC 02/12] libcamera: pipelines: Explicitly\n\tallocate streams","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Oct 28, 2019 at 03:25:15AM +0100, Niklas Söderlund wrote:\n> Prepare for sub-classing the Stream class with a V4L2 specific\n> implementation which will need to be constructed later when knowledge\n> about the V4L2 video device is available.\n>\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 60 +++++++++++++-----------\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++----\n>  src/libcamera/pipeline/uvcvideo.cpp      | 13 +++--\n>  src/libcamera/pipeline/vimc.cpp          | 14 ++++--\n>  4 files changed, 60 insertions(+), 46 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 06ee6ccac641eb41..ff90b729e558c3a3 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -137,8 +137,8 @@ public:\n>  class IPU3Stream : public Stream\n>  {\n>  public:\n> -\tIPU3Stream()\n> -\t\t: active_(false), device_(nullptr)\n> +\tIPU3Stream(ImgUDevice::ImgUOutput *device, const std::string &name)\n> +\t\t: active_(false), name_(name), device_(device)\n>  \t{\n>  \t}\n>\n> @@ -151,10 +151,16 @@ class IPU3CameraData : public CameraData\n>  {\n>  public:\n>  \tIPU3CameraData(PipelineHandler *pipe)\n> -\t\t: CameraData(pipe)\n> +\t\t: CameraData(pipe), outStream_(nullptr), vfStream_(nullptr)\n>  \t{\n>  \t}\n>\n> +\t~IPU3CameraData()\n> +\t{\n> +\t\tdelete outStream_;\n> +\t\tdelete vfStream_;\n> +\t}\n\nHave you considered making these and the other stream pointers in the\npatch unique_ptr<> to avoid tracking their lifecycle ?\n\nLooking at the usage pattern it's probably not worth it though, as\nthey're part of CameraData which is a unique_ptr of its own.\n\nThe rest of the patch looks good, thanks!\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n   j\n\n> +\n>  \tvoid imguOutputBufferReady(Buffer *buffer);\n>  \tvoid imguInputBufferReady(Buffer *buffer);\n>  \tvoid cio2BufferReady(Buffer *buffer);\n> @@ -162,8 +168,8 @@ public:\n>  \tCIO2Device cio2_;\n>  \tImgUDevice *imgu_;\n>\n> -\tIPU3Stream outStream_;\n> -\tIPU3Stream vfStream_;\n> +\tIPU3Stream *outStream_;\n> +\tIPU3Stream *vfStream_;\n>\n>  \tstd::vector<std::unique_ptr<Buffer>> rawBuffers_;\n>  };\n> @@ -342,8 +348,8 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  \t * stream otherwise.\n>  \t */\n>  \tstd::set<const IPU3Stream *> availableStreams = {\n> -\t\t&data_->outStream_,\n> -\t\t&data_->vfStream_,\n> +\t\tdata_->outStream_,\n> +\t\tdata_->vfStream_,\n>  \t};\n>\n>  \tstreams_.clear();\n> @@ -356,9 +362,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  \t\tconst IPU3Stream *stream;\n>\n>  \t\tif (cfg.size == sensorFormat_.size)\n> -\t\t\tstream = &data_->outStream_;\n> +\t\t\tstream = data_->outStream_;\n>  \t\telse\n> -\t\t\tstream = &data_->vfStream_;\n> +\t\t\tstream = data_->vfStream_;\n>\n>  \t\tif (availableStreams.find(stream) == availableStreams.end())\n>  \t\t\tstream = *availableStreams.begin();\n> @@ -366,7 +372,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  \t\tLOG(IPU3, Debug)\n>  \t\t\t<< \"Assigned '\" << stream->name_ << \"' to stream \" << i;\n>\n> -\t\tbool scale = stream == &data_->vfStream_;\n> +\t\tbool scale = stream == data_->vfStream_;\n>  \t\tadjustStream(config_[i], scale);\n>\n>  \t\tif (cfg.pixelFormat != pixelFormat || cfg.size != size) {\n> @@ -394,8 +400,8 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \tIPU3CameraConfiguration *config;\n>  \tstd::set<IPU3Stream *> streams = {\n> -\t\t&data->outStream_,\n> -\t\t&data->vfStream_,\n> +\t\tdata->outStream_,\n> +\t\tdata->vfStream_,\n>  \t};\n>\n>  \tconfig = new IPU3CameraConfiguration(camera, data);\n> @@ -413,10 +419,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \t\t\t * and VideoRecording roles are not allowed on\n>  \t\t\t * the output stream.\n>  \t\t\t */\n> -\t\t\tif (streams.find(&data->outStream_) != streams.end()) {\n> -\t\t\t\tstream = &data->outStream_;\n> -\t\t\t} else if (streams.find(&data->vfStream_) != streams.end()) {\n> -\t\t\t\tstream = &data->vfStream_;\n> +\t\t\tif (streams.find(data->outStream_) != streams.end()) {\n> +\t\t\t\tstream = data->outStream_;\n> +\t\t\t} else if (streams.find(data->vfStream_) != streams.end()) {\n> +\t\t\t\tstream = data->vfStream_;\n>  \t\t\t} else {\n>  \t\t\t\tLOG(IPU3, Error)\n>  \t\t\t\t\t<< \"No stream available for requested role \"\n> @@ -446,14 +452,14 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \t\t\t * \\todo This is an artificial limitation until we\n>  \t\t\t * figure out the exact capabilities of the hardware.\n>  \t\t\t */\n> -\t\t\tif (streams.find(&data->vfStream_) == streams.end()) {\n> +\t\t\tif (streams.find(data->vfStream_) == streams.end()) {\n>  \t\t\t\tLOG(IPU3, Error)\n>  \t\t\t\t\t<< \"No stream available for requested role \"\n>  \t\t\t\t\t<< role;\n>  \t\t\t\tbreak;\n>  \t\t\t}\n>\n> -\t\t\tstream = &data->vfStream_;\n> +\t\t\tstream = data->vfStream_;\n>\n>  \t\t\t/*\n>  \t\t\t * Align the default viewfinder size to the maximum\n> @@ -494,8 +500,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>  \tIPU3CameraConfiguration *config =\n>  \t\tstatic_cast<IPU3CameraConfiguration *>(c);\n>  \tIPU3CameraData *data = cameraData(camera);\n> -\tIPU3Stream *outStream = &data->outStream_;\n> -\tIPU3Stream *vfStream = &data->vfStream_;\n> +\tIPU3Stream *outStream = data->outStream_;\n> +\tIPU3Stream *vfStream = data->vfStream_;\n>  \tCIO2Device *cio2 = &data->cio2_;\n>  \tImgUDevice *imgu = data->imgu_;\n>  \tint ret;\n> @@ -629,8 +635,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n>  \t\t\t\t\t const std::set<Stream *> &streams)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> -\tIPU3Stream *outStream = &data->outStream_;\n> -\tIPU3Stream *vfStream = &data->vfStream_;\n> +\tIPU3Stream *outStream = data->outStream_;\n> +\tIPU3Stream *vfStream = data->vfStream_;\n>  \tCIO2Device *cio2 = &data->cio2_;\n>  \tImgUDevice *imgu = data->imgu_;\n>  \tunsigned int bufferCount;\n> @@ -858,8 +864,8 @@ int PipelineHandlerIPU3::registerCameras()\n>  \t\tstd::unique_ptr<IPU3CameraData> data =\n>  \t\t\tutils::make_unique<IPU3CameraData>(this);\n>  \t\tstd::set<Stream *> streams = {\n> -\t\t\t&data->outStream_,\n> -\t\t\t&data->vfStream_,\n> +\t\t\tdata->outStream_,\n> +\t\t\tdata->vfStream_,\n>  \t\t};\n>  \t\tCIO2Device *cio2 = &data->cio2_;\n>\n> @@ -874,10 +880,8 @@ int PipelineHandlerIPU3::registerCameras()\n>  \t\t * second.\n>  \t\t */\n>  \t\tdata->imgu_ = numCameras ? &imgu1_ : &imgu0_;\n> -\t\tdata->outStream_.device_ = &data->imgu_->output_;\n> -\t\tdata->outStream_.name_ = \"output\";\n> -\t\tdata->vfStream_.device_ = &data->imgu_->viewfinder_;\n> -\t\tdata->vfStream_.name_ = \"viewfinder\";\n> +\t\tdata->outStream_ = new IPU3Stream(&data->imgu_->output_, \"output\");\n> +\t\tdata->vfStream_ = new IPU3Stream(&data->imgu_->viewfinder_, \"viewfinder\");\n>\n>  \t\t/*\n>  \t\t * Connect video devices' 'bufferReady' signals to their\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index a99df4f8b846d8b7..cefdb54a06d440fb 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -116,19 +116,20 @@ class RkISP1CameraData : public CameraData\n>  {\n>  public:\n>  \tRkISP1CameraData(PipelineHandler *pipe)\n> -\t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> -\t\t  frameInfo_(pipe)\n> +\t\t: CameraData(pipe), stream_(nullptr), sensor_(nullptr),\n> +\t\t  frame_(0), frameInfo_(pipe)\n>  \t{\n>  \t}\n>\n>  \t~RkISP1CameraData()\n>  \t{\n> +\t\tdelete stream_;\n>  \t\tdelete sensor_;\n>  \t}\n>\n>  \tint loadIPA();\n>\n> -\tStream stream_;\n> +\tStream *stream_;\n>  \tCameraSensor *sensor_;\n>  \tunsigned int frame_;\n>  \tstd::vector<IPABuffer> ipaBuffers_;\n> @@ -650,7 +651,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>  \tif (ret)\n>  \t\treturn ret;\n>\n> -\tcfg.setStream(&data->stream_);\n> +\tcfg.setStream(data->stream_);\n>\n>  \treturn 0;\n>  }\n> @@ -772,8 +773,8 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>  \t/* Inform IPA of stream configuration and sensor controls. */\n>  \tstd::map<unsigned int, IPAStream> streamConfig;\n>  \tstreamConfig[0] = {\n> -\t\t.pixelFormat = data->stream_.configuration().pixelFormat,\n> -\t\t.size = data->stream_.configuration().size,\n> +\t\t.pixelFormat = data->stream_->configuration().pixelFormat,\n> +\t\t.size = data->stream_->configuration().size,\n>  \t};\n>\n>  \tstd::map<unsigned int, ControlInfoMap> entityControls;\n> @@ -812,7 +813,7 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n>  int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)\n>  {\n>  \tRkISP1CameraData *data = cameraData(camera);\n> -\tStream *stream = &data->stream_;\n> +\tStream *stream = data->stream_;\n>\n>  \tRkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,\n>  \t\t\t\t\t\t\tstream);\n> @@ -873,6 +874,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>  \tstd::unique_ptr<RkISP1CameraData> data =\n>  \t\tutils::make_unique<RkISP1CameraData>(this);\n>\n> +\tdata->stream_ = new Stream();\n> +\n>  \tControlInfoMap::Map ctrls;\n>  \tctrls.emplace(std::piecewise_construct,\n>  \t\t      std::forward_as_tuple(&controls::AeEnable),\n> @@ -889,7 +892,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>  \tif (ret)\n>  \t\treturn ret;\n>\n> -\tstd::set<Stream *> streams{ &data->stream_ };\n> +\tstd::set<Stream *> streams{ data->stream_ };\n>  \tstd::shared_ptr<Camera> camera =\n>  \t\tCamera::create(this, sensor->name(), streams);\n>  \tregisterCamera(std::move(camera), std::move(data));\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index dc17b456af6987e6..5aa6a8f9e0b2b7a4 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -31,12 +31,13 @@ class UVCCameraData : public CameraData\n>  {\n>  public:\n>  \tUVCCameraData(PipelineHandler *pipe)\n> -\t\t: CameraData(pipe), video_(nullptr)\n> +\t\t: CameraData(pipe), video_(nullptr), stream_(nullptr)\n>  \t{\n>  \t}\n>\n>  \t~UVCCameraData()\n>  \t{\n> +\t\tdelete stream_;\n>  \t\tdelete video_;\n>  \t}\n>\n> @@ -44,7 +45,7 @@ public:\n>  \tvoid bufferReady(Buffer *buffer);\n>\n>  \tV4L2VideoDevice *video_;\n> -\tStream stream_;\n> +\tStream *stream_;\n>  };\n>\n>  class UVCCameraConfiguration : public CameraConfiguration\n> @@ -187,7 +188,7 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n>  \t    format.fourcc != cfg.pixelFormat)\n>  \t\treturn -EINVAL;\n>\n> -\tcfg.setStream(&data->stream_);\n> +\tcfg.setStream(data->stream_);\n>\n>  \treturn 0;\n>  }\n> @@ -265,7 +266,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n>  int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n>  {\n>  \tUVCCameraData *data = cameraData(camera);\n> -\tBuffer *buffer = request->findBuffer(&data->stream_);\n> +\tBuffer *buffer = request->findBuffer(data->stream_);\n>  \tif (!buffer) {\n>  \t\tLOG(UVC, Error)\n>  \t\t\t<< \"Attempt to queue request with invalid stream\";\n> @@ -310,7 +311,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>  \t}\n>\n>  \t/* Create and register the camera. */\n> -\tstd::set<Stream *> streams{ &data->stream_ };\n> +\tstd::set<Stream *> streams{ data->stream_ };\n>  \tstd::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);\n>  \tregisterCamera(std::move(camera), std::move(data));\n>\n> @@ -330,6 +331,8 @@ int UVCCameraData::init(MediaEntity *entity)\n>  \tif (ret)\n>  \t\treturn ret;\n>\n> +\tstream_ = new Stream();\n> +\n>  \tvideo_->bufferReady.connect(this, &UVCCameraData::bufferReady);\n>\n>  \t/* Initialise the supported controls. */\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 5f83ae2b85997828..49b850cf0153020f 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -40,7 +40,8 @@ class VimcCameraData : public CameraData\n>  public:\n>  \tVimcCameraData(PipelineHandler *pipe)\n>  \t\t: CameraData(pipe), sensor_(nullptr), debayer_(nullptr),\n> -\t\t  scaler_(nullptr), video_(nullptr), raw_(nullptr)\n> +\t\t  scaler_(nullptr), video_(nullptr), raw_(nullptr),\n> +\t\t  stream_(nullptr)\n>  \t{\n>  \t}\n>\n> @@ -51,6 +52,7 @@ public:\n>  \t\tdelete scaler_;\n>  \t\tdelete video_;\n>  \t\tdelete raw_;\n> +\t\tdelete stream_;\n>  \t}\n>\n>  \tint init(MediaDevice *media);\n> @@ -61,7 +63,7 @@ public:\n>  \tV4L2Subdevice *scaler_;\n>  \tV4L2VideoDevice *video_;\n>  \tV4L2VideoDevice *raw_;\n> -\tStream stream_;\n> +\tStream *stream_;\n>  };\n>\n>  class VimcCameraConfiguration : public CameraConfiguration\n> @@ -253,7 +255,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n>  \tif (ret)\n>  \t\treturn ret;\n>\n> -\tcfg.setStream(&data->stream_);\n> +\tcfg.setStream(data->stream_);\n>\n>  \treturn 0;\n>  }\n> @@ -325,7 +327,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n>  int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n>  {\n>  \tVimcCameraData *data = cameraData(camera);\n> -\tBuffer *buffer = request->findBuffer(&data->stream_);\n> +\tBuffer *buffer = request->findBuffer(data->stream_);\n>  \tif (!buffer) {\n>  \t\tLOG(VIMC, Error)\n>  \t\t\t<< \"Attempt to queue request with invalid stream\";\n> @@ -375,7 +377,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n>  \t\treturn false;\n>\n>  \t/* Create and register the camera. */\n> -\tstd::set<Stream *> streams{ &data->stream_ };\n> +\tstd::set<Stream *> streams{ data->stream_ };\n>  \tstd::shared_ptr<Camera> camera = Camera::create(this, \"VIMC Sensor B\",\n>  \t\t\t\t\t\t\tstreams);\n>  \tregisterCamera(std::move(camera), std::move(data));\n> @@ -417,6 +419,8 @@ int VimcCameraData::init(MediaDevice *media)\n>  \tif (video_->open())\n>  \t\treturn -ENODEV;\n>\n> +\tstream_ = new Stream();\n> +\n>  \tvideo_->bufferReady.connect(this, &VimcCameraData::bufferReady);\n>\n>  \traw_ = new V4L2VideoDevice(media->getEntityByName(\"Raw Capture 1\"));\n> --\n> 2.23.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 97C4E60C41\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  6 Nov 2019 09:47:31 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 299F320000F;\n\tWed,  6 Nov 2019 08:47:30 +0000 (UTC)"],"Date":"Wed, 6 Nov 2019 09:49:27 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191106084927.vfkdx6tmtr4jukga@uno.localdomain>","References":"<20191028022525.796995-1-niklas.soderlund@ragnatech.se>\n\t<20191028022525.796995-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"jcgqzgq33slc36or\"","Content-Disposition":"inline","In-Reply-To":"<20191028022525.796995-3-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [RFC 02/12] libcamera: pipelines: Explicitly\n\tallocate streams","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 06 Nov 2019 08:47:32 -0000"}}]