[{"id":4477,"web_url":"https://patchwork.libcamera.org/comment/4477/","msgid":"<20200421162302.GF2600980@oden.dyn.berto.se>","date":"2020-04-21T16:23:02","subject":"Re: [libcamera-devel] [PATCH v4 11/11] libcamera: pipeline: simple:\n\tIntegrate converter support","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2020-04-04 03:44:38 +0300, Laurent Pinchart wrote:\n> Add support for an optional format converter, supported by the\n> SimpleConverter class. If a converter is available for the pipeline, it\n> will be used to expose additional pixel formats.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n> Changes since v2:\n> \n> - Rebase on top of V4L2PixelFormat\n> - Don't crash if the device has no converter\n> ---\n>  src/libcamera/pipeline/simple/simple.cpp | 196 ++++++++++++++++++++---\n>  1 file changed, 178 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index e4f33f6ff531..8e7dd091b4ab 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -11,6 +11,7 @@\n>  #include <list>\n>  #include <map>\n>  #include <memory>\n> +#include <queue>\n>  #include <set>\n>  #include <string>\n>  #include <string.h>\n> @@ -31,17 +32,24 @@\n>  #include \"v4l2_subdevice.h\"\n>  #include \"v4l2_videodevice.h\"\n>  \n> +#include \"converter.h\"\n> +\n>  namespace libcamera {\n>  \n>  LOG_DEFINE_CATEGORY(SimplePipeline)\n>  \n>  class SimplePipelineHandler;\n>  \n> +struct SimplePipelineInfo {\n> +\tconst char *driver;\n> +\tconst char *converter;\n> +};\n> +\n>  namespace {\n>  \n> -static const char * const drivers[] = {\n> -\t\"imx7-csi\",\n> -\t\"sun6i-csi\",\n> +static const SimplePipelineInfo supportedDevices[] = {\n> +\t{ \"imx7-csi\", \"pxp\" },\n> +\t{ \"sun6i-csi\", nullptr },\n>  };\n>  \n>  } /* namespace */\n> @@ -88,6 +96,8 @@ public:\n>  \n>  \tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n>  \n> +\tbool needConversion() const { return needConversion_; }\n> +\n>  private:\n>  \t/*\n>  \t * The SimpleCameraData instance is guaranteed to be valid as long as\n> @@ -98,6 +108,7 @@ private:\n>  \tconst SimpleCameraData *data_;\n>  \n>  \tV4L2SubdeviceFormat sensorFormat_;\n> +\tbool needConversion_;\n>  };\n>  \n>  class SimplePipelineHandler : public PipelineHandler\n> @@ -120,6 +131,7 @@ public:\n>  \n>  \tV4L2VideoDevice *video() { return video_; }\n>  \tV4L2Subdevice *subdev(const MediaEntity *entity);\n> +\tSimpleConverter *converter() { return converter_; }\n>  \n>  protected:\n>  \tint queueRequestDevice(Camera *camera, Request *request) override;\n> @@ -136,11 +148,17 @@ private:\n>  \tint createCamera(MediaEntity *sensor);\n>  \n>  \tvoid bufferReady(FrameBuffer *buffer);\n> +\tvoid converterDone(FrameBuffer *input, FrameBuffer *output);\n>  \n>  \tMediaDevice *media_;\n>  \tV4L2VideoDevice *video_;\n>  \tstd::map<const MediaEntity *, V4L2Subdevice> subdevs_;\n>  \n> +\tSimpleConverter *converter_;\n> +\tbool useConverter_;\n> +\tstd::vector<std::unique_ptr<FrameBuffer>> converterBuffers_;\n> +\tstd::queue<FrameBuffer *> converterQueue_;\n> +\n>  \tCamera *activeCamera_;\n>  };\n>  \n> @@ -219,6 +237,7 @@ int SimpleCameraData::init()\n>  {\n>  \tSimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(pipe_);\n>  \tV4L2VideoDevice *video = pipe->video();\n> +\tSimpleConverter *converter = pipe->converter();\n>  \tint ret;\n>  \n>  \t/*\n> @@ -275,7 +294,13 @@ int SimpleCameraData::init()\n>  \t\t\tconfig.pixelFormat = pixelFormat;\n>  \t\t\tconfig.size = format.size;\n>  \n> -\t\t\tformats_[pixelFormat] = config;\n> +\t\t\tif (!converter) {\n> +\t\t\t\tformats_[pixelFormat] = config;\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\n> +\t\t\tfor (PixelFormat format : converter->formats(pixelFormat))\n> +\t\t\t\tformats_[format] = config;\n>  \t\t}\n>  \t}\n>  \n> @@ -414,6 +439,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>  \t\tstatus = Adjusted;\n>  \t}\n>  \n> +\tneedConversion_ = cfg.pixelFormat != pipeConfig.pixelFormat;\n> +\n>  \tcfg.bufferCount = 3;\n>  \n>  \treturn status;\n> @@ -424,13 +451,14 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>   */\n>  \n>  SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)\n> -\t: PipelineHandler(manager), video_(nullptr)\n> +\t: PipelineHandler(manager), video_(nullptr), converter_(nullptr)\n>  {\n>  }\n>  \n>  SimplePipelineHandler::~SimplePipelineHandler()\n>  {\n>  \tdelete video_;\n> +\tdelete converter_;\n>  }\n>  \n>  CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera,\n> @@ -496,22 +524,37 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n>  \t\treturn ret;\n>  \n>  \t/* Configure the video node. */\n> -\tV4L2PixelFormat videoFormat = video_->toV4L2PixelFormat(cfg.pixelFormat);\n> +\tV4L2PixelFormat videoFormat = video_->toV4L2PixelFormat(pipeConfig.pixelFormat);\n>  \n> -\tV4L2DeviceFormat outputFormat = {};\n> -\toutputFormat.fourcc = videoFormat;\n> -\toutputFormat.size = cfg.size;\n> +\tV4L2DeviceFormat captureFormat = {};\n> +\tcaptureFormat.fourcc = videoFormat;\n> +\tcaptureFormat.size = cfg.size;\n>  \n> -\tret = video_->setFormat(&outputFormat);\n> +\tret = video_->setFormat(&captureFormat);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tif (outputFormat.size != cfg.size || outputFormat.fourcc != videoFormat) {\n> +\tif (captureFormat.fourcc != videoFormat || captureFormat.size != cfg.size) {\n>  \t\tLOG(SimplePipeline, Error)\n>  \t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n>  \t\treturn -EINVAL;\n>  \t}\n>  \n> +\t/* Configure the converter if required. */\n> +\tuseConverter_ = config->needConversion();\n> +\n> +\tif (useConverter_) {\n> +\t\tint ret = converter_->configure(pipeConfig.pixelFormat,\n> +\t\t\t\t\t\tcfg.pixelFormat, cfg.size);\n> +\t\tif (ret < 0) {\n> +\t\t\tLOG(SimplePipeline, Error)\n> +\t\t\t\t<< \"Unable to configure converter\";\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tLOG(SimplePipeline, Debug) << \"Using format converter\";\n> +\t}\n> +\n>  \tcfg.setStream(&data->stream_);\n>  \n>  \treturn 0;\n> @@ -522,24 +565,47 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n>  {\n>  \tunsigned int count = stream->configuration().bufferCount;\n>  \n> -\treturn video_->exportBuffers(count, buffers);\n> +\t/*\n> +\t * Export buffers on the converter or capture video node, depending on\n> +\t * whether the converter is used or not.\n> +\t */\n> +\tif (useConverter_)\n> +\t\treturn converter_->exportBuffers(count, buffers);\n> +\telse\n> +\t\treturn video_->exportBuffers(count, buffers);\n>  }\n>  \n>  int SimplePipelineHandler::start(Camera *camera)\n>  {\n>  \tSimpleCameraData *data = cameraData(camera);\n>  \tunsigned int count = data->stream_.configuration().bufferCount;\n> +\tint ret;\n>  \n> -\tint ret = video_->importBuffers(count);\n> +\tif (useConverter_)\n> +\t\tret = video_->allocateBuffers(count, &converterBuffers_);\n> +\telse\n> +\t\tret = video_->importBuffers(count);\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n>  \tret = video_->streamOn();\n>  \tif (ret < 0) {\n> -\t\tvideo_->releaseBuffers();\n> +\t\tstop(camera);\n>  \t\treturn ret;\n>  \t}\n>  \n> +\tif (useConverter_) {\n> +\t\tret = converter_->start(count);\n> +\t\tif (ret < 0) {\n> +\t\t\tstop(camera);\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\t/* Queue all internal buffers for capture. */\n> +\t\tfor (std::unique_ptr<FrameBuffer> &buffer : converterBuffers_)\n> +\t\t\tvideo_->queueBuffer(buffer.get());\n> +\t}\n> +\n>  \tactiveCamera_ = camera;\n>  \n>  \treturn 0;\n> @@ -547,8 +613,13 @@ int SimplePipelineHandler::start(Camera *camera)\n>  \n>  void SimplePipelineHandler::stop(Camera *camera)\n>  {\n> +\tif (useConverter_)\n> +\t\tconverter_->stop();\n> +\n>  \tvideo_->streamOff();\n>  \tvideo_->releaseBuffers();\n> +\n> +\tconverterBuffers_.clear();\n>  \tactiveCamera_ = nullptr;\n>  }\n>  \n> @@ -564,6 +635,15 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n>  \t\treturn -ENOENT;\n>  \t}\n>  \n> +\t/*\n> +\t * If conversion is needed, push the buffer to the converter queue, it\n> +\t * will be handed to the converter in the capture completion handler.\n> +\t */\n> +\tif (useConverter_) {\n> +\t\tconverterQueue_.push(buffer);\n> +\t\treturn 0;\n> +\t}\n> +\n>  \treturn video_->queueBuffer(buffer);\n>  }\n>  \n> @@ -573,11 +653,20 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n>  \n>  bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n>  {\n> -\tfor (const char *driver : drivers) {\n> -\t\tDeviceMatch dm(driver);\n> +\tMediaDevice *converter = nullptr;\n> +\n> +\tfor (const SimplePipelineInfo &info : supportedDevices) {\n> +\t\tDeviceMatch dm(info.driver);\n>  \t\tmedia_ = acquireMediaDevice(enumerator, dm);\n> -\t\tif (media_)\n> +\t\tif (!media_)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (!info.converter)\n>  \t\t\tbreak;\n> +\n> +\t\tDeviceMatch converterMatch(info.converter);\n> +\t\tconverter = acquireMediaDevice(enumerator, converterMatch);\n> +\t\tbreak;\n>  \t}\n>  \n>  \tif (!media_)\n> @@ -632,6 +721,19 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n>  \n>  \tvideo_->bufferReady.connect(this, &SimplePipelineHandler::bufferReady);\n>  \n> +\t/* Open the converter, if any. */\n> +\tif (converter) {\n> +\t\tconverter_ = new SimpleConverter(converter);\n> +\t\tif (converter_->open() < 0) {\n> +\t\t\tLOG(SimplePipeline, Warning)\n> +\t\t\t\t<< \"Failed to open converter, disabling format conversion\";\n> +\t\t\tdelete converter_;\n> +\t\t\tconverter_ = nullptr;\n> +\t\t}\n> +\n> +\t\tconverter_->bufferReady.connect(this, &SimplePipelineHandler::converterDone);\n> +\t}\n> +\n>  \t/*\n>  \t * Create one camera data instance for each sensor and gather all\n>  \t * entities in all pipelines.\n> @@ -706,12 +808,70 @@ V4L2Subdevice *SimplePipelineHandler::subdev(const MediaEntity *entity)\n>  \n>  void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n>  {\n> -\tASSERT(activeCamera_);\n> +\t/*\n> +\t * If an error occurred during capture, or if the buffer was cancelled,\n> +\t * complete the request, even if the converter is in use as there's no\n> +\t * point converting an erroneous buffer.\n> +\t */\n> +\tif (buffer->metadata().status != FrameMetadata::FrameSuccess) {\n> +\t\tif (useConverter_) {\n> +\t\t\t/* Requeue the buffer for capture. */\n> +\t\t\tvideo_->queueBuffer(buffer);\n> +\n> +\t\t\t/*\n> +\t\t\t * Get the next user-facing buffer to complete the\n> +\t\t\t * request.\n> +\t\t\t */\n> +\t\t\tif (converterQueue_.empty())\n> +\t\t\t\treturn;\n> +\n> +\t\t\tbuffer = converterQueue_.front();\n> +\t\t\tconverterQueue_.pop();\n> +\t\t}\n> +\n> +\t\tRequest *request = buffer->request();\n> +\t\tcompleteBuffer(activeCamera_, request, buffer);\n> +\t\tcompleteRequest(activeCamera_, request);\n> +\t\treturn;\n> +\t}\n> +\n> +\t/*\n> +\t * Queue the captured and the request buffer to the converter if format\n> +\t * conversion is needed. If there's no queued request, just requeue the\n> +\t * captured buffer for capture.\n> +\t */\n> +\tif (useConverter_) {\n> +\t\tif (converterQueue_.empty()) {\n> +\t\t\tvideo_->queueBuffer(buffer);\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tFrameBuffer *output = converterQueue_.front();\n> +\t\tconverterQueue_.pop();\n> +\n> +\t\tconverter_->queueBuffers(buffer, output);\n> +\t\treturn;\n> +\t}\n> +\n> +\t/* Otherwise simply complete the request. */\n>  \tRequest *request = buffer->request();\n>  \tcompleteBuffer(activeCamera_, request, buffer);\n>  \tcompleteRequest(activeCamera_, request);\n>  }\n>  \n> +void SimplePipelineHandler::converterDone(FrameBuffer *input,\n> +\t\t\t\t\t  FrameBuffer *output)\n> +{\n> +\t/* Complete the request. */\n> +\tASSERT(activeCamera_);\n> +\tRequest *request = output->request();\n> +\tcompleteBuffer(activeCamera_, request, output);\n> +\tcompleteRequest(activeCamera_, request);\n> +\n> +\t/* Queue the input buffer back for capture. */\n> +\tvideo_->queueBuffer(input);\n> +}\n> +\n>  REGISTER_PIPELINE_HANDLER(SimplePipelineHandler);\n>  \n>  } /* namespace libcamera */\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B4AB60406\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 Apr 2020 18:23:05 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id w20so8341311ljj.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 Apr 2020 09:23:05 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\to22sm2323675ljj.100.2020.04.21.09.23.03\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 21 Apr 2020 09:23:03 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"dRo2XZaU\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=bMQSSJCEe8cnwFMNtEEi/anHlDYiuYnbugOtXyYWczU=;\n\tb=dRo2XZaUUoUg5K6Qdy3xn0o8MJvBxNVqcbS3eqwuyOlqR6y64ObhyNBPH+/vl9v7pu\n\tErFgEAbeexb2yl4/xpxUtwbANZTo/9yLv6jmdhNWJRObQn/mhvpgL4TsKDMa5NvyRuxH\n\t7Mli5rcG8Z5moxjQyH3cQrD7ibySftSM8dH5ykNpAV9eYVbaSAtYx78nXYKUfJlsdyIh\n\tfyzCcvDENLtOr+yLJpNiqIZn9cM52hF0nH/PGDwNqaPQcfSqqKV6pzGY/otAq0gDvrHr\n\tQ91nslmybeadNacA9c8J83yHV3ctfygCab6pBU7AzLEcK+eyUrWq0ThOjGk1ZO4InrsK\n\tvjSg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=bMQSSJCEe8cnwFMNtEEi/anHlDYiuYnbugOtXyYWczU=;\n\tb=H4yTd7qvPSRDpqC5/lvLUhJD0DQVk6ErwilDYwxz/DRtbhPT6SSH/RxOGnBEuPGzFN\n\tfrHUtvk7kFscbKVYm7GX4UEfd3P9x2m68Md9bGitQ7V1iXOlUH90Ed9sLIKQl2iy9xSp\n\tLnRS6pbrzKmeANleCW2UqGChqNhZkJLGZQz7/tCNg1kMCeQEm4LRKL2OYl1DUP7M/Bm9\n\tGcTyGxqI3aWzO0FE2nzgtaxxXmr86GjUQSwZQ2CrycphypWNS0BG2b9OQWRYOZzEcYvb\n\tBKN0PqFDWV9FYCX0M+x8wNNXsm6BsOb/qjrXxBjzS43BXXoetBsGOcoQr8jNKbuUrHIA\n\ta9DQ==","X-Gm-Message-State":"AGi0PuYqRkbnBVEPs8DwxRuugaSQK52YLqS3vdvu3PfrcYq8iRblQ/Im\n\tsxX/wShg1hBU6Z5V5ZjztHcjGN4xUeU=","X-Google-Smtp-Source":"APiQypJvoC3MmoNCFyrr8VaVLsehttTPVCggSJDaOXCQv9ZSvCOfs1gAUl3PvXzgWzoMOXydGk46+Q==","X-Received":"by 2002:a2e:9a93:: with SMTP id\n\tp19mr11946135lji.77.1587486184430; \n\tTue, 21 Apr 2020 09:23:04 -0700 (PDT)","Date":"Tue, 21 Apr 2020 18:23:02 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, Martijn Braam <martijn@brixit.nl>, \n\tBenjamin GAIGNARD <benjamin.gaignard@st.com>","Message-ID":"<20200421162302.GF2600980@oden.dyn.berto.se>","References":"<20200404004438.17992-1-laurent.pinchart@ideasonboard.com>\n\t<20200404004438.17992-12-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200404004438.17992-12-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 11/11] libcamera: pipeline: simple:\n\tIntegrate converter support","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":"Tue, 21 Apr 2020 16:23:05 -0000"}}]