[{"id":11285,"web_url":"https://patchwork.libcamera.org/comment/11285/","msgid":"<20200709133850.GM3875643@oden.dyn.berto.se>","date":"2020-07-09T13:38:50","subject":"Re: [libcamera-devel] [PATCH v2 10/20] libcamera: ipu3: Adjust and\n\tassign streams in validate()","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work, I like this change.\n\nOn 2020-07-09 10:41:18 +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 assume 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 \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 204 ++++++++++++---------------\n> 1 file changed, 91 insertions(+), 113 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 9128e42d42ed..18f4a02cc270 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -69,9 +69,6 @@ public:\n> \tconst std::vector &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> @@ -136,96 +133,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 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> @@ -248,17 +155,26 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t */\n> \tunsigned int rawCount = 0;\n> \tunsigned int outCount = 0;\n> +\tSize yuvSize;\n> \tSize size;\n> \n> \tfor (const StreamConfiguration &cfg : config_) {\n> \t\tconst PixelFormatInfo &info =\n> \t\t\tPixelFormatInfo::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} else {\n> \t\t\toutCount++;\n> \n> +\t\t\t/*\n> +\t\t\t * Collect the maximum processed size to later assign\n> +\t\t\t * streams to configurations.\n> +\t\t\t */\n> +\t\t\tif (cfg.size > yuvSize)\n> +\t\t\t\tyuvSize = cfg.size;\n> +\t\t}\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> @@ -277,24 +193,94 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \tif (!cio2Configuration_.pixelFormat.isValid())\n> \t\treturn Invalid;\n> \n> -\t/* Assign streams to each configuration entry. */\n> -\tassignStreams();\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> +\t\tconst PixelFormatInfo &info =\n> +\t\t\tPixelFormatInfo::info(cfg.pixelFormat);\n> \n> -\t\tif (stream == &data_->rawStream_) {\n> +\t\tLOG(IPU3, Debug) << \"Validating configuration: \" << cfg.toString();\n> +\n> +\t\t/* Initialize the RAW stream with the CIO2 configuration. */\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\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.setStream(const_cast(&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\tcontinue;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Assign and configure the main and viewfinder outputs.\n> +\t\t */\n> +\n> +\t\t/* Clamp the size to match the ImgU size limits. */\n> +\t\tcfg.size.width = utils::clamp(cfg.size.width,\n> +\t\t\t\t\t minIPU3OutputSize.width,\n> +\t\t\t\t\t IPU3_OUTPUT_MAX_WIDTH);\n> +\t\tcfg.size.height = utils::clamp(cfg.size.height,\n> +\t\t\t\t\t minIPU3OutputSize.height,\n> +\t\t\t\t\t IPU3_OUTPUT_MAX_HEIGHT);\n> +\n> +\t\t/*\n> +\t\t * All the ImgU output stream should be smaller than the ImgU\n> +\t\t * input frame, to give space to the IF and BDS to crop the\n> +\t\t * image.\n> +\t\t *\n> +\t\t * \\todo Give the BDS rectangle at least 32 pixels from the\n> +\t\t * input frame size. This is an assumption deduced\n> +\t\t * from inspecting the pipe configuration tool output\n> +\t\t * and the platform configuration files shipped for the\n> +\t\t * Soraka device by ChromeOS.\n> +\t\t */\n> +\t\tif (cfg.size.width >= (cio2Configuration_.size.width - 31))\n> +\t\t\tcfg.size.width = cio2Configuration_.size.width - 32;\n> +\n> +\t\tif (cfg.size.height >= (cio2Configuration_.size.height - 31))\n> +\t\t\tcfg.size.height = cio2Configuration_.size.height - 32;\n\nCould you not clamp cfg.size.width within minIPU3OutputSize.width and \ncio2Configuration_.size.width - 32 instead of minIPU3OutputSize.width \nand IPU3_OUTPUT_MAX_WIDTH?\n\nSame for hight of course.\n\n> +\n> +\t\t/*\n> +\t\t * Adjust to match the main output or the viewfinder\n> +\t\t * output constraints and assign streams.\n> +\t\t *\n> +\t\t * Use the main output stream in case only one stream is\n> +\t\t * requested or if the current configuration is the one with\n> +\t\t * the maximum RGB/YUV output size.\n> +\t\t */\n\nThis comment is 1 (see below).\n\n> +\t\tcfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;\n> +\t\tcfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;\n> +\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> +\t\tcfg.pixelFormat = formats::NV12;\n> +\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\nI would drop this comment and move comment [1] here. There is no need to \ndocument const_cast<>() for the setStream() call for the RAW stream what \nis different here?\n\n> +\t\tStream *stream;\n> +\t\tif (mainOutputAvailable &&\n> +\t\t (oldCfg.size == yuvSize || outCount == 1)) {\n> +\t\t\tstream = const_cast(&data_->outStream_);\n> +\t\t\tmainOutputAvailable = false;\n> +\n> +\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg.toString()\n> +\t\t\t\t\t << \" to the main output\";\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\tstream = const_cast(&data_->vfStream_);\n> +\n> +\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg.toString()\n> +\t\t\t\t\t << \" to the viewfinder output\";\n> \t\t}\n> +\t\tcfg.setStream(stream);\n> \n> \t\tif (cfg.pixelFormat != oldCfg.pixelFormat ||\n> \t\t cfg.size != oldCfg.size) {\n> @@ -472,16 +458,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(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> 2.27.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":"","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 472E1BD792\n\tfor ;\n\tThu, 9 Jul 2020 13:38:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2C96612E1;\n\tThu, 9 Jul 2020 15:38:53 +0200 (CEST)","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 A1B50603AE\n\tfor ;\n\tThu, 9 Jul 2020 15:38:52 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id b25so2425223ljp.6\n\tfor ;\n\tThu, 09 Jul 2020 06:38:52 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tj26sm823699lji.29.2020.07.09.06.38.50\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 09 Jul 2020 06:38:51 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"im+AHzu6\"; dkim-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=Z35nmcd+ymClapJEb0HzOdvEh8gPXy38FX5bs4emZy4=;\n\tb=im+AHzu6Vkhu3YD5PZT0lKUIeGPVqxVgUHatLHi+0lWRpxN9RbTzYyyZVepEi03VAS\n\tTAVFC6FS6VGNhRyol4cIdJkgS9V9QaTL0HGUd4+Kz8tto7xtJ82r8jFmvunvbPzGkIxP\n\tc2GTAww5HefL3vQS4H5ooBNZHtiKKGJPOVohSevfQDP5eXNCcIDW/ViWW2HjA8WWj5F7\n\tBI6zK6x/l/C/lHRasSWJlnib1wbdrayfHLtUUMHPwWPGrnSFfBHWQ4jSEfFicOTLymk3\n\tZiJC/Hz5Tjz7YmBoaMHfBcHLAAlq7C+LNu0aIa4HPHLeyhXlpfh+ecxShJ076K1V99jc\n\tWhgw==","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=Z35nmcd+ymClapJEb0HzOdvEh8gPXy38FX5bs4emZy4=;\n\tb=VCWuywUaVssGi3ENomE2rB/YQBMKjCxdQ+WBy374t7XB1SWthCxoGaZQ6CHDVGA6pi\n\tfGIQaoBSxfHx8ekGfSLMA3inxlE7aN+nq+UHSKkmLMlJ4zBiNjGUpn5ZUmK02pOa9QfY\n\t1twShU3zVO7ACmYLwEdlW0bW16ETaNFvszg2PbwP9gl9r+3yiIAU3gvk1WdqYWNYSRCd\n\tejejJxV9lFH1154ndFO++o14XXB9EGNze5l0KNJGggp/hf/dpPr4NV8YYnyGnQ2PT3NA\n\txAxownC2jAiLJ652hBPLmeECdFxZLnHlvTDPXmMhypWDPRHuoM56w0buzdP2Mjhcp+AE\n\tqweg==","X-Gm-Message-State":"AOAM532XO9UOXNHOrG+H56d2VtX6M1bW9O1J5Cds/tuMjX8qyA7gZ6J0\n\tABmS5NiB5exYlGlqZ3vO7347JQ==","X-Google-Smtp-Source":"ABdhPJxp7qjx3TZ43JXtCx/yR9rwyhH1sHLAXjzy3oCrJ6IBUnbgd7l3HfUR3SfW6iQ3uB2bGT2rag==","X-Received":"by 2002:a2e:a552:: with SMTP id\n\te18mr36369779ljn.162.1594301931797; \n\tThu, 09 Jul 2020 06:38:51 -0700 (PDT)","Date":"Thu, 9 Jul 2020 15:38:50 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200709133850.GM3875643@oden.dyn.berto.se>","References":"<20200709084128.5316-1-jacopo@jmondi.org>\n\t<20200709084128.5316-11-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200709084128.5316-11-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 10/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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11307,"web_url":"https://patchwork.libcamera.org/comment/11307/","msgid":"<20200710071420.eorxdjrjgdft6x37@uno.localdomain>","date":"2020-07-10T07:14:20","subject":"Re: [libcamera-devel] [PATCH v2 10/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 Niklas,\n\nOn Thu, Jul 09, 2020 at 03:38:50PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work, I like this change.\n>\n> On 2020-07-09 10:41:18 +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 assume 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 \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 204 ++++++++++++---------------\n> > 1 file changed, 91 insertions(+), 113 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 9128e42d42ed..18f4a02cc270 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -69,9 +69,6 @@ public:\n> > \tconst std::vector &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> > @@ -136,96 +133,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 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> > @@ -248,17 +155,26 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > \t */\n> > \tunsigned int rawCount = 0;\n> > \tunsigned int outCount = 0;\n> > +\tSize yuvSize;\n> > \tSize size;\n> >\n> > \tfor (const StreamConfiguration &cfg : config_) {\n> > \t\tconst PixelFormatInfo &info =\n> > \t\t\tPixelFormatInfo::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} else {\n> > \t\t\toutCount++;\n> >\n> > +\t\t\t/*\n> > +\t\t\t * Collect the maximum processed size to later assign\n> > +\t\t\t * streams to configurations.\n> > +\t\t\t */\n> > +\t\t\tif (cfg.size > yuvSize)\n> > +\t\t\t\tyuvSize = cfg.size;\n> > +\t\t}\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> > @@ -277,24 +193,94 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > \tif (!cio2Configuration_.pixelFormat.isValid())\n> > \t\treturn Invalid;\n> >\n> > -\t/* Assign streams to each configuration entry. */\n> > -\tassignStreams();\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> > +\t\tconst PixelFormatInfo &info =\n> > +\t\t\tPixelFormatInfo::info(cfg.pixelFormat);\n> >\n> > -\t\tif (stream == &data_->rawStream_) {\n> > +\t\tLOG(IPU3, Debug) << \"Validating configuration: \" << cfg.toString();\n> > +\n> > +\t\t/* Initialize the RAW stream with the CIO2 configuration. */\n> > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\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.setStream(const_cast(&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\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Assign and configure the main and viewfinder outputs.\n> > +\t\t */\n> > +\n> > +\t\t/* Clamp the size to match the ImgU size limits. */\n> > +\t\tcfg.size.width = utils::clamp(cfg.size.width,\n> > +\t\t\t\t\t minIPU3OutputSize.width,\n> > +\t\t\t\t\t IPU3_OUTPUT_MAX_WIDTH);\n> > +\t\tcfg.size.height = utils::clamp(cfg.size.height,\n> > +\t\t\t\t\t minIPU3OutputSize.height,\n> > +\t\t\t\t\t IPU3_OUTPUT_MAX_HEIGHT);\n> > +\n> > +\t\t/*\n> > +\t\t * All the ImgU output stream should be smaller than the ImgU\n> > +\t\t * input frame, to give space to the IF and BDS to crop the\n> > +\t\t * image.\n> > +\t\t *\n> > +\t\t * \\todo Give the BDS rectangle at least 32 pixels from the\n> > +\t\t * input frame size. This is an assumption deduced\n> > +\t\t * from inspecting the pipe configuration tool output\n> > +\t\t * and the platform configuration files shipped for the\n> > +\t\t * Soraka device by ChromeOS.\n> > +\t\t */\n> > +\t\tif (cfg.size.width >= (cio2Configuration_.size.width - 31))\n> > +\t\t\tcfg.size.width = cio2Configuration_.size.width - 32;\n> > +\n> > +\t\tif (cfg.size.height >= (cio2Configuration_.size.height - 31))\n> > +\t\t\tcfg.size.height = cio2Configuration_.size.height - 32;\n>\n> Could you not clamp cfg.size.width within minIPU3OutputSize.width and\n> cio2Configuration_.size.width - 32 instead of minIPU3OutputSize.width\n> and IPU3_OUTPUT_MAX_WIDTH?\n\nI probably should! I had this two steps procedure, but there's not\nreason to keep it\n\n>\n> Same for hight of course.\n>\n> > +\n> > +\t\t/*\n> > +\t\t * Adjust to match the main output or the viewfinder\n> > +\t\t * output constraints and assign streams.\n> > +\t\t *\n> > +\t\t * Use the main output stream in case only one stream is\n> > +\t\t * requested or if the current configuration is the one with\n> > +\t\t * the maximum RGB/YUV output size.\n> > +\t\t */\n>\n> This comment is 1 (see below).\n>\n> > +\t\tcfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;\n> > +\t\tcfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;\n> > +\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> > +\t\tcfg.pixelFormat = formats::NV12;\n> > +\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>\n> I would drop this comment and move comment [1] here. There is no need to\n> document const_cast<>() for the setStream() call for the RAW stream what\n> is different here?\n\nJust copied from configure(). Later in the series a commit will remove\nit explictely instead of slipping its removal in with this change.\n\n>\n> > +\t\tStream *stream;\n> > +\t\tif (mainOutputAvailable &&\n> > +\t\t (oldCfg.size == yuvSize || outCount == 1)) {\n> > +\t\t\tstream = const_cast(&data_->outStream_);\n> > +\t\t\tmainOutputAvailable = false;\n> > +\n> > +\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg.toString()\n> > +\t\t\t\t\t << \" to the main output\";\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\tstream = const_cast(&data_->vfStream_);\n> > +\n> > +\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg.toString()\n> > +\t\t\t\t\t << \" to the viewfinder output\";\n> > \t\t}\n> > +\t\tcfg.setStream(stream);\n> >\n> > \t\tif (cfg.pixelFormat != oldCfg.pixelFormat ||\n> > \t\t cfg.size != oldCfg.size) {\n> > @@ -472,16 +458,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(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> > 2.27.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","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 AEAA4BD790\n\tfor ;\n\tFri, 10 Jul 2020 07:10:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 451DE613AB;\n\tFri, 10 Jul 2020 09:10:48 +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 8CB9661167\n\tfor ;\n\tFri, 10 Jul 2020 09:10:47 +0200 (CEST)","from uno.localdomain (host-95-245-128-189.retail.telecomitalia.it\n\t[95.245.128.189]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 53B871C0004;\n\tFri, 10 Jul 2020 07:10:45 +0000 (UTC)"],"X-Originating-IP":"95.245.128.189","Date":"Fri, 10 Jul 2020 09:14:20 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200710071420.eorxdjrjgdft6x37@uno.localdomain>","References":"<20200709084128.5316-1-jacopo@jmondi.org>\n\t<20200709084128.5316-11-jacopo@jmondi.org>\n\t<20200709133850.GM3875643@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200709133850.GM3875643@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v2 10/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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11319,"web_url":"https://patchwork.libcamera.org/comment/11319/","msgid":"<20200710112854.GH5964@pendragon.ideasonboard.com>","date":"2020-07-10T11:28:54","subject":"Re: [libcamera-devel] [PATCH v2 10/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 Fri, Jul 10, 2020 at 09:14:20AM +0200, Jacopo Mondi wrote:\n> On Thu, Jul 09, 2020 at 03:38:50PM +0200, Niklas Söderlund wrote:\n> > On 2020-07-09 10:41:18 +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 assume only full-size streams\n\ns/assume/assumes/\n\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 \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 204 ++++++++++++---------------\n> > > 1 file changed, 91 insertions(+), 113 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 9128e42d42ed..18f4a02cc270 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -69,9 +69,6 @@ public:\n> > > \tconst std::vector &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> > > @@ -136,96 +133,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 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> > > @@ -248,17 +155,26 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > \t */\n> > > \tunsigned int rawCount = 0;\n> > > \tunsigned int outCount = 0;\n\nyuvCount ?\n\n> > > +\tSize yuvSize;\n> > > \tSize size;\n> > >\n> > > \tfor (const StreamConfiguration &cfg : config_) {\n> > > \t\tconst PixelFormatInfo &info =\n> > > \t\t\tPixelFormatInfo::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} else {\n> > > \t\t\toutCount++;\n> > >\n> > > +\t\t\t/*\n> > > +\t\t\t * Collect the maximum processed size to later assign\n> > > +\t\t\t * streams to configurations.\n> > > +\t\t\t */\n\nMaybe maxYuvSize instead of yuvSize then ?\n\n> > > +\t\t\tif (cfg.size > yuvSize)\n> > > +\t\t\t\tyuvSize = cfg.size;\n\nNote that the > operator on Size is implemented based on the < operator,\nwhich is defined as\n\n- A size with smaller width and smaller height is smaller.\n- A size with smaller area is smaller.\n- A size with smaller width is smaller.\n\nI wonder if here you don't want the bounding rectangle instead, which\ncould be calculated with\n\n\t\t\tyuvSize = yuvSize.expandedTo(cfg.size);\n\n(Size::expandedTo() is a new function from the not-yet-merged\n\"libcamera: geometry: Add helper functions to the Size class\" patch)\n\n> > > +\t\t}\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> > > @@ -277,24 +193,94 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > \tif (!cio2Configuration_.pixelFormat.isValid())\n> > > \t\treturn Invalid;\n> > >\n> > > -\t/* Assign streams to each configuration entry. */\n> > > -\tassignStreams();\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\nI'd name this requestedCfg or originalCfg.\n\n> > > -\t\tconst Stream *stream = streams_[i];\n> > > +\t\tconst PixelFormatInfo &info =\n> > > +\t\t\tPixelFormatInfo::info(cfg.pixelFormat);\n> > >\n> > > -\t\tif (stream == &data_->rawStream_) {\n> > > +\t\tLOG(IPU3, Debug) << \"Validating configuration: \" << cfg.toString();\n\nMaybe s/configuration/stream/ ?\n\n> > > +\n> > > +\t\t/* Initialize the RAW stream with the CIO2 configuration. */\n> > > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\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.setStream(const_cast(&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\tcontinue;\n> > > +\t\t}\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Assign and configure the main and viewfinder outputs.\n> > > +\t\t */\n\nThis holds on a single line.\n\n> > > +\n> > > +\t\t/* Clamp the size to match the ImgU size limits. */\n> > > +\t\tcfg.size.width = utils::clamp(cfg.size.width,\n> > > +\t\t\t\t\t minIPU3OutputSize.width,\n> > > +\t\t\t\t\t IPU3_OUTPUT_MAX_WIDTH);\n> > > +\t\tcfg.size.height = utils::clamp(cfg.size.height,\n> > > +\t\t\t\t\t minIPU3OutputSize.height,\n> > > +\t\t\t\t\t IPU3_OUTPUT_MAX_HEIGHT);\n> > > +\n> > > +\t\t/*\n> > > +\t\t * All the ImgU output stream should be smaller than the ImgU\n\ns/stream/streams/\n\nReviewed-by: Laurent Pinchart \n\n> > > +\t\t * input frame, to give space to the IF and BDS to crop the\n> > > +\t\t * image.\n> > > +\t\t *\n> > > +\t\t * \\todo Give the BDS rectangle at least 32 pixels from the\n> > > +\t\t * input frame size. This is an assumption deduced\n> > > +\t\t * from inspecting the pipe configuration tool output\n> > > +\t\t * and the platform configuration files shipped for the\n> > > +\t\t * Soraka device by ChromeOS.\n> > > +\t\t */\n> > > +\t\tif (cfg.size.width >= (cio2Configuration_.size.width - 31))\n> > > +\t\t\tcfg.size.width = cio2Configuration_.size.width - 32;\n> > > +\n> > > +\t\tif (cfg.size.height >= (cio2Configuration_.size.height - 31))\n> > > +\t\t\tcfg.size.height = cio2Configuration_.size.height - 32;\n> >\n> > Could you not clamp cfg.size.width within minIPU3OutputSize.width and\n> > cio2Configuration_.size.width - 32 instead of minIPU3OutputSize.width\n> > and IPU3_OUTPUT_MAX_WIDTH?\n> \n> I probably should! I had this two steps procedure, but there's not\n> reason to keep it\n> \n> > Same for hight of course.\n> >\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Adjust to match the main output or the viewfinder\n> > > +\t\t * output constraints and assign streams.\n> > > +\t\t *\n> > > +\t\t * Use the main output stream in case only one stream is\n> > > +\t\t * requested or if the current configuration is the one with\n> > > +\t\t * the maximum RGB/YUV output size.\n> > > +\t\t */\n> >\n> > This comment is 1 (see below).\n> >\n> > > +\t\tcfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;\n> > > +\t\tcfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;\n> > > +\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> > > +\t\tcfg.pixelFormat = formats::NV12;\n> > > +\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> >\n> > I would drop this comment and move comment [1] here. There is no need to\n> > document const_cast<>() for the setStream() call for the RAW stream what\n> > is different here?\n> \n> Just copied from configure(). Later in the series a commit will remove\n> it explictely instead of slipping its removal in with this change.\n> \n> > > +\t\tStream *stream;\n> > > +\t\tif (mainOutputAvailable &&\n> > > +\t\t (oldCfg.size == yuvSize || outCount == 1)) {\n> > > +\t\t\tstream = const_cast(&data_->outStream_);\n> > > +\t\t\tmainOutputAvailable = false;\n> > > +\n> > > +\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg.toString()\n> > > +\t\t\t\t\t << \" to the main output\";\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\tstream = const_cast(&data_->vfStream_);\n> > > +\n> > > +\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg.toString()\n> > > +\t\t\t\t\t << \" to the viewfinder output\";\n> > > \t\t}\n> > > +\t\tcfg.setStream(stream);\n> > >\n> > > \t\tif (cfg.pixelFormat != oldCfg.pixelFormat ||\n> > > \t\t cfg.size != oldCfg.size) {\n> > > @@ -472,16 +458,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(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":"","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 40AA9BD792\n\tfor ;\n\tFri, 10 Jul 2020 11:29:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D5AB613BD;\n\tFri, 10 Jul 2020 13:29:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 319AC6118A\n\tfor ;\n\tFri, 10 Jul 2020 13:29:01 +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 93FD22C0;\n\tFri, 10 Jul 2020 13:29:00 +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=\"mwvh4ZkP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594380540;\n\tbh=K/WUzcmM3Nua7ipAMJaoWyDzmPboREGPXw4uwk1tNv4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mwvh4ZkPrpAZGXYAZ2KZbyRD/owBiiO7gwiHxrwIqvbui4rETdjk0csyImeGL2Aay\n\tvl13geX9+BPOv0g6F8/bw7nzW42B24NLQtZUjgE/YhF8g1xDs1vt7PSrlIYsJaa+OW\n\tASDrz9IazH9shSuRVtZ7NcmloLm2eUxVnSKfg1v4=","Date":"Fri, 10 Jul 2020 14:28:54 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200710112854.GH5964@pendragon.ideasonboard.com>","References":"<20200709084128.5316-1-jacopo@jmondi.org>\n\t<20200709084128.5316-11-jacopo@jmondi.org>\n\t<20200709133850.GM3875643@oden.dyn.berto.se>\n\t<20200710071420.eorxdjrjgdft6x37@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200710071420.eorxdjrjgdft6x37@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 10/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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11328,"web_url":"https://patchwork.libcamera.org/comment/11328/","msgid":"<20200710123150.t2o3pii5ncjqcwlb@uno.localdomain>","date":"2020-07-10T12:31:50","subject":"Re: [libcamera-devel] [PATCH v2 10/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 Fri, Jul 10, 2020 at 02:28:54PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Jul 10, 2020 at 09:14:20AM +0200, Jacopo Mondi wrote:\n> > On Thu, Jul 09, 2020 at 03:38:50PM +0200, Niklas Söderlund wrote:\n> > > On 2020-07-09 10:41:18 +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 assume only full-size streams\n>\n> s/assume/assumes/\n>\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 \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 204 ++++++++++++---------------\n> > > > 1 file changed, 91 insertions(+), 113 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 9128e42d42ed..18f4a02cc270 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -69,9 +69,6 @@ public:\n> > > > \tconst std::vector &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> > > > @@ -136,96 +133,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 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> > > > @@ -248,17 +155,26 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > \t */\n> > > > \tunsigned int rawCount = 0;\n> > > > \tunsigned int outCount = 0;\n>\n> yuvCount ?\n>\n\nAs a general question, I refrained from using yuv as this could very\nwell be an RGB stream.. ah no, ImgU can only produce NV12 :)\n\n> > > > +\tSize yuvSize;\n> > > > \tSize size;\n> > > >\n> > > > \tfor (const StreamConfiguration &cfg : config_) {\n> > > > \t\tconst PixelFormatInfo &info =\n> > > > \t\t\tPixelFormatInfo::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} else {\n> > > > \t\t\toutCount++;\n> > > >\n> > > > +\t\t\t/*\n> > > > +\t\t\t * Collect the maximum processed size to later assign\n> > > > +\t\t\t * streams to configurations.\n> > > > +\t\t\t */\n>\n> Maybe maxYuvSize instead of yuvSize then ?\n>\n> > > > +\t\t\tif (cfg.size > yuvSize)\n> > > > +\t\t\t\tyuvSize = cfg.size;\n>\n> Note that the > operator on Size is implemented based on the < operator,\n> which is defined as\n>\n> - A size with smaller width and smaller height is smaller.\n> - A size with smaller area is smaller.\n> - A size with smaller width is smaller.\n>\n> I wonder if here you don't want the bounding rectangle instead, which\n> could be calculated with\n>\n> \t\t\tyuvSize = yuvSize.expandedTo(cfg.size);\n\nI get what's happening, but in a few weeks I'll re-read that, then I\nhave to open the expandedTo() implementation and realize this expands\nto the largest of the two sizes. Maybe I'll add a comment, but I found\nit hard to remember as a pattern :)\n\n>\n> (Size::expandedTo() is a new function from the not-yet-merged\n> \"libcamera: geometry: Add helper functions to the Size class\" patch)\n>\n> > > > +\t\t}\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> > > > @@ -277,24 +193,94 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > \tif (!cio2Configuration_.pixelFormat.isValid())\n> > > > \t\treturn Invalid;\n> > > >\n> > > > -\t/* Assign streams to each configuration entry. */\n> > > > -\tassignStreams();\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>\n> I'd name this requestedCfg or originalCfg.\n>\n\nWas there already, and I'm always carefull in changing existing code.\nPhone niklas for details.\n\n> > > > -\t\tconst Stream *stream = streams_[i];\n> > > > +\t\tconst PixelFormatInfo &info =\n> > > > +\t\t\tPixelFormatInfo::info(cfg.pixelFormat);\n> > > >\n> > > > -\t\tif (stream == &data_->rawStream_) {\n> > > > +\t\tLOG(IPU3, Debug) << \"Validating configuration: \" << cfg.toString();\n>\n> Maybe s/configuration/stream/ ?\n>\n> > > > +\n> > > > +\t\t/* Initialize the RAW stream with the CIO2 configuration. */\n> > > > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\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.setStream(const_cast(&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\tcontinue;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\t/*\n> > > > +\t\t * Assign and configure the main and viewfinder outputs.\n> > > > +\t\t */\n>\n> This holds on a single line.\n>\n\nActually this was intentional as here a 'different' section of code\nbegins. I could change it, as it is not a common pattern indeed.\n\n> > > > +\n> > > > +\t\t/* Clamp the size to match the ImgU size limits. */\n> > > > +\t\tcfg.size.width = utils::clamp(cfg.size.width,\n> > > > +\t\t\t\t\t minIPU3OutputSize.width,\n> > > > +\t\t\t\t\t IPU3_OUTPUT_MAX_WIDTH);\n> > > > +\t\tcfg.size.height = utils::clamp(cfg.size.height,\n> > > > +\t\t\t\t\t minIPU3OutputSize.height,\n> > > > +\t\t\t\t\t IPU3_OUTPUT_MAX_HEIGHT);\n> > > > +\n> > > > +\t\t/*\n> > > > +\t\t * All the ImgU output stream should be smaller than the ImgU\n>\n> s/stream/streams/\n>\n> Reviewed-by: Laurent Pinchart \n>\n\nThanks\n j\n\n> > > > +\t\t * input frame, to give space to the IF and BDS to crop the\n> > > > +\t\t * image.\n> > > > +\t\t *\n> > > > +\t\t * \\todo Give the BDS rectangle at least 32 pixels from the\n> > > > +\t\t * input frame size. This is an assumption deduced\n> > > > +\t\t * from inspecting the pipe configuration tool output\n> > > > +\t\t * and the platform configuration files shipped for the\n> > > > +\t\t * Soraka device by ChromeOS.\n> > > > +\t\t */\n> > > > +\t\tif (cfg.size.width >= (cio2Configuration_.size.width - 31))\n> > > > +\t\t\tcfg.size.width = cio2Configuration_.size.width - 32;\n> > > > +\n> > > > +\t\tif (cfg.size.height >= (cio2Configuration_.size.height - 31))\n> > > > +\t\t\tcfg.size.height = cio2Configuration_.size.height - 32;\n> > >\n> > > Could you not clamp cfg.size.width within minIPU3OutputSize.width and\n> > > cio2Configuration_.size.width - 32 instead of minIPU3OutputSize.width\n> > > and IPU3_OUTPUT_MAX_WIDTH?\n> >\n> > I probably should! I had this two steps procedure, but there's not\n> > reason to keep it\n> >\n> > > Same for hight of course.\n> > >\n> > > > +\n> > > > +\t\t/*\n> > > > +\t\t * Adjust to match the main output or the viewfinder\n> > > > +\t\t * output constraints and assign streams.\n> > > > +\t\t *\n> > > > +\t\t * Use the main output stream in case only one stream is\n> > > > +\t\t * requested or if the current configuration is the one with\n> > > > +\t\t * the maximum RGB/YUV output size.\n> > > > +\t\t */\n> > >\n> > > This comment is 1 (see below).\n> > >\n> > > > +\t\tcfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;\n> > > > +\t\tcfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;\n> > > > +\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> > > > +\t\tcfg.pixelFormat = formats::NV12;\n> > > > +\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> > >\n> > > I would drop this comment and move comment [1] here. There is no need to\n> > > document const_cast<>() for the setStream() call for the RAW stream what\n> > > is different here?\n> >\n> > Just copied from configure(). Later in the series a commit will remove\n> > it explictely instead of slipping its removal in with this change.\n> >\n> > > > +\t\tStream *stream;\n> > > > +\t\tif (mainOutputAvailable &&\n> > > > +\t\t (oldCfg.size == yuvSize || outCount == 1)) {\n> > > > +\t\t\tstream = const_cast(&data_->outStream_);\n> > > > +\t\t\tmainOutputAvailable = false;\n> > > > +\n> > > > +\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg.toString()\n> > > > +\t\t\t\t\t << \" to the main output\";\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\tstream = const_cast(&data_->vfStream_);\n> > > > +\n> > > > +\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg.toString()\n> > > > +\t\t\t\t\t << \" to the viewfinder output\";\n> > > > \t\t}\n> > > > +\t\tcfg.setStream(stream);\n> > > >\n> > > > \t\tif (cfg.pixelFormat != oldCfg.pixelFormat ||\n> > > > \t\t cfg.size != oldCfg.size) {\n> > > > @@ -472,16 +458,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(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":"","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 1A167BD792\n\tfor ;\n\tFri, 10 Jul 2020 12:28:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A96DF613AF;\n\tFri, 10 Jul 2020 14:28:18 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E53B7611BA\n\tfor ;\n\tFri, 10 Jul 2020 14:28:16 +0200 (CEST)","from uno.localdomain (host-95-245-128-189.retail.telecomitalia.it\n\t[95.245.128.189]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 6E6A71BF20E;\n\tFri, 10 Jul 2020 12:28:15 +0000 (UTC)"],"X-Originating-IP":"95.245.128.189","Date":"Fri, 10 Jul 2020 14:31:50 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200710123150.t2o3pii5ncjqcwlb@uno.localdomain>","References":"<20200709084128.5316-1-jacopo@jmondi.org>\n\t<20200709084128.5316-11-jacopo@jmondi.org>\n\t<20200709133850.GM3875643@oden.dyn.berto.se>\n\t<20200710071420.eorxdjrjgdft6x37@uno.localdomain>\n\t<20200710112854.GH5964@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200710112854.GH5964@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 10/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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]