[{"id":11392,"web_url":"https://patchwork.libcamera.org/comment/11392/","msgid":"<20200714220652.GH5854@pendragon.ideasonboard.com>","date":"2020-07-14T22:06:52","subject":"Re: [libcamera-devel] [PATCH 14/20] libcamera: ipu3: Adjust and\n\tassign streams in validate()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Jul 14, 2020 at 12:42:06PM +0200, Jacopo Mondi wrote:\n> Remove the adjustStream() and assignStream() methods, and perform stream\n> adjustment and assignment while iterating the StreamConfiguration\n> items.\n> \n> The adjustStream() implementation had some arbitrary assumption, like\n> the main output having to be as large as the sensor resolution, and did\n> not take into account the different alignment requirements between the\n> main output and the viewfinder output.\n> \n> The assignStream() implementation also assumes only full-size streams\n> can be produced by the main output, and having it as a separate function\n> prevents adjusting streams according to which output they are assigned.\n> \n> Blend the two implementation in a single loop and perform the required\n> stream adjustment and assignment in one go.\n> \n> As streams are now assigned at validate() time, remove the same\n> operation from the configure() function.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 247 ++++++++++++---------------\n>  1 file changed, 108 insertions(+), 139 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 19e07fd57e39..1161987a4322 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -70,9 +70,6 @@ public:\n>  \tconst std::vector<const Stream *> &streams() { return streams_; }\n>  \n>  private:\n> -\tvoid assignStreams();\n> -\tvoid adjustStream(StreamConfiguration &cfg, bool scale);\n> -\n>  \t/*\n>  \t * The IPU3CameraData instance is guaranteed to be valid as long as the\n>  \t * corresponding Camera instance is valid. In order to borrow a\n> @@ -137,96 +134,6 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n>  \tdata_ = data;\n>  }\n>  \n> -void IPU3CameraConfiguration::assignStreams()\n> -{\n> -\t/*\n> -\t * Verify and update all configuration entries, and assign a stream to\n> -\t * each of them. The viewfinder stream can scale, while the output\n> -\t * stream can crop only, so select the output stream when the requested\n> -\t * resolution is equal to the sensor resolution, and the viewfinder\n> -\t * stream otherwise.\n> -\t */\n> -\tstd::set<const Stream *> availableStreams = {\n> -\t\t&data_->outStream_,\n> -\t\t&data_->vfStream_,\n> -\t\t&data_->rawStream_,\n> -\t};\n> -\n> -\t/*\n> -\t * The caller is responsible to limit the number of requested streams\n> -\t * to a number supported by the pipeline before calling this function.\n> -\t */\n> -\tASSERT(availableStreams.size() >= config_.size());\n> -\n> -\tstreams_.clear();\n> -\tstreams_.reserve(config_.size());\n> -\n> -\tfor (const StreamConfiguration &cfg : config_) {\n> -\t\tconst PixelFormatInfo &info =\n> -\t\t\tPixelFormatInfo::info(cfg.pixelFormat);\n> -\t\tconst Stream *stream;\n> -\n> -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> -\t\t\tstream = &data_->rawStream_;\n> -\t\telse if (cfg.size == cio2Configuration_.size)\n> -\t\t\tstream = &data_->outStream_;\n> -\t\telse\n> -\t\t\tstream = &data_->vfStream_;\n> -\n> -\t\tif (availableStreams.find(stream) == availableStreams.end())\n> -\t\t\tstream = *availableStreams.begin();\n> -\n> -\t\tstreams_.push_back(stream);\n> -\t\tavailableStreams.erase(stream);\n> -\t}\n> -}\n> -\n> -void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> -{\n> -\t/* The only pixel format the driver supports is NV12. */\n> -\tcfg.pixelFormat = formats::NV12;\n> -\n> -\tif (scale) {\n> -\t\t/*\n> -\t\t * Provide a suitable default that matches the sensor aspect\n> -\t\t * ratio.\n> -\t\t */\n> -\t\tif (cfg.size.isNull()) {\n> -\t\t\tcfg.size.width = 1280;\n> -\t\t\tcfg.size.height = 1280 * cio2Configuration_.size.height\n> -\t\t\t\t\t/ cio2Configuration_.size.width;\n> -\t\t}\n> -\n> -\t\t/*\n> -\t\t * \\todo: Clamp the size to the hardware bounds when we will\n> -\t\t * figure them out.\n> -\t\t *\n> -\t\t * \\todo: Handle the scaler (BDS) restrictions. The BDS can\n> -\t\t * only scale with the same factor in both directions, and the\n> -\t\t * scaling factor is limited to a multiple of 1/32. At the\n> -\t\t * moment the ImgU driver hides these constraints by applying\n> -\t\t * additional cropping, this should be fixed on the driver\n> -\t\t * side, and cropping should be exposed to us.\n> -\t\t */\n> -\t} else {\n> -\t\t/*\n> -\t\t * \\todo: Properly support cropping when the ImgU driver\n> -\t\t * interface will be cleaned up.\n> -\t\t */\n> -\t\tcfg.size = cio2Configuration_.size;\n> -\t}\n> -\n> -\t/*\n> -\t * Clamp the size to match the ImgU alignment constraints. The width\n> -\t * shall be a multiple of 8 pixels and the height a multiple of 4\n> -\t * pixels.\n> -\t */\n> -\tif (cfg.size.width % 8 || cfg.size.height % 4) {\n> -\t\tcfg.size.width &= ~7;\n> -\t\tcfg.size.height &= ~3;\n> -\t}\n> -}\n> -\n>  CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  {\n>  \tStatus status = Valid;\n> @@ -242,71 +149,141 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  \n>  \t/*\n>  \t * Validate the requested stream configuration and select the sensor\n> -\t * format by collecting the maximum width and height and picking the\n> -\t * closest larger match, as the IPU3 can downscale only. If no\n> -\t * resolution is requested for any stream, or if no sensor resolution is\n> -\t * large enough, pick the largest one.\n> +\t * format by collecting the maximum RAW stream width and height and\n> +\t * picking the closest larger match, as the IPU3 can downscale only. If\n> +\t * no resolution is requested for the RAW stream, use the one from the\n> +\t * largest YUV stream, plus margins pixels for the IF and BDS to scale.\n> +\t * If no resolution is requested for any stream, pick the largest one.\n>  \t */\n>  \tunsigned int rawCount = 0;\n>  \tunsigned int yuvCount = 0;\n> -\tSize size;\n> +\tSize maxYuvSize;\n> +\tSize maxRawSize;\n>  \n>  \tfor (const StreamConfiguration &cfg : config_) {\n>  \t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n>  \n> -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n>  \t\t\trawCount++;\n> -\t\telse\n> +\t\t\tmaxRawSize = maxRawSize.expandedTo(cfg.size);\n\nNot something that is needed as a prerequisite for this series, but do\nyou think it would be useful to also have in-place versions of the Size\n(and Rectangle) helpers ? This line would become\n\n\t\t\tmaxRawSize.expandTo(cfg.size);\n\n> +\t\t} else {\n>  \t\t\tyuvCount++;\n> -\n> -\t\tif (cfg.size.width > size.width)\n> -\t\t\tsize.width = cfg.size.width;\n> -\t\tif (cfg.size.height > size.height)\n> -\t\t\tsize.height = cfg.size.height;\n> +\t\t\tmaxYuvSize = maxYuvSize.expandedTo(cfg.size);\n> +\t\t}\n>  \t}\n>  \tif (rawCount > 1 || yuvCount > 2) {\n>  \t\tLOG(IPU3, Debug)\n>  \t\t\t<< \"Camera configuration not supported\";\n>  \t\treturn Invalid;\n>  \t}\n\nMaybe a blank line here (and above this block too) ?\n\n> +\tif (maxRawSize.isNull()) {\n> +\t\tmaxRawSize.width = utils::alignUp(maxYuvSize.width,\n> +\t\t\t\t\t\t  IMGU_OUTPUT_WIDTH_MARGIN);\n> +\t\tmaxRawSize.height = utils::alignUp(maxYuvSize.height,\n> +\t\t\t\t\t\t   IMGU_OUTPUT_HEIGHT_MARGIN);\n\nMaybe\n\n\t\tmaxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n\t\t\t\t\t\t    IMGU_OUTPUT_HEIGHT_MARGIN);\n\n?\n\n> +\t\tmaxRawSize = maxRawSize.boundedTo(data_->cio2_.sensor()->resolution());\n\nOr even\n\n\t\tmaxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n\t\t\t\t\t\t    IMGU_OUTPUT_HEIGHT_MARGIN)\n\t\t\t\t       .boundedTo(data_->cio2_.sensor()->resolution());\n\n> +\t}\n>  \n> -\t/* Generate raw configuration from CIO2. */\n> -\tcio2Configuration_ = data_->cio2_.generateConfiguration(size);\n> +\t/*\n> +\t * Generate raw configuration from CIO2.\n> +\t *\n> +\t * The output YUV streams will be limited in size to the maximum\n> +\t * frame size requested for the RAW stream.\n> +\t */\n> +\tcio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);\n>  \tif (!cio2Configuration_.pixelFormat.isValid())\n>  \t\treturn Invalid;\n>  \n> -\t/* Assign streams to each configuration entry. */\n> -\tassignStreams();\n> +\tLOG(IPU3, Debug) << \"CIO2 configuration: \" << cio2Configuration_.toString();\n>  \n> -\t/* Verify and adjust configuration if needed. */\n> +\t/*\n> +\t * Adjust the configurations if needed and assign streams while\n> +\t * iterating them.\n> +\t */\n> +\tbool mainOutputAvailable = true;\n>  \tfor (unsigned int i = 0; i < config_.size(); ++i) {\n> -\t\tStreamConfiguration &cfg = config_[i];\n> -\t\tconst StreamConfiguration oldCfg = cfg;\n> -\t\tconst Stream *stream = streams_[i];\n> -\n> -\t\tif (stream == &data_->rawStream_) {\n> -\t\t\tcfg.size = cio2Configuration_.size;\n> -\t\t\tcfg.pixelFormat = cio2Configuration_.pixelFormat;\n> -\t\t\tcfg.bufferCount = cio2Configuration_.bufferCount;\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(config_[i].pixelFormat);\n> +\t\tconst StreamConfiguration originalCfg = config_[i];\n> +\t\tStreamConfiguration *cfg = &config_[i];\n\nI'm not asking for this to be changed, but out of curiotiry, why are you\nturning the reference into a pointer ?\n\n> +\n> +\t\tLOG(IPU3, Debug) << \"Validating configuration: \" << config_[i].toString();\n\nWould it make sense to s/configuration/stream/ ?\n\n> +\n> +\n\nExtra blank line ?\n\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> +\t\t\t/* Initialize the RAW stream with the CIO2 configuration. */\n> +\t\t\tcfg->size = cio2Configuration_.size;\n> +\t\t\tcfg->pixelFormat = cio2Configuration_.pixelFormat;\n> +\t\t\tcfg->bufferCount = cio2Configuration_.bufferCount;\n> +\t\t\tcfg->stride = info.stride(cfg->size.width, 0, 64);\n> +\t\t\tcfg->frameSize = info.frameSize(cfg->size, 64);\n> +\t\t\tcfg->setStream(const_cast<Stream *>(&data_->rawStream_));\n> +\n> +\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> +\t\t\t\t\t << \" to the raw stream\";\n>  \t\t} else {\n> -\t\t\tbool scale = stream == &data_->vfStream_;\n> -\t\t\tadjustStream(config_[i], scale);\n> -\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> +\t\t\t/* Assign and configure the main and viewfinder outputs. */\n> +\n> +\t\t\t/*\n> +\t\t\t * Clamp the size to match the ImgU size limits and the\n> +\t\t\t * margins from the CIO2 output frame size.\n> +\t\t\t *\n> +\t\t\t * The ImgU outputs needs to be rounded down to 64\n> +\t\t\t * pixels in width and 32 pixels in height from the\n> +\t\t\t * input frame size.\n> +\t\t\t *\n> +\t\t\t * \\todo Verify this assumption and find out if it\n> +\t\t\t * depends on the BDS scaling factor of 1/32, as the\n> +\t\t\t * main output has no YUV scaler as the viewfinder\n> +\t\t\t * output has.\n> +\t\t\t */\n> +\t\t\tunsigned int limit;\n> +\t\t\tlimit = utils::alignDown(cio2Configuration_.size.width - 1,\n> +\t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n\nWhere does the - 1 come from ?\n\n> +\t\t\tcfg->size.width = utils::clamp(cfg->size.width,\n> +\t\t\t\t\t\t       IMGU_OUTPUT_MIN_SIZE.width,\n> +\t\t\t\t\t\t       limit);\n> +\n> +\t\t\tlimit = utils::alignDown(cio2Configuration_.size.height - 1,\n> +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> +\t\t\tcfg->size.height = utils::clamp(cfg->size.height,\n> +\t\t\t\t\t\t\tIMGU_OUTPUT_MIN_SIZE.height,\n> +\t\t\t\t\t\t\tlimit);\n> +\n> +\t\t\tcfg->size = cfg->size.alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> +\t\t\t\t\t\t\t    IMGU_OUTPUT_HEIGHT_ALIGN);\n> +\n> +\t\t\tcfg->pixelFormat = formats::NV12;\n> +\t\t\tcfg->bufferCount = IPU3_BUFFER_COUNT;\n> +\t\t\tcfg->stride = info.stride(cfg->size.width, 0, 1);\n> +\t\t\tcfg->frameSize = info.frameSize(cfg->size, 1);\n> +\n> +\t\t\t/*\n> +\t\t\t * Use the main output stream in case only one stream is\n> +\t\t\t * requested or if the current configuration is the one with\n> +\t\t\t * the maximum YUV output size.\n> +\t\t\t */\n> +\t\t\tif (mainOutputAvailable &&\n> +\t\t\t    (originalCfg.size == maxYuvSize || yuvCount == 1)) {\n> +\t\t\t\tcfg->setStream(const_cast<Stream *>(&data_->outStream_));\n> +\t\t\t\tmainOutputAvailable = false;\n> +\n> +\t\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> +\t\t\t\t\t\t << \" to the main output\";\n> +\t\t\t} else {\n> +\t\t\t\tcfg->setStream(const_cast<Stream *>(&data_->vfStream_));\n> +\n> +\t\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> +\t\t\t\t\t\t << \" to the viewfinder output\";\n> +\t\t\t}\n>  \t\t}\n>  \n> -\t\tif (cfg.pixelFormat != oldCfg.pixelFormat ||\n> -\t\t    cfg.size != oldCfg.size) {\n> +\t\tif (cfg->pixelFormat != originalCfg.pixelFormat ||\n> +\t\t    cfg->size != originalCfg.size) {\n>  \t\t\tLOG(IPU3, Debug)\n>  \t\t\t\t<< \"Stream \" << i << \" configuration adjusted to \"\n> -\t\t\t\t<< cfg.toString();\n> +\t\t\t\t<< cfg->toString();\n>  \t\t\tstatus = Adjusted;\n>  \t\t}\n> -\n> -\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> -\t\tbool packedRaw = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> -\n> -\t\tcfg.stride = info.stride(cfg.size.width, 0, packedRaw ? 64 : 1);\n> -\t\tcfg.frameSize = info.frameSize(cfg.size, packedRaw ? 64 : 1);\n>  \t}\n>  \n>  \treturn status;\n> @@ -475,16 +452,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>  \tbool vfActive = false;\n>  \n>  \tfor (unsigned int i = 0; i < config->size(); ++i) {\n> -\t\t/*\n> -\t\t * Use a const_cast<> here instead of storing a mutable stream\n> -\t\t * pointer in the configuration to let the compiler catch\n> -\t\t * unwanted modifications of camera data in the configuration\n> -\t\t * validate() implementation.\n> -\t\t */\n> -\t\tStream *stream = const_cast<Stream *>(config->streams()[i]);\n>  \t\tStreamConfiguration &cfg = (*config)[i];\n> -\n> -\t\tcfg.setStream(stream);\n> +\t\tStream *stream = cfg.stream();\n>  \n>  \t\tif (stream == outStream) {\n>  \t\t\tret = imgu->configureOutput(cfg, &outputFormat);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 45FD4BD790\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 14 Jul 2020 22:07:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B4E6E60905;\n\tWed, 15 Jul 2020 00:07:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3330A6048C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 15 Jul 2020 00:07:00 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 98D3771D;\n\tWed, 15 Jul 2020 00:06:59 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hI0+7KrQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594764419;\n\tbh=NV5RWtPtw1NQj7YnKCdH0KiuZOFjwdEgakKI/vD3dkY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hI0+7KrQfBuNLF0vZfS+BhX0rLnA0lNfh0QADF9dTZ6y7+zzgdCAwiemQ+aUnwQwK\n\tFwhQNWaJk0C6DHIXu65gC0Us0YTFYBFadSBbuLfvFITW5nDfe52jyoo8kE66Llka+8\n\tYN73QS4vuksaNmwB5Ukz5nRIV4A8pjrOvKOYmfxY=","Date":"Wed, 15 Jul 2020 01:06:52 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20200714220652.GH5854@pendragon.ideasonboard.com>","References":"<20200714104212.48683-1-jacopo@jmondi.org>\n\t<20200714104212.48683-15-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200714104212.48683-15-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 14/20] libcamera: ipu3: Adjust and\n\tassign streams in validate()","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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":11403,"web_url":"https://patchwork.libcamera.org/comment/11403/","msgid":"<20200715073551.u6qp7lgipghod3kw@uno.localdomain>","date":"2020-07-15T07:35:51","subject":"Re: [libcamera-devel] [PATCH 14/20] libcamera: ipu3: Adjust and\n\tassign streams in validate()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Jul 15, 2020 at 01:06:52AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Jul 14, 2020 at 12:42:06PM +0200, Jacopo Mondi wrote:\n> > Remove the adjustStream() and assignStream() methods, and perform stream\n> > adjustment and assignment while iterating the StreamConfiguration\n> > items.\n> >\n> > The adjustStream() implementation had some arbitrary assumption, like\n> > the main output having to be as large as the sensor resolution, and did\n> > not take into account the different alignment requirements between the\n> > main output and the viewfinder output.\n> >\n> > The assignStream() implementation also assumes only full-size streams\n> > can be produced by the main output, and having it as a separate function\n> > prevents adjusting streams according to which output they are assigned.\n> >\n> > Blend the two implementation in a single loop and perform the required\n> > stream adjustment and assignment in one go.\n> >\n> > As streams are now assigned at validate() time, remove the same\n> > operation from the configure() function.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 247 ++++++++++++---------------\n> >  1 file changed, 108 insertions(+), 139 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 19e07fd57e39..1161987a4322 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -70,9 +70,6 @@ public:\n> >  \tconst std::vector<const Stream *> &streams() { return streams_; }\n> >\n> >  private:\n> > -\tvoid assignStreams();\n> > -\tvoid adjustStream(StreamConfiguration &cfg, bool scale);\n> > -\n> >  \t/*\n> >  \t * The IPU3CameraData instance is guaranteed to be valid as long as the\n> >  \t * corresponding Camera instance is valid. In order to borrow a\n> > @@ -137,96 +134,6 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n> >  \tdata_ = data;\n> >  }\n> >\n> > -void IPU3CameraConfiguration::assignStreams()\n> > -{\n> > -\t/*\n> > -\t * Verify and update all configuration entries, and assign a stream to\n> > -\t * each of them. The viewfinder stream can scale, while the output\n> > -\t * stream can crop only, so select the output stream when the requested\n> > -\t * resolution is equal to the sensor resolution, and the viewfinder\n> > -\t * stream otherwise.\n> > -\t */\n> > -\tstd::set<const Stream *> availableStreams = {\n> > -\t\t&data_->outStream_,\n> > -\t\t&data_->vfStream_,\n> > -\t\t&data_->rawStream_,\n> > -\t};\n> > -\n> > -\t/*\n> > -\t * The caller is responsible to limit the number of requested streams\n> > -\t * to a number supported by the pipeline before calling this function.\n> > -\t */\n> > -\tASSERT(availableStreams.size() >= config_.size());\n> > -\n> > -\tstreams_.clear();\n> > -\tstreams_.reserve(config_.size());\n> > -\n> > -\tfor (const StreamConfiguration &cfg : config_) {\n> > -\t\tconst PixelFormatInfo &info =\n> > -\t\t\tPixelFormatInfo::info(cfg.pixelFormat);\n> > -\t\tconst Stream *stream;\n> > -\n> > -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > -\t\t\tstream = &data_->rawStream_;\n> > -\t\telse if (cfg.size == cio2Configuration_.size)\n> > -\t\t\tstream = &data_->outStream_;\n> > -\t\telse\n> > -\t\t\tstream = &data_->vfStream_;\n> > -\n> > -\t\tif (availableStreams.find(stream) == availableStreams.end())\n> > -\t\t\tstream = *availableStreams.begin();\n> > -\n> > -\t\tstreams_.push_back(stream);\n> > -\t\tavailableStreams.erase(stream);\n> > -\t}\n> > -}\n> > -\n> > -void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> > -{\n> > -\t/* The only pixel format the driver supports is NV12. */\n> > -\tcfg.pixelFormat = formats::NV12;\n> > -\n> > -\tif (scale) {\n> > -\t\t/*\n> > -\t\t * Provide a suitable default that matches the sensor aspect\n> > -\t\t * ratio.\n> > -\t\t */\n> > -\t\tif (cfg.size.isNull()) {\n> > -\t\t\tcfg.size.width = 1280;\n> > -\t\t\tcfg.size.height = 1280 * cio2Configuration_.size.height\n> > -\t\t\t\t\t/ cio2Configuration_.size.width;\n> > -\t\t}\n> > -\n> > -\t\t/*\n> > -\t\t * \\todo: Clamp the size to the hardware bounds when we will\n> > -\t\t * figure them out.\n> > -\t\t *\n> > -\t\t * \\todo: Handle the scaler (BDS) restrictions. The BDS can\n> > -\t\t * only scale with the same factor in both directions, and the\n> > -\t\t * scaling factor is limited to a multiple of 1/32. At the\n> > -\t\t * moment the ImgU driver hides these constraints by applying\n> > -\t\t * additional cropping, this should be fixed on the driver\n> > -\t\t * side, and cropping should be exposed to us.\n> > -\t\t */\n> > -\t} else {\n> > -\t\t/*\n> > -\t\t * \\todo: Properly support cropping when the ImgU driver\n> > -\t\t * interface will be cleaned up.\n> > -\t\t */\n> > -\t\tcfg.size = cio2Configuration_.size;\n> > -\t}\n> > -\n> > -\t/*\n> > -\t * Clamp the size to match the ImgU alignment constraints. The width\n> > -\t * shall be a multiple of 8 pixels and the height a multiple of 4\n> > -\t * pixels.\n> > -\t */\n> > -\tif (cfg.size.width % 8 || cfg.size.height % 4) {\n> > -\t\tcfg.size.width &= ~7;\n> > -\t\tcfg.size.height &= ~3;\n> > -\t}\n> > -}\n> > -\n> >  CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> >  {\n> >  \tStatus status = Valid;\n> > @@ -242,71 +149,141 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> >\n> >  \t/*\n> >  \t * Validate the requested stream configuration and select the sensor\n> > -\t * format by collecting the maximum width and height and picking the\n> > -\t * closest larger match, as the IPU3 can downscale only. If no\n> > -\t * resolution is requested for any stream, or if no sensor resolution is\n> > -\t * large enough, pick the largest one.\n> > +\t * format by collecting the maximum RAW stream width and height and\n> > +\t * picking the closest larger match, as the IPU3 can downscale only. If\n> > +\t * no resolution is requested for the RAW stream, use the one from the\n> > +\t * largest YUV stream, plus margins pixels for the IF and BDS to scale.\n> > +\t * If no resolution is requested for any stream, pick the largest one.\n> >  \t */\n> >  \tunsigned int rawCount = 0;\n> >  \tunsigned int yuvCount = 0;\n> > -\tSize size;\n> > +\tSize maxYuvSize;\n> > +\tSize maxRawSize;\n> >\n> >  \tfor (const StreamConfiguration &cfg : config_) {\n> >  \t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> >\n> > -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> >  \t\t\trawCount++;\n> > -\t\telse\n> > +\t\t\tmaxRawSize = maxRawSize.expandedTo(cfg.size);\n>\n> Not something that is needed as a prerequisite for this series, but do\n> you think it would be useful to also have in-place versions of the Size\n> (and Rectangle) helpers ? This line would become\n>\n> \t\t\tmaxRawSize.expandTo(cfg.size);\n>\n\nHaving chased for like 15 minutes why my Size didn't get update when I\nused this, I would really say yes.\n\n> > +\t\t} else {\n> >  \t\t\tyuvCount++;\n> > -\n> > -\t\tif (cfg.size.width > size.width)\n> > -\t\t\tsize.width = cfg.size.width;\n> > -\t\tif (cfg.size.height > size.height)\n> > -\t\t\tsize.height = cfg.size.height;\n> > +\t\t\tmaxYuvSize = maxYuvSize.expandedTo(cfg.size);\n> > +\t\t}\n> >  \t}\n> >  \tif (rawCount > 1 || yuvCount > 2) {\n> >  \t\tLOG(IPU3, Debug)\n> >  \t\t\t<< \"Camera configuration not supported\";\n> >  \t\treturn Invalid;\n> >  \t}\n>\n> Maybe a blank line here (and above this block too) ?\n>\n> > +\tif (maxRawSize.isNull()) {\n> > +\t\tmaxRawSize.width = utils::alignUp(maxYuvSize.width,\n> > +\t\t\t\t\t\t  IMGU_OUTPUT_WIDTH_MARGIN);\n> > +\t\tmaxRawSize.height = utils::alignUp(maxYuvSize.height,\n> > +\t\t\t\t\t\t   IMGU_OUTPUT_HEIGHT_MARGIN);\n>\n> Maybe\n>\n> \t\tmaxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> \t\t\t\t\t\t    IMGU_OUTPUT_HEIGHT_MARGIN);\n>\n> ?\n>\n> > +\t\tmaxRawSize = maxRawSize.boundedTo(data_->cio2_.sensor()->resolution());\n>\n> Or even\n>\n> \t\tmaxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> \t\t\t\t\t\t    IMGU_OUTPUT_HEIGHT_MARGIN)\n> \t\t\t\t       .boundedTo(data_->cio2_.sensor()->resolution());\n>\n\nYes, sorry, I got here through several attempts where I mangled sizes\nindividually and didn't notice I could have used the new Size methods\n\n> > +\t}\n> >\n> > -\t/* Generate raw configuration from CIO2. */\n> > -\tcio2Configuration_ = data_->cio2_.generateConfiguration(size);\n> > +\t/*\n> > +\t * Generate raw configuration from CIO2.\n> > +\t *\n> > +\t * The output YUV streams will be limited in size to the maximum\n> > +\t * frame size requested for the RAW stream.\n> > +\t */\n> > +\tcio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);\n> >  \tif (!cio2Configuration_.pixelFormat.isValid())\n> >  \t\treturn Invalid;\n> >\n> > -\t/* Assign streams to each configuration entry. */\n> > -\tassignStreams();\n> > +\tLOG(IPU3, Debug) << \"CIO2 configuration: \" << cio2Configuration_.toString();\n> >\n> > -\t/* Verify and adjust configuration if needed. */\n> > +\t/*\n> > +\t * Adjust the configurations if needed and assign streams while\n> > +\t * iterating them.\n> > +\t */\n> > +\tbool mainOutputAvailable = true;\n> >  \tfor (unsigned int i = 0; i < config_.size(); ++i) {\n> > -\t\tStreamConfiguration &cfg = config_[i];\n> > -\t\tconst StreamConfiguration oldCfg = cfg;\n> > -\t\tconst Stream *stream = streams_[i];\n> > -\n> > -\t\tif (stream == &data_->rawStream_) {\n> > -\t\t\tcfg.size = cio2Configuration_.size;\n> > -\t\t\tcfg.pixelFormat = cio2Configuration_.pixelFormat;\n> > -\t\t\tcfg.bufferCount = cio2Configuration_.bufferCount;\n> > +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(config_[i].pixelFormat);\n> > +\t\tconst StreamConfiguration originalCfg = config_[i];\n> > +\t\tStreamConfiguration *cfg = &config_[i];\n>\n> I'm not asking for this to be changed, but out of curiotiry, why are you\n> turning the reference into a pointer ?\n>\n\nAs we actually modify it's content. This is not a function parameter,\nwhere we enforce that rule, but I still find more natural to use\npointers for things that have to be modified, and references for\nthings that stay constant.\n\n> > +\n> > +\t\tLOG(IPU3, Debug) << \"Validating configuration: \" << config_[i].toString();\n>\n> Would it make sense to s/configuration/stream/ ?\n>\n\nYou probably suggested this already and I missed it.\n\n> > +\n> > +\n>\n> Extra blank line ?\n>\n\nHave I missed a checkstyle warning ?\n\n> > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > +\t\t\t/* Initialize the RAW stream with the CIO2 configuration. */\n> > +\t\t\tcfg->size = cio2Configuration_.size;\n> > +\t\t\tcfg->pixelFormat = cio2Configuration_.pixelFormat;\n> > +\t\t\tcfg->bufferCount = cio2Configuration_.bufferCount;\n> > +\t\t\tcfg->stride = info.stride(cfg->size.width, 0, 64);\n> > +\t\t\tcfg->frameSize = info.frameSize(cfg->size, 64);\n> > +\t\t\tcfg->setStream(const_cast<Stream *>(&data_->rawStream_));\n> > +\n> > +\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> > +\t\t\t\t\t << \" to the raw stream\";\n> >  \t\t} else {\n> > -\t\t\tbool scale = stream == &data_->vfStream_;\n> > -\t\t\tadjustStream(config_[i], scale);\n> > -\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> > +\t\t\t/* Assign and configure the main and viewfinder outputs. */\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Clamp the size to match the ImgU size limits and the\n> > +\t\t\t * margins from the CIO2 output frame size.\n> > +\t\t\t *\n> > +\t\t\t * The ImgU outputs needs to be rounded down to 64\n> > +\t\t\t * pixels in width and 32 pixels in height from the\n> > +\t\t\t * input frame size.\n> > +\t\t\t *\n> > +\t\t\t * \\todo Verify this assumption and find out if it\n> > +\t\t\t * depends on the BDS scaling factor of 1/32, as the\n> > +\t\t\t * main output has no YUV scaler as the viewfinder\n> > +\t\t\t * output has.\n> > +\t\t\t */\n> > +\t\t\tunsigned int limit;\n> > +\t\t\tlimit = utils::alignDown(cio2Configuration_.size.width - 1,\n> > +\t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n>\n> Where does the - 1 come from ?\n>\n\nThe ouput sizes have to be strictly smaller than the input frame size.\nHere I'm looking for the largest multiple of IMGU_OUTPUT_WIDTH_MARGIN\nwhich is strictly smaller than cio2Configuration_.size.width.\n\nThat's for the -1. The reason why I'm searching for a number aligned\nto that margin value comes from inspecting the pipe configuration tool\nresults and the xml files. This seems to be the combination that\nsatisfies the tool in most cases. I know it's weak as a reasoning, but\nI was not able to find out what are the hardware/firmware requirements\nthat lead to this, if not the way parameters are computed by the\nscript (which probably reflect how the hw/fw work though).\n\n> > +\t\t\tcfg->size.width = utils::clamp(cfg->size.width,\n> > +\t\t\t\t\t\t       IMGU_OUTPUT_MIN_SIZE.width,\n> > +\t\t\t\t\t\t       limit);\n> > +\n> > +\t\t\tlimit = utils::alignDown(cio2Configuration_.size.height - 1,\n> > +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> > +\t\t\tcfg->size.height = utils::clamp(cfg->size.height,\n> > +\t\t\t\t\t\t\tIMGU_OUTPUT_MIN_SIZE.height,\n> > +\t\t\t\t\t\t\tlimit);\n> > +\n> > +\t\t\tcfg->size = cfg->size.alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> > +\t\t\t\t\t\t\t    IMGU_OUTPUT_HEIGHT_ALIGN);\n> > +\n> > +\t\t\tcfg->pixelFormat = formats::NV12;\n> > +\t\t\tcfg->bufferCount = IPU3_BUFFER_COUNT;\n> > +\t\t\tcfg->stride = info.stride(cfg->size.width, 0, 1);\n> > +\t\t\tcfg->frameSize = info.frameSize(cfg->size, 1);\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Use the main output stream in case only one stream is\n> > +\t\t\t * requested or if the current configuration is the one with\n> > +\t\t\t * the maximum YUV output size.\n> > +\t\t\t */\n> > +\t\t\tif (mainOutputAvailable &&\n> > +\t\t\t    (originalCfg.size == maxYuvSize || yuvCount == 1)) {\n> > +\t\t\t\tcfg->setStream(const_cast<Stream *>(&data_->outStream_));\n> > +\t\t\t\tmainOutputAvailable = false;\n> > +\n> > +\t\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> > +\t\t\t\t\t\t << \" to the main output\";\n> > +\t\t\t} else {\n> > +\t\t\t\tcfg->setStream(const_cast<Stream *>(&data_->vfStream_));\n> > +\n> > +\t\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> > +\t\t\t\t\t\t << \" to the viewfinder output\";\n> > +\t\t\t}\n> >  \t\t}\n> >\n> > -\t\tif (cfg.pixelFormat != oldCfg.pixelFormat ||\n> > -\t\t    cfg.size != oldCfg.size) {\n> > +\t\tif (cfg->pixelFormat != originalCfg.pixelFormat ||\n> > +\t\t    cfg->size != originalCfg.size) {\n> >  \t\t\tLOG(IPU3, Debug)\n> >  \t\t\t\t<< \"Stream \" << i << \" configuration adjusted to \"\n> > -\t\t\t\t<< cfg.toString();\n> > +\t\t\t\t<< cfg->toString();\n> >  \t\t\tstatus = Adjusted;\n> >  \t\t}\n> > -\n> > -\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> > -\t\tbool packedRaw = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > -\n> > -\t\tcfg.stride = info.stride(cfg.size.width, 0, packedRaw ? 64 : 1);\n> > -\t\tcfg.frameSize = info.frameSize(cfg.size, packedRaw ? 64 : 1);\n> >  \t}\n> >\n> >  \treturn status;\n> > @@ -475,16 +452,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> >  \tbool vfActive = false;\n> >\n> >  \tfor (unsigned int i = 0; i < config->size(); ++i) {\n> > -\t\t/*\n> > -\t\t * Use a const_cast<> here instead of storing a mutable stream\n> > -\t\t * pointer in the configuration to let the compiler catch\n> > -\t\t * unwanted modifications of camera data in the configuration\n> > -\t\t * validate() implementation.\n> > -\t\t */\n> > -\t\tStream *stream = const_cast<Stream *>(config->streams()[i]);\n> >  \t\tStreamConfiguration &cfg = (*config)[i];\n> > -\n> > -\t\tcfg.setStream(stream);\n> > +\t\tStream *stream = cfg.stream();\n> >\n> >  \t\tif (stream == outStream) {\n> >  \t\t\tret = imgu->configureOutput(cfg, &outputFormat);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 66503BD790\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 15 Jul 2020 07:32:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C9B266093A;\n\tWed, 15 Jul 2020 09:32:20 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA97160489\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 15 Jul 2020 09:32:19 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 5A5721C0003;\n\tWed, 15 Jul 2020 07:32:19 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 15 Jul 2020 09:35:51 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20200715073551.u6qp7lgipghod3kw@uno.localdomain>","References":"<20200714104212.48683-1-jacopo@jmondi.org>\n\t<20200714104212.48683-15-jacopo@jmondi.org>\n\t<20200714220652.GH5854@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200714220652.GH5854@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 14/20] libcamera: ipu3: Adjust and\n\tassign streams in validate()","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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":11421,"web_url":"https://patchwork.libcamera.org/comment/11421/","msgid":"<20200716233724.GK5960@pendragon.ideasonboard.com>","date":"2020-07-16T23:37:24","subject":"Re: [libcamera-devel] [PATCH 14/20] libcamera: ipu3: Adjust and\n\tassign streams in validate()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Jul 15, 2020 at 09:35:51AM +0200, Jacopo Mondi wrote:\n> On Wed, Jul 15, 2020 at 01:06:52AM +0300, Laurent Pinchart wrote:\n> > On Tue, Jul 14, 2020 at 12:42:06PM +0200, Jacopo Mondi wrote:\n> > > Remove the adjustStream() and assignStream() methods, and perform stream\n> > > adjustment and assignment while iterating the StreamConfiguration\n> > > items.\n> > >\n> > > The adjustStream() implementation had some arbitrary assumption, like\n> > > the main output having to be as large as the sensor resolution, and did\n> > > not take into account the different alignment requirements between the\n> > > main output and the viewfinder output.\n> > >\n> > > The assignStream() implementation also assumes only full-size streams\n> > > can be produced by the main output, and having it as a separate function\n> > > prevents adjusting streams according to which output they are assigned.\n> > >\n> > > Blend the two implementation in a single loop and perform the required\n> > > stream adjustment and assignment in one go.\n> > >\n> > > As streams are now assigned at validate() time, remove the same\n> > > operation from the configure() function.\n> > >\n> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 247 ++++++++++++---------------\n> > >  1 file changed, 108 insertions(+), 139 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 19e07fd57e39..1161987a4322 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -70,9 +70,6 @@ public:\n> > >  \tconst std::vector<const Stream *> &streams() { return streams_; }\n> > >\n> > >  private:\n> > > -\tvoid assignStreams();\n> > > -\tvoid adjustStream(StreamConfiguration &cfg, bool scale);\n> > > -\n> > >  \t/*\n> > >  \t * The IPU3CameraData instance is guaranteed to be valid as long as the\n> > >  \t * corresponding Camera instance is valid. In order to borrow a\n> > > @@ -137,96 +134,6 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n> > >  \tdata_ = data;\n> > >  }\n> > >\n> > > -void IPU3CameraConfiguration::assignStreams()\n> > > -{\n> > > -\t/*\n> > > -\t * Verify and update all configuration entries, and assign a stream to\n> > > -\t * each of them. The viewfinder stream can scale, while the output\n> > > -\t * stream can crop only, so select the output stream when the requested\n> > > -\t * resolution is equal to the sensor resolution, and the viewfinder\n> > > -\t * stream otherwise.\n> > > -\t */\n> > > -\tstd::set<const Stream *> availableStreams = {\n> > > -\t\t&data_->outStream_,\n> > > -\t\t&data_->vfStream_,\n> > > -\t\t&data_->rawStream_,\n> > > -\t};\n> > > -\n> > > -\t/*\n> > > -\t * The caller is responsible to limit the number of requested streams\n> > > -\t * to a number supported by the pipeline before calling this function.\n> > > -\t */\n> > > -\tASSERT(availableStreams.size() >= config_.size());\n> > > -\n> > > -\tstreams_.clear();\n> > > -\tstreams_.reserve(config_.size());\n> > > -\n> > > -\tfor (const StreamConfiguration &cfg : config_) {\n> > > -\t\tconst PixelFormatInfo &info =\n> > > -\t\t\tPixelFormatInfo::info(cfg.pixelFormat);\n> > > -\t\tconst Stream *stream;\n> > > -\n> > > -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > > -\t\t\tstream = &data_->rawStream_;\n> > > -\t\telse if (cfg.size == cio2Configuration_.size)\n> > > -\t\t\tstream = &data_->outStream_;\n> > > -\t\telse\n> > > -\t\t\tstream = &data_->vfStream_;\n> > > -\n> > > -\t\tif (availableStreams.find(stream) == availableStreams.end())\n> > > -\t\t\tstream = *availableStreams.begin();\n> > > -\n> > > -\t\tstreams_.push_back(stream);\n> > > -\t\tavailableStreams.erase(stream);\n> > > -\t}\n> > > -}\n> > > -\n> > > -void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> > > -{\n> > > -\t/* The only pixel format the driver supports is NV12. */\n> > > -\tcfg.pixelFormat = formats::NV12;\n> > > -\n> > > -\tif (scale) {\n> > > -\t\t/*\n> > > -\t\t * Provide a suitable default that matches the sensor aspect\n> > > -\t\t * ratio.\n> > > -\t\t */\n> > > -\t\tif (cfg.size.isNull()) {\n> > > -\t\t\tcfg.size.width = 1280;\n> > > -\t\t\tcfg.size.height = 1280 * cio2Configuration_.size.height\n> > > -\t\t\t\t\t/ cio2Configuration_.size.width;\n> > > -\t\t}\n> > > -\n> > > -\t\t/*\n> > > -\t\t * \\todo: Clamp the size to the hardware bounds when we will\n> > > -\t\t * figure them out.\n> > > -\t\t *\n> > > -\t\t * \\todo: Handle the scaler (BDS) restrictions. The BDS can\n> > > -\t\t * only scale with the same factor in both directions, and the\n> > > -\t\t * scaling factor is limited to a multiple of 1/32. At the\n> > > -\t\t * moment the ImgU driver hides these constraints by applying\n> > > -\t\t * additional cropping, this should be fixed on the driver\n> > > -\t\t * side, and cropping should be exposed to us.\n> > > -\t\t */\n> > > -\t} else {\n> > > -\t\t/*\n> > > -\t\t * \\todo: Properly support cropping when the ImgU driver\n> > > -\t\t * interface will be cleaned up.\n> > > -\t\t */\n> > > -\t\tcfg.size = cio2Configuration_.size;\n> > > -\t}\n> > > -\n> > > -\t/*\n> > > -\t * Clamp the size to match the ImgU alignment constraints. The width\n> > > -\t * shall be a multiple of 8 pixels and the height a multiple of 4\n> > > -\t * pixels.\n> > > -\t */\n> > > -\tif (cfg.size.width % 8 || cfg.size.height % 4) {\n> > > -\t\tcfg.size.width &= ~7;\n> > > -\t\tcfg.size.height &= ~3;\n> > > -\t}\n> > > -}\n> > > -\n> > >  CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > >  {\n> > >  \tStatus status = Valid;\n> > > @@ -242,71 +149,141 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > >\n> > >  \t/*\n> > >  \t * Validate the requested stream configuration and select the sensor\n> > > -\t * format by collecting the maximum width and height and picking the\n> > > -\t * closest larger match, as the IPU3 can downscale only. If no\n> > > -\t * resolution is requested for any stream, or if no sensor resolution is\n> > > -\t * large enough, pick the largest one.\n> > > +\t * format by collecting the maximum RAW stream width and height and\n> > > +\t * picking the closest larger match, as the IPU3 can downscale only. If\n> > > +\t * no resolution is requested for the RAW stream, use the one from the\n> > > +\t * largest YUV stream, plus margins pixels for the IF and BDS to scale.\n> > > +\t * If no resolution is requested for any stream, pick the largest one.\n> > >  \t */\n> > >  \tunsigned int rawCount = 0;\n> > >  \tunsigned int yuvCount = 0;\n> > > -\tSize size;\n> > > +\tSize maxYuvSize;\n> > > +\tSize maxRawSize;\n> > >\n> > >  \tfor (const StreamConfiguration &cfg : config_) {\n> > >  \t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> > >\n> > > -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > >  \t\t\trawCount++;\n> > > -\t\telse\n> > > +\t\t\tmaxRawSize = maxRawSize.expandedTo(cfg.size);\n> >\n> > Not something that is needed as a prerequisite for this series, but do\n> > you think it would be useful to also have in-place versions of the Size\n> > (and Rectangle) helpers ? This line would become\n> >\n> > \t\t\tmaxRawSize.expandTo(cfg.size);\n> >\n> \n> Having chased for like 15 minutes why my Size didn't get update when I\n> used this, I would really say yes.\n\nSorry about causing you to lose time :-/ I wonder why the compiler\ndoesn't warn when calling a const method and ignoring the result\ncompletely.\n\nI've pushed the in-place helpers, feel free to use them in a new version\nof this series if you want.\n\n> > > +\t\t} else {\n> > >  \t\t\tyuvCount++;\n> > > -\n> > > -\t\tif (cfg.size.width > size.width)\n> > > -\t\t\tsize.width = cfg.size.width;\n> > > -\t\tif (cfg.size.height > size.height)\n> > > -\t\t\tsize.height = cfg.size.height;\n> > > +\t\t\tmaxYuvSize = maxYuvSize.expandedTo(cfg.size);\n> > > +\t\t}\n> > >  \t}\n> > >  \tif (rawCount > 1 || yuvCount > 2) {\n> > >  \t\tLOG(IPU3, Debug)\n> > >  \t\t\t<< \"Camera configuration not supported\";\n> > >  \t\treturn Invalid;\n> > >  \t}\n> >\n> > Maybe a blank line here (and above this block too) ?\n> >\n> > > +\tif (maxRawSize.isNull()) {\n> > > +\t\tmaxRawSize.width = utils::alignUp(maxYuvSize.width,\n> > > +\t\t\t\t\t\t  IMGU_OUTPUT_WIDTH_MARGIN);\n> > > +\t\tmaxRawSize.height = utils::alignUp(maxYuvSize.height,\n> > > +\t\t\t\t\t\t   IMGU_OUTPUT_HEIGHT_MARGIN);\n> >\n> > Maybe\n> >\n> > \t\tmaxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > \t\t\t\t\t\t    IMGU_OUTPUT_HEIGHT_MARGIN);\n> >\n> > ?\n> >\n> > > +\t\tmaxRawSize = maxRawSize.boundedTo(data_->cio2_.sensor()->resolution());\n> >\n> > Or even\n> >\n> > \t\tmaxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > \t\t\t\t\t\t    IMGU_OUTPUT_HEIGHT_MARGIN)\n> > \t\t\t\t       .boundedTo(data_->cio2_.sensor()->resolution());\n> >\n> \n> Yes, sorry, I got here through several attempts where I mangled sizes\n> individually and didn't notice I could have used the new Size methods\n\nNo worries :-)\n\n> > > +\t}\n> > >\n> > > -\t/* Generate raw configuration from CIO2. */\n> > > -\tcio2Configuration_ = data_->cio2_.generateConfiguration(size);\n> > > +\t/*\n> > > +\t * Generate raw configuration from CIO2.\n> > > +\t *\n> > > +\t * The output YUV streams will be limited in size to the maximum\n> > > +\t * frame size requested for the RAW stream.\n> > > +\t */\n> > > +\tcio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);\n> > >  \tif (!cio2Configuration_.pixelFormat.isValid())\n> > >  \t\treturn Invalid;\n> > >\n> > > -\t/* Assign streams to each configuration entry. */\n> > > -\tassignStreams();\n> > > +\tLOG(IPU3, Debug) << \"CIO2 configuration: \" << cio2Configuration_.toString();\n> > >\n> > > -\t/* Verify and adjust configuration if needed. */\n> > > +\t/*\n> > > +\t * Adjust the configurations if needed and assign streams while\n> > > +\t * iterating them.\n> > > +\t */\n> > > +\tbool mainOutputAvailable = true;\n> > >  \tfor (unsigned int i = 0; i < config_.size(); ++i) {\n> > > -\t\tStreamConfiguration &cfg = config_[i];\n> > > -\t\tconst StreamConfiguration oldCfg = cfg;\n> > > -\t\tconst Stream *stream = streams_[i];\n> > > -\n> > > -\t\tif (stream == &data_->rawStream_) {\n> > > -\t\t\tcfg.size = cio2Configuration_.size;\n> > > -\t\t\tcfg.pixelFormat = cio2Configuration_.pixelFormat;\n> > > -\t\t\tcfg.bufferCount = cio2Configuration_.bufferCount;\n> > > +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(config_[i].pixelFormat);\n> > > +\t\tconst StreamConfiguration originalCfg = config_[i];\n> > > +\t\tStreamConfiguration *cfg = &config_[i];\n> >\n> > I'm not asking for this to be changed, but out of curiotiry, why are you\n> > turning the reference into a pointer ?\n> \n> As we actually modify it's content. This is not a function parameter,\n> where we enforce that rule, but I still find more natural to use\n> pointers for things that have to be modified, and references for\n> things that stay constant.\n\nIt could be a good practice indeed.\n\n> > > +\n> > > +\t\tLOG(IPU3, Debug) << \"Validating configuration: \" << config_[i].toString();\n> >\n> > Would it make sense to s/configuration/stream/ ?\n> \n> You probably suggested this already and I missed it.\n> \n> > > +\n> > > +\n> >\n> > Extra blank line ?\n> \n> Have I missed a checkstyle warning ?\n\nI wish checkstyle was perfect :-)\n\n> > > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > > +\t\t\t/* Initialize the RAW stream with the CIO2 configuration. */\n> > > +\t\t\tcfg->size = cio2Configuration_.size;\n> > > +\t\t\tcfg->pixelFormat = cio2Configuration_.pixelFormat;\n> > > +\t\t\tcfg->bufferCount = cio2Configuration_.bufferCount;\n> > > +\t\t\tcfg->stride = info.stride(cfg->size.width, 0, 64);\n> > > +\t\t\tcfg->frameSize = info.frameSize(cfg->size, 64);\n> > > +\t\t\tcfg->setStream(const_cast<Stream *>(&data_->rawStream_));\n> > > +\n> > > +\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> > > +\t\t\t\t\t << \" to the raw stream\";\n> > >  \t\t} else {\n> > > -\t\t\tbool scale = stream == &data_->vfStream_;\n> > > -\t\t\tadjustStream(config_[i], scale);\n> > > -\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> > > +\t\t\t/* Assign and configure the main and viewfinder outputs. */\n> > > +\n> > > +\t\t\t/*\n> > > +\t\t\t * Clamp the size to match the ImgU size limits and the\n> > > +\t\t\t * margins from the CIO2 output frame size.\n> > > +\t\t\t *\n> > > +\t\t\t * The ImgU outputs needs to be rounded down to 64\n> > > +\t\t\t * pixels in width and 32 pixels in height from the\n> > > +\t\t\t * input frame size.\n> > > +\t\t\t *\n> > > +\t\t\t * \\todo Verify this assumption and find out if it\n> > > +\t\t\t * depends on the BDS scaling factor of 1/32, as the\n> > > +\t\t\t * main output has no YUV scaler as the viewfinder\n> > > +\t\t\t * output has.\n> > > +\t\t\t */\n> > > +\t\t\tunsigned int limit;\n> > > +\t\t\tlimit = utils::alignDown(cio2Configuration_.size.width - 1,\n> > > +\t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n> >\n> > Where does the - 1 come from ?\n> \n> The ouput sizes have to be strictly smaller than the input frame size.\n> Here I'm looking for the largest multiple of IMGU_OUTPUT_WIDTH_MARGIN\n> which is strictly smaller than cio2Configuration_.size.width.\n> \n> That's for the -1. The reason why I'm searching for a number aligned\n> to that margin value comes from inspecting the pipe configuration tool\n> results and the xml files. This seems to be the combination that\n> satisfies the tool in most cases. I know it's weak as a reasoning, but\n> I was not able to find out what are the hardware/firmware requirements\n> that lead to this, if not the way parameters are computed by the\n> script (which probably reflect how the hw/fw work though).\n\nI'm sure we'll rework this in the future when we'll get a better\nunderstanding of how the hardware works. Can I ask you to make a note of\nthe points you don't fully understand ? We can try to get answers to our\nquestions.\n\n> > > +\t\t\tcfg->size.width = utils::clamp(cfg->size.width,\n> > > +\t\t\t\t\t\t       IMGU_OUTPUT_MIN_SIZE.width,\n> > > +\t\t\t\t\t\t       limit);\n> > > +\n> > > +\t\t\tlimit = utils::alignDown(cio2Configuration_.size.height - 1,\n> > > +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> > > +\t\t\tcfg->size.height = utils::clamp(cfg->size.height,\n> > > +\t\t\t\t\t\t\tIMGU_OUTPUT_MIN_SIZE.height,\n> > > +\t\t\t\t\t\t\tlimit);\n> > > +\n> > > +\t\t\tcfg->size = cfg->size.alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> > > +\t\t\t\t\t\t\t    IMGU_OUTPUT_HEIGHT_ALIGN);\n> > > +\n> > > +\t\t\tcfg->pixelFormat = formats::NV12;\n> > > +\t\t\tcfg->bufferCount = IPU3_BUFFER_COUNT;\n> > > +\t\t\tcfg->stride = info.stride(cfg->size.width, 0, 1);\n> > > +\t\t\tcfg->frameSize = info.frameSize(cfg->size, 1);\n> > > +\n> > > +\t\t\t/*\n> > > +\t\t\t * Use the main output stream in case only one stream is\n> > > +\t\t\t * requested or if the current configuration is the one with\n> > > +\t\t\t * the maximum YUV output size.\n> > > +\t\t\t */\n> > > +\t\t\tif (mainOutputAvailable &&\n> > > +\t\t\t    (originalCfg.size == maxYuvSize || yuvCount == 1)) {\n> > > +\t\t\t\tcfg->setStream(const_cast<Stream *>(&data_->outStream_));\n> > > +\t\t\t\tmainOutputAvailable = false;\n> > > +\n> > > +\t\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> > > +\t\t\t\t\t\t << \" to the main output\";\n> > > +\t\t\t} else {\n> > > +\t\t\t\tcfg->setStream(const_cast<Stream *>(&data_->vfStream_));\n> > > +\n> > > +\t\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> > > +\t\t\t\t\t\t << \" to the viewfinder output\";\n> > > +\t\t\t}\n> > >  \t\t}\n> > >\n> > > -\t\tif (cfg.pixelFormat != oldCfg.pixelFormat ||\n> > > -\t\t    cfg.size != oldCfg.size) {\n> > > +\t\tif (cfg->pixelFormat != originalCfg.pixelFormat ||\n> > > +\t\t    cfg->size != originalCfg.size) {\n> > >  \t\t\tLOG(IPU3, Debug)\n> > >  \t\t\t\t<< \"Stream \" << i << \" configuration adjusted to \"\n> > > -\t\t\t\t<< cfg.toString();\n> > > +\t\t\t\t<< cfg->toString();\n> > >  \t\t\tstatus = Adjusted;\n> > >  \t\t}\n> > > -\n> > > -\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> > > -\t\tbool packedRaw = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > > -\n> > > -\t\tcfg.stride = info.stride(cfg.size.width, 0, packedRaw ? 64 : 1);\n> > > -\t\tcfg.frameSize = info.frameSize(cfg.size, packedRaw ? 64 : 1);\n> > >  \t}\n> > >\n> > >  \treturn status;\n> > > @@ -475,16 +452,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > >  \tbool vfActive = false;\n> > >\n> > >  \tfor (unsigned int i = 0; i < config->size(); ++i) {\n> > > -\t\t/*\n> > > -\t\t * Use a const_cast<> here instead of storing a mutable stream\n> > > -\t\t * pointer in the configuration to let the compiler catch\n> > > -\t\t * unwanted modifications of camera data in the configuration\n> > > -\t\t * validate() implementation.\n> > > -\t\t */\n> > > -\t\tStream *stream = const_cast<Stream *>(config->streams()[i]);\n> > >  \t\tStreamConfiguration &cfg = (*config)[i];\n> > > -\n> > > -\t\tcfg.setStream(stream);\n> > > +\t\tStream *stream = cfg.stream();\n> > >\n> > >  \t\tif (stream == outStream) {\n> > >  \t\t\tret = imgu->configureOutput(cfg, &outputFormat);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 99DEDBD792\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 16 Jul 2020 23:37:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 199F660553;\n\tFri, 17 Jul 2020 01:37:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5CB9B60493\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 17 Jul 2020 01:37:32 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CA2C52B7;\n\tFri, 17 Jul 2020 01:37:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jJfbBs/9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594942652;\n\tbh=o1hoJf6fUyhd7vAvNCMSYmyhBum0yYSYIvlfV5914FM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jJfbBs/9PkK93jvHsl6yST8zWKGMH37vaC/WmEX+GRFPQGlKdHwXQw3eiqowJMFnb\n\tVfSUJbk0iMjCjwLbPhpu9oPvPyPpZEjJ+Qv8Q+e8EsQ6y/O5Zgaj7ELEwQ6bZ3q5Ww\n\t0NqIiQ3APyUd+wkJLMZm8CsYDOxxGbXmnj2q+oVs=","Date":"Fri, 17 Jul 2020 02:37:24 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20200716233724.GK5960@pendragon.ideasonboard.com>","References":"<20200714104212.48683-1-jacopo@jmondi.org>\n\t<20200714104212.48683-15-jacopo@jmondi.org>\n\t<20200714220652.GH5854@pendragon.ideasonboard.com>\n\t<20200715073551.u6qp7lgipghod3kw@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200715073551.u6qp7lgipghod3kw@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 14/20] libcamera: ipu3: Adjust and\n\tassign streams in validate()","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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]