[{"id":36381,"web_url":"https://patchwork.libcamera.org/comment/36381/","msgid":"<85sefcgl02.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-10-21T18:36:29","subject":"Re: [PATCH v13 0/8] Enable raw streams with software ISP","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"There is another branch of this series by Umang. Since there was no\npreference expressed which branch to use for the next version, I\ncontinue with mine.\n\nWhile I tried to incorporate some changes from Umang's branch\npreviously, some differences remain, which I think require more\ndiscussion. I rebased Umang's branch on the same master version, made a\ndiff between the two branches and commented on it below. The diff shows\nan update of the posted patches to Umang's branch, i.e. the original\nversion in the diff is this v13, the new version is the rebased Umang's\nbranch.\n\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 607b07949..529d7344d 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -11,7 +11,6 @@\n> #include \n> #include \n> #include \n> -#include \n> #include \n> #include \n> #include \n> @@ -26,9 +25,7 @@\n> #include \n> \n> #include \n> -#include \n> #include \n> -#include \n> #include \n> #include \n> #include \n> @@ -40,7 +37,6 @@\n> #include \"libcamera/internal/converter.h\"\n> #include \"libcamera/internal/delayed_controls.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> -#include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/global_configuration.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> @@ -267,9 +263,9 @@ static const SimplePipelineInfo supportedDevices[] = {\n> { \"sun6i-csi\", {}, false },\n> };\n> \n> -bool isRaw(const StreamConfiguration &cfg)\n> +bool isFormatRaw(const libcamera::PixelFormat &pixFmt)\n\nisFormatRaw -> isRaw + argument type change was done in v12. Umang's\nversion uses in one (different) place format that is not stored in the\nconfiguration.\n\n> {\n> - return libcamera::PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==\n> + return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==\n> libcamera::PixelFormatInfo::ColourEncodingRAW;\n> }\n> \n> @@ -372,11 +368,6 @@ private:\n> void ispStatsReady(uint32_t frame, uint32_t bufferId);\n> void metadataReady(uint32_t frame, const ControlList &metadata);\n> void setSensorControls(const ControlList &sensorControls);\n\nA significant difference between the two versions is the buffer\nhandling.\n\nMy version distinguishes between raw/non-raw cases at all the particular\nplaces:\n\n> -\n> - bool processedRequested() const\n> - {\n> - return streams_.size() - (rawStream_ ? 1 : 0) > 0;\n> - }\n> };\n> \n> class SimpleCameraConfiguration : public CameraConfiguration\n> @@ -884,13 +875,10 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> * point converting an erroneous buffer.\n> */\n> if (buffer->metadata().status != FrameMetadata::FrameSuccess) {\n> - if (rawStream_) {\n> + if (!useConversion_) {\n> /* No conversion, just complete the request. */\n> Request *request = buffer->request();\n> pipe->completeBuffer(request, buffer);\n> - SimpleFrameInfo *info = frameInfo_.find(request->sequence());\n> - if (info)\n> - info->metadataRequired = false;\n> tryCompleteRequest(request);\n> return;\n> }\n> @@ -900,8 +888,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> * buffer for capture (unless the stream is being stopped), and\n> * complete the request with all the user-facing buffers.\n> */\n> - if (buffer->metadata().status != FrameMetadata::FrameCancelled &&\n> - !rawStream_)\n> + if (buffer->metadata().status != FrameMetadata::FrameCancelled)\n> video_->queueBuffer(buffer);\n> \n> if (conversionQueue_.empty())\n> @@ -957,7 +944,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> \n> if (converter_)\n> converter_->queueBuffers(buffer, conversionQueue_.front().outputs);\n> - else if (processedRequested())\n> + else\n> /*\n> * request->sequence() cannot be retrieved from `buffer' inside\n> * queueBuffers because unique_ptr's make buffer->request() invalid\n> @@ -967,8 +954,6 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> conversionQueue_.front().outputs);\n> \n> conversionQueue_.pop();\n> - if (rawStream_)\n> - pipe->completeBuffer(request, buffer);\n> return;\n> }\n> \n> @@ -1006,8 +991,14 @@ void SimpleCameraData::tryCompleteRequest(Request *request)\n> void SimpleCameraData::conversionInputDone(FrameBuffer *buffer)\n> {\n> /* Queue the input buffer back for capture. */\n> - if (!rawStream_)\n> + if (!rawStream_) {\n> video_->queueBuffer(buffer);\n\nWhile Umang's version has this final buffer completion:\n\n> + } else {\n> + /* Complete the input buffer. */\n> + Request *request = buffer->request();\n> + if (pipe()->completeBuffer(request, buffer))\n> + tryCompleteRequest(request);\n> + }\n> }\n> \n> void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> @@ -1156,31 +1147,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> LOG(SimplePipeline, Debug)\n> << \"Largest stream size is \" << maxStreamSize;\n> \n> + /* Cap the number of raw stream configuration */\n> + unsigned int rawCount = 0;\n> + PixelFormat requestedRawFormat;\n> + for (const StreamConfiguration &cfg : config_) {\n> + if (!isFormatRaw(cfg.pixelFormat))\n> + continue;\n> + requestedRawFormat = cfg.pixelFormat;\n> + rawCount++;\n> + }\n> +\n> + if (rawCount > 1) {\n> + LOG(SimplePipeline, Error)\n> + << \"Camera configuration with \"\n> + << rawCount << \" raw streams not supported\";\n> + return Invalid;\n> + }\n> +\n\nThis differs in how to check for multiple raw streams in validate() and\nhow to select the config but doesn't seem to do something very\ndifferent.\n\n> /*\n> - * Find the best configuration for the pipeline using a heuristic. First\n> - * select the pixel format based on the streams. If there is a raw stream,\n> - * its format has precedence. If there is no raw stream, the streams are\n> - * considered ordered from highest to lowest priority. Default to the first\n> - * pipeline configuration if no streams request a supported pixel format.\n> + * Find the best configuration for the pipeline using a heuristic.\n> + * First select the pixel format based on the raw streams followed by\n> + * non-raw streams (which are considered ordered from highest to lowest\n> + * priority). Default to the first pipeline configuration if no streams\n> + * request a supported pixel format.\n> */\n> - std::optional rawFormat;\n> - for (const auto &cfg : config_)\n> - if (isRaw(cfg)) {\n> - if (rawFormat) {\n> - LOG(SimplePipeline, Error)\n> - << \"Can't capture multiple raw streams\";\n> - return Invalid;\n> - }\n> - rawFormat = cfg.pixelFormat;\n> - }\n> + const std::vector *configs =\n> + &data_->formats_.begin()->second;\n> \n> - const std::vector *configs = nullptr;\n> - if (rawFormat) {\n> - auto it = data_->formats_.find(rawFormat.value());\n> - if (it != data_->formats_.end())\n> - configs = &it->second;\n> - }\n> - if (!configs)\n> + auto rawIter = data_->formats_.find(requestedRawFormat);\n> + if (rawIter != data_->formats_.end()) {\n> + configs = &rawIter->second;\n> + } else {\n> for (const StreamConfiguration &cfg : config_) {\n> auto it = data_->formats_.find(cfg.pixelFormat);\n> if (it != data_->formats_.end()) {\n> @@ -1188,8 +1185,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> break;\n> }\n> }\n> - if (!configs)\n> - configs = &data_->formats_.begin()->second;\n> + }\n> \n> /*\n> * \\todo Pick the best sensor output media bus format when the\n> @@ -1242,63 +1238,39 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> * require any conversion, similar to raw capture use cases). This is\n> * left as a future improvement.\n> */\n> - needConversion_ = config_.size() > 1 + (rawFormat ? 1 : 0);\n> + needConversion_ = config_.size() > 1 + rawCount;\n> \n> for (unsigned int i = 0; i < config_.size(); ++i) {\n> StreamConfiguration &cfg = config_[i];\n> - const bool raw = isRaw(cfg);\n> + const bool raw = isFormatRaw(cfg.pixelFormat);\n> \n> - /* Adjust the pixel format, colour space and size. */\n> -\n> - PixelFormat pixelFormat;\n> + /* Adjust the raw pixel format and size. */\n> if (raw) {\n> - pixelFormat = pipeConfig_->captureFormat;\n> - } else {\n> - auto it = std::find(pipeConfig_->outputFormats.begin(),\n> - pipeConfig_->outputFormats.end(),\n> - cfg.pixelFormat);\n> - if (it == pipeConfig_->outputFormats.end())\n> - it = pipeConfig_->outputFormats.begin();\n> - pixelFormat = *it;\n> - }\n> - if (cfg.pixelFormat != pixelFormat) {\n> - LOG(SimplePipeline, Debug)\n> - << \"Adjusting pixel format of a \"\n> - << (raw ? \"raw\" : \"processed\")\n> - << \" stream from \" << cfg.pixelFormat\n> - << \" to \" << pixelFormat;\n> - cfg.pixelFormat = pixelFormat;\n> - status = Adjusted;\n> - }\n> + if (cfg.pixelFormat != pipeConfig_->captureFormat ||\n> + cfg.size != pipeConfig_->captureSize) {\n> + cfg.pixelFormat = pipeConfig_->captureFormat;\n> + cfg.size = pipeConfig_->captureSize;\n> \n> - /*\n> - * Best effort to fix the color space. If the color space is not set,\n> - * set it according to the pixel format, which may not be correct (pixel\n> - * formats and color spaces are different things, although somewhat\n> - * related) but we don't have a better option at the moment. Then in any\n> - * case, perform the standard pixel format based color space adjustment.\n> - */\n> - if (!cfg.colorSpace) {\n> - const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);\n> - switch (info.colourEncoding) {\n> - case PixelFormatInfo::ColourEncodingRGB:\n> - cfg.colorSpace = ColorSpace::Srgb;\n> - break;\n> - case libcamera::PixelFormatInfo::ColourEncodingYUV:\n> - cfg.colorSpace = ColorSpace::Sycc;\n> - break;\n> - default:\n> - cfg.colorSpace = ColorSpace::Raw;\n> + LOG(SimplePipeline, Debug)\n> + << \"Adjusting raw stream to \"\n> + << cfg.toString();\n> + status = Adjusted;\n> }\n> - LOG(SimplePipeline, Debug)\n> - << \"Unspecified color space set to \"\n> - << cfg.colorSpace.value().toString();\n> - status = Adjusted;\n> }\n> - if (cfg.colorSpace->adjust(pixelFormat)) {\n\nThe colour space adjustment is the primary difference in this diff area.\nIn Umang's version, it's still handled when processing stream roles and\nit's missing here in validate(). The rest is more similar than the size\nof the diff output might suggest.\n\n> +\n> + /* Adjust the non-raw pixel format and size. */\n> + auto it = std::find(pipeConfig_->outputFormats.begin(),\n> + pipeConfig_->outputFormats.end(),\n> + cfg.pixelFormat);\n> + if (it == pipeConfig_->outputFormats.end())\n> + it = pipeConfig_->outputFormats.begin();\n> +\n> + PixelFormat pixelFormat = *it;\n> + if (!raw && cfg.pixelFormat != pixelFormat) {\n> LOG(SimplePipeline, Debug)\n> - << \"Color space adjusted to \"\n> - << cfg.colorSpace.value().toString();\n> + << \"Adjusting pixel format from \"\n> + << cfg.pixelFormat << \" to \" << pixelFormat;\n> + cfg.pixelFormat = pixelFormat;\n> status = Adjusted;\n> }\n> \n> @@ -1321,20 +1293,11 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> \n> /* \\todo Create a libcamera core class to group format and size */\n> if (cfg.pixelFormat != pipeConfig_->captureFormat ||\n> - cfg.size != pipeConfig_->captureSize) {\n> - if (raw) {\n> - cfg.pixelFormat = pipeConfig_->captureFormat;\n> - cfg.size = pipeConfig_->captureSize;\n> - LOG(SimplePipeline, Debug)\n> - << \"Adjusting raw configuration to \" << cfg;\n> - status = Adjusted;\n> - } else {\n> - needConversion_ = true;\n> - }\n> - }\n> + cfg.size != pipeConfig_->captureSize)\n> + needConversion_ = true;\n> \n> /* Set the stride and frameSize. */\n> - if (needConversion_ && !raw) {\n> + if (needConversion_) {\n> std::tie(cfg.stride, cfg.frameSize) =\n> data_->converter_\n> ? data_->converter_->strideAndFrameSize(cfg.pixelFormat,\n> @@ -1379,7 +1342,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> \n> SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)\n> : PipelineHandler(manager, kMaxQueuedRequestsDevice),\n> - converter_(nullptr)\n> + converter_(nullptr),\n> + swIspEnabled_(false)\n> {\n> }\n> \n> @@ -1393,78 +1357,64 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span if (roles.empty())\n> return config;\n> \n> - bool processedRequested = false;\n> - bool rawRequested = false;\n> - for (const auto &role : roles)\n> - if (role == StreamRole::Raw) {\n> - if (rawRequested) {\n> - LOG(SimplePipeline, Error)\n> - << \"Can't capture multiple raw streams\";\n> - return nullptr;\n> - }\n> - rawRequested = true;\n> - } else {\n> - processedRequested = true;\n> - }\n\nIIRC Umang suggested that checking for this in validate() is enough and\nit's not necessary to have a sort of duplicate check in\ngenerateConfiguration().\n\nThe rest here is similar in both the versions in what it does.\n\n> -\n> - /* Create the formats maps. */\n> - std::map> processedFormats;\n> - std::map> rawFormats;\n> -\n> - for (const SimpleCameraData::Configuration &cfg : data->configs_) {\n> - rawFormats[cfg.captureFormat].push_back(cfg.captureSize);\n> - for (PixelFormat format : cfg.outputFormats)\n> - processedFormats[format].push_back(cfg.outputSizes);\n> - }\n> + /* Create the formats map. */\n> + std::map> formats;\n> \n> - if (processedRequested && processedFormats.empty()) {\n> - LOG(SimplePipeline, Error)\n> - << \"Processed stream requsted but no corresponding output configuration found\";\n> - return nullptr;\n> - }\n> - if (rawRequested && rawFormats.empty()) {\n> - LOG(SimplePipeline, Error)\n> - << \"Raw stream requsted but no corresponding output configuration found\";\n> - return nullptr;\n\nThese checks are not present in Umang's version.\n\n> + for (const auto &it : data->formats_) {\n> + for (const SimpleCameraData::Configuration *cfg : it.second)\n> + formats[it.first].push_back(cfg->outputSizes);\n> }\n> \n> - auto setUpFormatSizes = [](std::map> &formats) {\n> - /* Sort the sizes and merge any consecutive overlapping ranges. */\n> -\n> - for (auto &[format, sizes] : formats) {\n> - std::sort(sizes.begin(), sizes.end(),\n> - [](SizeRange &a, SizeRange &b) {\n> - return a.min < b.min;\n> - });\n> -\n> - auto cur = sizes.begin();\n> - auto next = cur;\n> -\n> - while (++next != sizes.end()) {\n> - if (cur->max.width >= next->min.width &&\n> - cur->max.height >= next->min.height)\n> - cur->max = next->max;\n> - else if (++cur != next)\n> - *cur = *next;\n> - }\n> -\n> - sizes.erase(++cur, sizes.end());\n> + /* Sort the sizes and merge any consecutive overlapping ranges. */\n> + for (auto &[format, sizes] : formats) {\n> + std::sort(sizes.begin(), sizes.end(),\n> + [](SizeRange &a, SizeRange &b) {\n> + return a.min < b.min;\n> + });\n> +\n> + auto cur = sizes.begin();\n> + auto next = cur;\n> +\n> + while (++next != sizes.end()) {\n> + if (cur->max.width >= next->min.width &&\n> + cur->max.height >= next->min.height)\n> + cur->max = next->max;\n> + else if (++cur != next)\n> + *cur = *next;\n> }\n> - };\n> - setUpFormatSizes(processedFormats);\n> - setUpFormatSizes(rawFormats);\n> +\n> + sizes.erase(++cur, sizes.end());\n> + }\n> \n> /*\n> * Create the stream configurations. Take the first entry in the formats\n> - * map as the default, for lack of a better option.\n> + * map as the default for each of role (raw or non-raw), for lack of a\n> + * better option.\n> *\n> * \\todo Implement a better way to pick the default format\n> */\n> for (StreamRole role : roles) {\n> - const auto &formats = (role == StreamRole::Raw ? rawFormats : processedFormats);\n> StreamConfiguration cfg{ StreamFormats{ formats } };\n> - cfg.pixelFormat = formats.begin()->first;\n> - cfg.size = formats.begin()->second[0].max;\n> +\n> + switch (role) {\n> + case StreamRole::Raw:\n> + for (auto &[format, sizes] : formats) {\n> + if (!isFormatRaw(format))\n> + continue;\n> + cfg.pixelFormat = format;\n> + cfg.size = sizes.back().max;\n> + cfg.colorSpace = ColorSpace::Raw;\n> + break;\n> + }\n> + break;\n> + default:\n> + for (auto &[format, sizes] : formats) {\n> + if (isFormatRaw(format))\n> + continue;\n> + cfg.pixelFormat = format;\n> + cfg.size = sizes[0].max;\n> + }\n> + }\n\nIn my version, colour space handling is now only in validate(). The\npixel format and size selection are similar in both the versions.\n\nThe rest of the diff are only cosmetic differences.\n\n> \n> config->addConfiguration(cfg);\n> }\n> @@ -1532,7 +1482,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> \n> for (unsigned int i = 0; i < config->size(); ++i) {\n> StreamConfiguration &cfg = config->at(i);\n> - bool rawStream = isRaw(cfg);\n> + bool rawStream = isFormatRaw(cfg.pixelFormat);\n> \n> cfg.setStream(&data->streams_[i]);\n> \n> @@ -1571,7 +1521,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> * Export buffers on the converter or capture video node, depending on\n> * whether the converter is used or not.\n> */\n> - if (data->useConversion_ && stream != data->rawStream_)\n> + if (data->useConversion_ && (stream != data->rawStream_))\n> return data->converter_\n> ? data->converter_->exportBuffers(stream, count, buffers)\n> : data->swIsp_->exportBuffers(stream, count, buffers);\n> @@ -1593,7 +1543,6 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> << pad->entity()->name() << \" in use\";\n> return -EBUSY;\n> }\n> -\n> if (data->useConversion_ && !data->rawStream_) {\n> /*\n> * When using the converter allocate a fixed number of internal\n> @@ -1603,10 +1552,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> &data->conversionBuffers_);\n> } else {\n> /*\n> - * Otherwise, prepare for using buffers from either the raw stream, if\n> - * requested, or the only stream configured.\n> + * Otherwise, prepare for using buffers from either the raw\n> + * stream((if requested) or the only stream configured.\n> */\n> - Stream *stream = (data->rawStream_ ? data->rawStream_ : &data->streams_[0]);\n> + Stream *stream;\n> + if (data->rawStream_)\n> + stream = data->rawStream_;\n> + else\n> + stream = &data->streams_[0];\n> +\n> ret = video->importBuffers(stream->configuration().bufferCount);\n> }\n> if (ret < 0) {\n> @@ -1647,9 +1601,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> }\n> \n> /* Queue all internal buffers for capture. */\n> - if (!data->rawStream_)\n> + if (!data->rawStream_) {\n> for (std::unique_ptr &buffer : data->conversionBuffers_)\n> video->queueBuffer(buffer.get());\n> + }\n> }\n> \n> return 0;\n> @@ -1700,7 +1655,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> * queue, it will be handed to the converter in the capture\n> * completion handler.\n> */\n> - if (data->useConversion_ && stream != data->rawStream_) {\n> + if (data->useConversion_ && (stream != data->rawStream_)) {\n> buffers.emplace(stream, buffer);\n> metadataRequired = !!data->swIsp_;\n> } else {\n> @@ -1836,13 +1791,13 @@ bool SimplePipelineHandler::matchDevice(MediaDevice *media,\n> /*\n> * When the software ISP is enabled, the simple pipeline handler\n> * exposes the raw stream, giving a total of two streams. This\n> - * is mutually exclusive with the presence of a converter.\n> + * is mutally exclusive with the presence of a converter.\n> */\n> ASSERT(!converter_);\n> numStreams = 2;\n> + swIspEnabled_ = true;\n> }\n> \n> - swIspEnabled_ = info.swIspEnabled;\n> const GlobalConfiguration &configuration = cameraManager()->_d()->configuration();\n> for (GlobalConfiguration::Configuration entry :\n> configuration.configuration()[\"pipelines\"][\"simple\"][\"supported_devices\"]","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 28D00C3259\n\tfor ;\n\tTue, 21 Oct 2025 18:36:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B9CA60793;\n\tTue, 21 Oct 2025 20:36:38 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79CD46078C\n\tfor ;\n\tTue, 21 Oct 2025 20:36:36 +0200 (CEST)","from mail-wr1-f71.google.com (mail-wr1-f71.google.com\n\t[209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-404-OIcaw7xzNre71cjBLhwehg-1; Tue, 21 Oct 2025 14:36:33 -0400","by mail-wr1-f71.google.com with SMTP id\n\tffacd0b85a97d-4270a273b6eso3241991f8f.0\n\tfor ;\n\tTue, 21 Oct 2025 11:36:33 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-427ea5a1056sm21390873f8f.2.2025.10.21.11.36.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 21 Oct 2025 11:36:30 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"D6GMuAO1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1761071795;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=i/oARbbI81BocT93Jpld2J09iXIJhpK1oXo/vg9ERbU=;\n\tb=D6GMuAO15AqAY8WNJBWT5GxMGO8V0XcvRvv6Z7+hbSSHft++TkUpwixiqevTg5luedu+9Q\n\tBR+f+oicMDTEVb3CPBNW4eNyJ/uiCrR4wS/FPBDrM45BbloSLacXyzNyaciCMSNxWZTfTQ\n\trd3TEUdVw1FNtujvV7GcsIVRjFkCviA=","X-MC-Unique":"OIcaw7xzNre71cjBLhwehg-1","X-Mimecast-MFC-AGG-ID":"OIcaw7xzNre71cjBLhwehg_1761071792","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1761071792; x=1761676592;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=i/oARbbI81BocT93Jpld2J09iXIJhpK1oXo/vg9ERbU=;\n\tb=ozccQyAlJCUuL12oNvULkHUm4bBtTfJUh77deWQQC39xikCk//Sb0IqQQv4tjwQE9L\n\tnl28wgfSzKRg3ALW/xaOE0k3JjsoXG0icfYzsU+D57Ww1id+qpkLWZA3Jc94olNZjtZm\n\txce7+Q1NLCl0XlW8KW7p0Rq2xGAZTDzDRpBU/z9e6trVQBScF4GYVSddYPoALOngdmdR\n\t9UmtjKyOlditaCnuovt3yBSl2wfK7MqkKbp/e87wep5nTzfj3lL/iWrnNxW+O8w5D+u6\n\tHom4pLXiGsmD048+milQ4TaMiiqDtVRtekMDq/zcZpcvF9NWitH4hFLSdidRDrC6eZ9P\n\txX+w==","X-Gm-Message-State":"AOJu0Yxs4VY7oJ+oNVzjMFySQT2kAncE9oKkQkKLIKUzvRu6nQ0GUb0B\n\tfcRIKz3GmCbcy8Q1WZaWr9pK2O8BayyrvKUBp+TOHftJz0PvhGoVc4zLHriyd0cKLabxfHEAN3y\n\tVDfEg3zvpxxnXL2+yZQi7gIhnA/+ecCut+cdGv727THO5W/mO2jOMuTDtpRP9q4QbARdL9M8jj+\n\tw=","X-Gm-Gg":"ASbGnctlVZ5ncaCJwiu3QY+p+n3yrKXNeMU1gcnAARIGjt+/VWwdm/XX0X/Cmk8sgM0\n\tf495EVJvv5hnNJzJ4t+EUJJd8/xv3kEqs3F+XCrYkUoaN7VtePR5LBlmCwD8tmr3OIErOPzD2kr\n\tANXjisn90OvdsK4vk7d/Gvs3OKabQheUFe97H3UJGgTHJa5C1dkcf7Yo0lh6HYgayI3cow7VDBM\n\tV7QVn/zv9Mmixzj18myf/3fdb9lO2S47ehlLga1t/q6DBig/CvMHDP0CR5y3fq4rZoHoqIJVWKu\n\t//2aAk+Lig8VVQwToHIScUwqgC3nyRRVPZGvF+Ud+mvo5wyR/K/NVs/GQcLFwKqERP1GNrjsp4J\n\tNgZFjIdB+kt17XPHtQCndB6EMWStT+AA5j/uKyvJYrcZx01EHx6zi","X-Received":["by 2002:a5d:5f47:0:b0:428:3ef4:9a0f with SMTP id\n\tffacd0b85a97d-4283ef49e24mr6594801f8f.6.1761071791973; \n\tTue, 21 Oct 2025 11:36:31 -0700 (PDT)","by 2002:a5d:5f47:0:b0:428:3ef4:9a0f with SMTP id\n\tffacd0b85a97d-4283ef49e24mr6594782f8f.6.1761071791419; \n\tTue, 21 Oct 2025 11:36:31 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHEclTnOAtwd7fFbbRUHEvk8964y+B1/vCm9rm5S9+X5PMyMpfs+s0MuBNj6eG5IGLkJ14V+Q==","From":"Milan Zamazal ","To":"libcamera-devel@lists.libcamera.org","Cc":"Laurent Pinchart ,\n\tKieran Bingham , =?utf-8?q?Barnab?=\n\t=?utf-8?b?w6FzIFDFkWN6ZQ==?=\n\t, Paul Elder\n\t, Umang Jain ","Subject":"Re: [PATCH v13 0/8] Enable raw streams with software ISP","In-Reply-To":"<20251021182716.29274-1-mzamazal@redhat.com> (Milan Zamazal's\n\tmessage of \"Tue, 21 Oct 2025 20:27:07 +0200\")","References":"<20251021182716.29274-1-mzamazal@redhat.com>","Date":"Tue, 21 Oct 2025 20:36:29 +0200","Message-ID":"<85sefcgl02.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"YW9Ut8EyFr6-EmGugFvfGMy9rDPCFDKzOeSAwDsJgiE_1761071792","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36387,"web_url":"https://patchwork.libcamera.org/comment/36387/","msgid":"<176113201126.199266.9310260576707132900@ping.linuxembedded.co.uk>","date":"2025-10-22T11:20:11","subject":"Re: [PATCH v13 0/8] Enable raw streams with software ISP","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-10-21 19:36:29)\n> There is another branch of this series by Umang. Since there was no\n> preference expressed which branch to use for the next version, I\n> continue with mine.\n> \n> While I tried to incorporate some changes from Umang's branch\n> previously, some differences remain, which I think require more\n> discussion. I rebased Umang's branch on the same master version, made a\n> diff between the two branches and commented on it below. The diff shows\n> an update of the posted patches to Umang's branch, i.e. the original\n> version in the diff is this v13, the new version is the rebased Umang's\n> branch.\n\nThanks,\n\nI had to do a few updates to camshark to test this series. The fixes are\navailable at:\n - https://gitlab.freedesktop.org/camera/camshark/-/merge_requests/14\n\nBut for this series at least - I can capture and view the raws.\n\nTested-by: Kieran Bingham \n\n\n> \n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 607b07949..529d7344d 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -11,7 +11,6 @@\n> > #include \n> > #include \n> > #include \n> > -#include \n> > #include \n> > #include \n> > #include \n> > @@ -26,9 +25,7 @@\n> > #include \n> > \n> > #include \n> > -#include \n> > #include \n> > -#include \n> > #include \n> > #include \n> > #include \n> > @@ -40,7 +37,6 @@\n> > #include \"libcamera/internal/converter.h\"\n> > #include \"libcamera/internal/delayed_controls.h\"\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > -#include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/global_configuration.h\"\n> > #include \"libcamera/internal/media_device.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> > @@ -267,9 +263,9 @@ static const SimplePipelineInfo supportedDevices[] = {\n> > { \"sun6i-csi\", {}, false },\n> > };\n> > \n> > -bool isRaw(const StreamConfiguration &cfg)\n> > +bool isFormatRaw(const libcamera::PixelFormat &pixFmt)\n> \n> isFormatRaw -> isRaw + argument type change was done in v12. Umang's\n> version uses in one (different) place format that is not stored in the\n> configuration.\n> \n> > {\n> > - return libcamera::PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==\n> > + return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==\n> > libcamera::PixelFormatInfo::ColourEncodingRAW;\n> > }\n> > \n> > @@ -372,11 +368,6 @@ private:\n> > void ispStatsReady(uint32_t frame, uint32_t bufferId);\n> > void metadataReady(uint32_t frame, const ControlList &metadata);\n> > void setSensorControls(const ControlList &sensorControls);\n> \n> A significant difference between the two versions is the buffer\n> handling.\n> \n> My version distinguishes between raw/non-raw cases at all the particular\n> places:\n> \n> > -\n> > - bool processedRequested() const\n> > - {\n> > - return streams_.size() - (rawStream_ ? 1 : 0) > 0;\n> > - }\n> > };\n> > \n> > class SimpleCameraConfiguration : public CameraConfiguration\n> > @@ -884,13 +875,10 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> > * point converting an erroneous buffer.\n> > */\n> > if (buffer->metadata().status != FrameMetadata::FrameSuccess) {\n> > - if (rawStream_) {\n> > + if (!useConversion_) {\n> > /* No conversion, just complete the request. */\n> > Request *request = buffer->request();\n> > pipe->completeBuffer(request, buffer);\n> > - SimpleFrameInfo *info = frameInfo_.find(request->sequence());\n> > - if (info)\n> > - info->metadataRequired = false;\n> > tryCompleteRequest(request);\n> > return;\n> > }\n> > @@ -900,8 +888,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> > * buffer for capture (unless the stream is being stopped), and\n> > * complete the request with all the user-facing buffers.\n> > */\n> > - if (buffer->metadata().status != FrameMetadata::FrameCancelled &&\n> > - !rawStream_)\n> > + if (buffer->metadata().status != FrameMetadata::FrameCancelled)\n> > video_->queueBuffer(buffer);\n> > \n> > if (conversionQueue_.empty())\n> > @@ -957,7 +944,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> > \n> > if (converter_)\n> > converter_->queueBuffers(buffer, conversionQueue_.front().outputs);\n> > - else if (processedRequested())\n> > + else\n> > /*\n> > * request->sequence() cannot be retrieved from `buffer' inside\n> > * queueBuffers because unique_ptr's make buffer->request() invalid\n> > @@ -967,8 +954,6 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> > conversionQueue_.front().outputs);\n> > \n> > conversionQueue_.pop();\n> > - if (rawStream_)\n> > - pipe->completeBuffer(request, buffer);\n> > return;\n> > }\n> > \n> > @@ -1006,8 +991,14 @@ void SimpleCameraData::tryCompleteRequest(Request *request)\n> > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer)\n> > {\n> > /* Queue the input buffer back for capture. */\n> > - if (!rawStream_)\n> > + if (!rawStream_) {\n> > video_->queueBuffer(buffer);\n> \n> While Umang's version has this final buffer completion:\n> \n> > + } else {\n> > + /* Complete the input buffer. */\n> > + Request *request = buffer->request();\n> > + if (pipe()->completeBuffer(request, buffer))\n> > + tryCompleteRequest(request);\n> > + }\n> > }\n> > \n> > void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> > @@ -1156,31 +1147,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > LOG(SimplePipeline, Debug)\n> > << \"Largest stream size is \" << maxStreamSize;\n> > \n> > + /* Cap the number of raw stream configuration */\n> > + unsigned int rawCount = 0;\n> > + PixelFormat requestedRawFormat;\n> > + for (const StreamConfiguration &cfg : config_) {\n> > + if (!isFormatRaw(cfg.pixelFormat))\n> > + continue;\n> > + requestedRawFormat = cfg.pixelFormat;\n> > + rawCount++;\n> > + }\n> > +\n> > + if (rawCount > 1) {\n> > + LOG(SimplePipeline, Error)\n> > + << \"Camera configuration with \"\n> > + << rawCount << \" raw streams not supported\";\n> > + return Invalid;\n> > + }\n> > +\n> \n> This differs in how to check for multiple raw streams in validate() and\n> how to select the config but doesn't seem to do something very\n> different.\n> \n> > /*\n> > - * Find the best configuration for the pipeline using a heuristic. First\n> > - * select the pixel format based on the streams. If there is a raw stream,\n> > - * its format has precedence. If there is no raw stream, the streams are\n> > - * considered ordered from highest to lowest priority. Default to the first\n> > - * pipeline configuration if no streams request a supported pixel format.\n> > + * Find the best configuration for the pipeline using a heuristic.\n> > + * First select the pixel format based on the raw streams followed by\n> > + * non-raw streams (which are considered ordered from highest to lowest\n> > + * priority). Default to the first pipeline configuration if no streams\n> > + * request a supported pixel format.\n> > */\n> > - std::optional rawFormat;\n> > - for (const auto &cfg : config_)\n> > - if (isRaw(cfg)) {\n> > - if (rawFormat) {\n> > - LOG(SimplePipeline, Error)\n> > - << \"Can't capture multiple raw streams\";\n> > - return Invalid;\n> > - }\n> > - rawFormat = cfg.pixelFormat;\n> > - }\n> > + const std::vector *configs =\n> > + &data_->formats_.begin()->second;\n> > \n> > - const std::vector *configs = nullptr;\n> > - if (rawFormat) {\n> > - auto it = data_->formats_.find(rawFormat.value());\n> > - if (it != data_->formats_.end())\n> > - configs = &it->second;\n> > - }\n> > - if (!configs)\n> > + auto rawIter = data_->formats_.find(requestedRawFormat);\n> > + if (rawIter != data_->formats_.end()) {\n> > + configs = &rawIter->second;\n> > + } else {\n> > for (const StreamConfiguration &cfg : config_) {\n> > auto it = data_->formats_.find(cfg.pixelFormat);\n> > if (it != data_->formats_.end()) {\n> > @@ -1188,8 +1185,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > break;\n> > }\n> > }\n> > - if (!configs)\n> > - configs = &data_->formats_.begin()->second;\n> > + }\n> > \n> > /*\n> > * \\todo Pick the best sensor output media bus format when the\n> > @@ -1242,63 +1238,39 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > * require any conversion, similar to raw capture use cases). This is\n> > * left as a future improvement.\n> > */\n> > - needConversion_ = config_.size() > 1 + (rawFormat ? 1 : 0);\n> > + needConversion_ = config_.size() > 1 + rawCount;\n> > \n> > for (unsigned int i = 0; i < config_.size(); ++i) {\n> > StreamConfiguration &cfg = config_[i];\n> > - const bool raw = isRaw(cfg);\n> > + const bool raw = isFormatRaw(cfg.pixelFormat);\n> > \n> > - /* Adjust the pixel format, colour space and size. */\n> > -\n> > - PixelFormat pixelFormat;\n> > + /* Adjust the raw pixel format and size. */\n> > if (raw) {\n> > - pixelFormat = pipeConfig_->captureFormat;\n> > - } else {\n> > - auto it = std::find(pipeConfig_->outputFormats.begin(),\n> > - pipeConfig_->outputFormats.end(),\n> > - cfg.pixelFormat);\n> > - if (it == pipeConfig_->outputFormats.end())\n> > - it = pipeConfig_->outputFormats.begin();\n> > - pixelFormat = *it;\n> > - }\n> > - if (cfg.pixelFormat != pixelFormat) {\n> > - LOG(SimplePipeline, Debug)\n> > - << \"Adjusting pixel format of a \"\n> > - << (raw ? \"raw\" : \"processed\")\n> > - << \" stream from \" << cfg.pixelFormat\n> > - << \" to \" << pixelFormat;\n> > - cfg.pixelFormat = pixelFormat;\n> > - status = Adjusted;\n> > - }\n> > + if (cfg.pixelFormat != pipeConfig_->captureFormat ||\n> > + cfg.size != pipeConfig_->captureSize) {\n> > + cfg.pixelFormat = pipeConfig_->captureFormat;\n> > + cfg.size = pipeConfig_->captureSize;\n> > \n> > - /*\n> > - * Best effort to fix the color space. If the color space is not set,\n> > - * set it according to the pixel format, which may not be correct (pixel\n> > - * formats and color spaces are different things, although somewhat\n> > - * related) but we don't have a better option at the moment. Then in any\n> > - * case, perform the standard pixel format based color space adjustment.\n> > - */\n> > - if (!cfg.colorSpace) {\n> > - const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);\n> > - switch (info.colourEncoding) {\n> > - case PixelFormatInfo::ColourEncodingRGB:\n> > - cfg.colorSpace = ColorSpace::Srgb;\n> > - break;\n> > - case libcamera::PixelFormatInfo::ColourEncodingYUV:\n> > - cfg.colorSpace = ColorSpace::Sycc;\n> > - break;\n> > - default:\n> > - cfg.colorSpace = ColorSpace::Raw;\n> > + LOG(SimplePipeline, Debug)\n> > + << \"Adjusting raw stream to \"\n> > + << cfg.toString();\n> > + status = Adjusted;\n> > }\n> > - LOG(SimplePipeline, Debug)\n> > - << \"Unspecified color space set to \"\n> > - << cfg.colorSpace.value().toString();\n> > - status = Adjusted;\n> > }\n> > - if (cfg.colorSpace->adjust(pixelFormat)) {\n> \n> The colour space adjustment is the primary difference in this diff area.\n> In Umang's version, it's still handled when processing stream roles and\n> it's missing here in validate(). The rest is more similar than the size\n> of the diff output might suggest.\n> \n> > +\n> > + /* Adjust the non-raw pixel format and size. */\n> > + auto it = std::find(pipeConfig_->outputFormats.begin(),\n> > + pipeConfig_->outputFormats.end(),\n> > + cfg.pixelFormat);\n> > + if (it == pipeConfig_->outputFormats.end())\n> > + it = pipeConfig_->outputFormats.begin();\n> > +\n> > + PixelFormat pixelFormat = *it;\n> > + if (!raw && cfg.pixelFormat != pixelFormat) {\n> > LOG(SimplePipeline, Debug)\n> > - << \"Color space adjusted to \"\n> > - << cfg.colorSpace.value().toString();\n> > + << \"Adjusting pixel format from \"\n> > + << cfg.pixelFormat << \" to \" << pixelFormat;\n> > + cfg.pixelFormat = pixelFormat;\n> > status = Adjusted;\n> > }\n> > \n> > @@ -1321,20 +1293,11 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > \n> > /* \\todo Create a libcamera core class to group format and size */\n> > if (cfg.pixelFormat != pipeConfig_->captureFormat ||\n> > - cfg.size != pipeConfig_->captureSize) {\n> > - if (raw) {\n> > - cfg.pixelFormat = pipeConfig_->captureFormat;\n> > - cfg.size = pipeConfig_->captureSize;\n> > - LOG(SimplePipeline, Debug)\n> > - << \"Adjusting raw configuration to \" << cfg;\n> > - status = Adjusted;\n> > - } else {\n> > - needConversion_ = true;\n> > - }\n> > - }\n> > + cfg.size != pipeConfig_->captureSize)\n> > + needConversion_ = true;\n> > \n> > /* Set the stride and frameSize. */\n> > - if (needConversion_ && !raw) {\n> > + if (needConversion_) {\n> > std::tie(cfg.stride, cfg.frameSize) =\n> > data_->converter_\n> > ? data_->converter_->strideAndFrameSize(cfg.pixelFormat,\n> > @@ -1379,7 +1342,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > \n> > SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)\n> > : PipelineHandler(manager, kMaxQueuedRequestsDevice),\n> > - converter_(nullptr)\n> > + converter_(nullptr),\n> > + swIspEnabled_(false)\n> > {\n> > }\n> > \n> > @@ -1393,78 +1357,64 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span > if (roles.empty())\n> > return config;\n> > \n> > - bool processedRequested = false;\n> > - bool rawRequested = false;\n> > - for (const auto &role : roles)\n> > - if (role == StreamRole::Raw) {\n> > - if (rawRequested) {\n> > - LOG(SimplePipeline, Error)\n> > - << \"Can't capture multiple raw streams\";\n> > - return nullptr;\n> > - }\n> > - rawRequested = true;\n> > - } else {\n> > - processedRequested = true;\n> > - }\n> \n> IIRC Umang suggested that checking for this in validate() is enough and\n> it's not necessary to have a sort of duplicate check in\n> generateConfiguration().\n> \n> The rest here is similar in both the versions in what it does.\n> \n> > -\n> > - /* Create the formats maps. */\n> > - std::map> processedFormats;\n> > - std::map> rawFormats;\n> > -\n> > - for (const SimpleCameraData::Configuration &cfg : data->configs_) {\n> > - rawFormats[cfg.captureFormat].push_back(cfg.captureSize);\n> > - for (PixelFormat format : cfg.outputFormats)\n> > - processedFormats[format].push_back(cfg.outputSizes);\n> > - }\n> > + /* Create the formats map. */\n> > + std::map> formats;\n> > \n> > - if (processedRequested && processedFormats.empty()) {\n> > - LOG(SimplePipeline, Error)\n> > - << \"Processed stream requsted but no corresponding output configuration found\";\n> > - return nullptr;\n> > - }\n> > - if (rawRequested && rawFormats.empty()) {\n> > - LOG(SimplePipeline, Error)\n> > - << \"Raw stream requsted but no corresponding output configuration found\";\n> > - return nullptr;\n> \n> These checks are not present in Umang's version.\n> \n> > + for (const auto &it : data->formats_) {\n> > + for (const SimpleCameraData::Configuration *cfg : it.second)\n> > + formats[it.first].push_back(cfg->outputSizes);\n> > }\n> > \n> > - auto setUpFormatSizes = [](std::map> &formats) {\n> > - /* Sort the sizes and merge any consecutive overlapping ranges. */\n> > -\n> > - for (auto &[format, sizes] : formats) {\n> > - std::sort(sizes.begin(), sizes.end(),\n> > - [](SizeRange &a, SizeRange &b) {\n> > - return a.min < b.min;\n> > - });\n> > -\n> > - auto cur = sizes.begin();\n> > - auto next = cur;\n> > -\n> > - while (++next != sizes.end()) {\n> > - if (cur->max.width >= next->min.width &&\n> > - cur->max.height >= next->min.height)\n> > - cur->max = next->max;\n> > - else if (++cur != next)\n> > - *cur = *next;\n> > - }\n> > -\n> > - sizes.erase(++cur, sizes.end());\n> > + /* Sort the sizes and merge any consecutive overlapping ranges. */\n> > + for (auto &[format, sizes] : formats) {\n> > + std::sort(sizes.begin(), sizes.end(),\n> > + [](SizeRange &a, SizeRange &b) {\n> > + return a.min < b.min;\n> > + });\n> > +\n> > + auto cur = sizes.begin();\n> > + auto next = cur;\n> > +\n> > + while (++next != sizes.end()) {\n> > + if (cur->max.width >= next->min.width &&\n> > + cur->max.height >= next->min.height)\n> > + cur->max = next->max;\n> > + else if (++cur != next)\n> > + *cur = *next;\n> > }\n> > - };\n> > - setUpFormatSizes(processedFormats);\n> > - setUpFormatSizes(rawFormats);\n> > +\n> > + sizes.erase(++cur, sizes.end());\n> > + }\n> > \n> > /*\n> > * Create the stream configurations. Take the first entry in the formats\n> > - * map as the default, for lack of a better option.\n> > + * map as the default for each of role (raw or non-raw), for lack of a\n> > + * better option.\n> > *\n> > * \\todo Implement a better way to pick the default format\n> > */\n> > for (StreamRole role : roles) {\n> > - const auto &formats = (role == StreamRole::Raw ? rawFormats : processedFormats);\n> > StreamConfiguration cfg{ StreamFormats{ formats } };\n> > - cfg.pixelFormat = formats.begin()->first;\n> > - cfg.size = formats.begin()->second[0].max;\n> > +\n> > + switch (role) {\n> > + case StreamRole::Raw:\n> > + for (auto &[format, sizes] : formats) {\n> > + if (!isFormatRaw(format))\n> > + continue;\n> > + cfg.pixelFormat = format;\n> > + cfg.size = sizes.back().max;\n> > + cfg.colorSpace = ColorSpace::Raw;\n> > + break;\n> > + }\n> > + break;\n> > + default:\n> > + for (auto &[format, sizes] : formats) {\n> > + if (isFormatRaw(format))\n> > + continue;\n> > + cfg.pixelFormat = format;\n> > + cfg.size = sizes[0].max;\n> > + }\n> > + }\n> \n> In my version, colour space handling is now only in validate(). The\n> pixel format and size selection are similar in both the versions.\n> \n> The rest of the diff are only cosmetic differences.\n> \n> > \n> > config->addConfiguration(cfg);\n> > }\n> > @@ -1532,7 +1482,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > \n> > for (unsigned int i = 0; i < config->size(); ++i) {\n> > StreamConfiguration &cfg = config->at(i);\n> > - bool rawStream = isRaw(cfg);\n> > + bool rawStream = isFormatRaw(cfg.pixelFormat);\n> > \n> > cfg.setStream(&data->streams_[i]);\n> > \n> > @@ -1571,7 +1521,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> > * Export buffers on the converter or capture video node, depending on\n> > * whether the converter is used or not.\n> > */\n> > - if (data->useConversion_ && stream != data->rawStream_)\n> > + if (data->useConversion_ && (stream != data->rawStream_))\n> > return data->converter_\n> > ? data->converter_->exportBuffers(stream, count, buffers)\n> > : data->swIsp_->exportBuffers(stream, count, buffers);\n> > @@ -1593,7 +1543,6 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > << pad->entity()->name() << \" in use\";\n> > return -EBUSY;\n> > }\n> > -\n> > if (data->useConversion_ && !data->rawStream_) {\n> > /*\n> > * When using the converter allocate a fixed number of internal\n> > @@ -1603,10 +1552,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > &data->conversionBuffers_);\n> > } else {\n> > /*\n> > - * Otherwise, prepare for using buffers from either the raw stream, if\n> > - * requested, or the only stream configured.\n> > + * Otherwise, prepare for using buffers from either the raw\n> > + * stream((if requested) or the only stream configured.\n> > */\n> > - Stream *stream = (data->rawStream_ ? data->rawStream_ : &data->streams_[0]);\n> > + Stream *stream;\n> > + if (data->rawStream_)\n> > + stream = data->rawStream_;\n> > + else\n> > + stream = &data->streams_[0];\n> > +\n> > ret = video->importBuffers(stream->configuration().bufferCount);\n> > }\n> > if (ret < 0) {\n> > @@ -1647,9 +1601,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > }\n> > \n> > /* Queue all internal buffers for capture. */\n> > - if (!data->rawStream_)\n> > + if (!data->rawStream_) {\n> > for (std::unique_ptr &buffer : data->conversionBuffers_)\n> > video->queueBuffer(buffer.get());\n> > + }\n> > }\n> > \n> > return 0;\n> > @@ -1700,7 +1655,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> > * queue, it will be handed to the converter in the capture\n> > * completion handler.\n> > */\n> > - if (data->useConversion_ && stream != data->rawStream_) {\n> > + if (data->useConversion_ && (stream != data->rawStream_)) {\n> > buffers.emplace(stream, buffer);\n> > metadataRequired = !!data->swIsp_;\n> > } else {\n> > @@ -1836,13 +1791,13 @@ bool SimplePipelineHandler::matchDevice(MediaDevice *media,\n> > /*\n> > * When the software ISP is enabled, the simple pipeline handler\n> > * exposes the raw stream, giving a total of two streams. This\n> > - * is mutually exclusive with the presence of a converter.\n> > + * is mutally exclusive with the presence of a converter.\n> > */\n> > ASSERT(!converter_);\n> > numStreams = 2;\n> > + swIspEnabled_ = true;\n> > }\n> > \n> > - swIspEnabled_ = info.swIspEnabled;\n> > const GlobalConfiguration &configuration = cameraManager()->_d()->configuration();\n> > for (GlobalConfiguration::Configuration entry :\n> > configuration.configuration()[\"pipelines\"][\"simple\"][\"supported_devices\"]\n>","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 EF434BE080\n\tfor ;\n\tWed, 22 Oct 2025 11:20:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1F861606D2;\n\tWed, 22 Oct 2025 13:20:17 +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 E7C41606B0\n\tfor ;\n\tWed, 22 Oct 2025 13:20:14 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1962B605;\n\tWed, 22 Oct 2025 13:18:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wQFypNxb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761131910;\n\tbh=UKVp7ZyS2n7JWQEqjtk8DLlTpI5RsN87DvQ+yG85xTw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=wQFypNxbisQdHHNXZFuLNstdJFzlkqYLN3YUD2/ImxrIlDeXPVChjAZVlEraecNeH\n\ti7LXSusgElVRKWq2nCoXrrgB9Bfu+7N7RTUv0Z9IReH/obMIO4mXDVR0iVhqfL5DYI\n\tE5m5TGwJdmSuYwyyVsMI+Vz+SQC1pwiC9TcPsgh0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<85sefcgl02.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","References":"<20251021182716.29274-1-mzamazal@redhat.com>\n\t<85sefcgl02.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Subject":"Re: [PATCH v13 0/8] Enable raw streams with software ISP","From":"Kieran Bingham ","Cc":"Laurent Pinchart , =?utf-8?q?Barnab?=\n\t=?utf-8?b?w6FzIFDFkWN6ZQ==?= ,\n\tPaul Elder , Umang Jain ","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Wed, 22 Oct 2025 12:20:11 +0100","Message-ID":"<176113201126.199266.9310260576707132900@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36388,"web_url":"https://patchwork.libcamera.org/comment/36388/","msgid":"<5kjtvmsrtwjxuffle7u4yxj3wkwqsr7d5hejt6icufkr765nm3@daxvltodcrny>","date":"2025-10-22T11:54:25","subject":"Re: [PATCH v13 0/8] Enable raw streams with software ISP","submitter":{"id":232,"url":"https://patchwork.libcamera.org/api/people/232/","name":"Umang Jain","email":"uajain@igalia.com"},"content":"Hi Milan,\n\nThanks for the setting up the diff.\n\nOn Tue, Oct 21, 2025 at 08:36:29PM +0200, Milan Zamazal wrote:\n> There is another branch of this series by Umang. Since there was no\n> preference expressed which branch to use for the next version, I\n> continue with mine.\n> \n> While I tried to incorporate some changes from Umang's branch\n> previously, some differences remain, which I think require more\n> discussion. I rebased Umang's branch on the same master version, made a\n> diff between the two branches and commented on it below. The diff shows\n> an update of the posted patches to Umang's branch, i.e. the original\n> version in the diff is this v13, the new version is the rebased Umang's\n> branch.\n> \n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 607b07949..529d7344d 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -11,7 +11,6 @@\n> > #include \n> > #include \n> > #include \n> > -#include \n> > #include \n> > #include \n> > #include \n> > @@ -26,9 +25,7 @@\n> > #include \n> > \n> > #include \n> > -#include \n> > #include \n> > -#include \n> > #include \n> > #include \n> > #include \n> > @@ -40,7 +37,6 @@\n> > #include \"libcamera/internal/converter.h\"\n> > #include \"libcamera/internal/delayed_controls.h\"\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > -#include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/global_configuration.h\"\n> > #include \"libcamera/internal/media_device.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> > @@ -267,9 +263,9 @@ static const SimplePipelineInfo supportedDevices[] = {\n> > { \"sun6i-csi\", {}, false },\n> > };\n> > \n> > -bool isRaw(const StreamConfiguration &cfg)\n> > +bool isFormatRaw(const libcamera::PixelFormat &pixFmt)\n> \n> isFormatRaw -> isRaw + argument type change was done in v12. Umang's\n> version uses in one (different) place format that is not stored in the\n> configuration.\n> \n> > {\n> > - return libcamera::PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==\n> > + return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==\n> > libcamera::PixelFormatInfo::ColourEncodingRAW;\n> > }\n> > \n> > @@ -372,11 +368,6 @@ private:\n> > void ispStatsReady(uint32_t frame, uint32_t bufferId);\n> > void metadataReady(uint32_t frame, const ControlList &metadata);\n> > void setSensorControls(const ControlList &sensorControls);\n> \n> A significant difference between the two versions is the buffer\n> handling.\n> \n> My version distinguishes between raw/non-raw cases at all the particular\n> places:\n\nIMO, tracking the raw stream if requested in data->rawStream_ is\nsufficent. I didn't find any need to split raw/non-raw cases \nas user testing with cam. Did you find any issues with that in testing\nthe cases? I would also make a list of various cam user testing commands\nand test both implementation against that. I did that last time, but it\nwas less formal.\n\nIf there are no behaviour change in terms of user testing, maybe we\nneed a tie-breaker ? \n\n> \n> > -\n> > - bool processedRequested() const\n> > - {\n> > - return streams_.size() - (rawStream_ ? 1 : 0) > 0;\n> > - }\n> > };\n> > \n> > class SimpleCameraConfiguration : public CameraConfiguration\n> > @@ -884,13 +875,10 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> > * point converting an erroneous buffer.\n> > */\n> > if (buffer->metadata().status != FrameMetadata::FrameSuccess) {\n> > - if (rawStream_) {\n> > + if (!useConversion_) {\n> > /* No conversion, just complete the request. */\n> > Request *request = buffer->request();\n> > pipe->completeBuffer(request, buffer);\n> > - SimpleFrameInfo *info = frameInfo_.find(request->sequence());\n> > - if (info)\n> > - info->metadataRequired = false;\n> > tryCompleteRequest(request);\n> > return;\n> > }\n> > @@ -900,8 +888,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> > * buffer for capture (unless the stream is being stopped), and\n> > * complete the request with all the user-facing buffers.\n> > */\n> > - if (buffer->metadata().status != FrameMetadata::FrameCancelled &&\n> > - !rawStream_)\n> > + if (buffer->metadata().status != FrameMetadata::FrameCancelled)\n> > video_->queueBuffer(buffer);\n> > \n> > if (conversionQueue_.empty())\n> > @@ -957,7 +944,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> > \n> > if (converter_)\n> > converter_->queueBuffers(buffer, conversionQueue_.front().outputs);\n> > - else if (processedRequested())\n> > + else\n> > /*\n> > * request->sequence() cannot be retrieved from `buffer' inside\n> > * queueBuffers because unique_ptr's make buffer->request() invalid\n> > @@ -967,8 +954,6 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> > conversionQueue_.front().outputs);\n> > \n> > conversionQueue_.pop();\n> > - if (rawStream_)\n> > - pipe->completeBuffer(request, buffer);\n> > return;\n> > }\n> > \n> > @@ -1006,8 +991,14 @@ void SimpleCameraData::tryCompleteRequest(Request *request)\n> > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer)\n> > {\n> > /* Queue the input buffer back for capture. */\n> > - if (!rawStream_)\n> > + if (!rawStream_) {\n> > video_->queueBuffer(buffer);\n> \n> While Umang's version has this final buffer completion:\n> \n\nYes, it's simple, if rawStream_ is requested, you complete the\nbuffer/request (no further processing required) in the else block.\nOtherwise, you queue for further processing (equivalent to your\nprocessedRequests)\n\n\n> > + } else {\n> > + /* Complete the input buffer. */\n> > + Request *request = buffer->request();\n> > + if (pipe()->completeBuffer(request, buffer))\n> > + tryCompleteRequest(request);\n> > + }\n> > }\n> > \n> > void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> > @@ -1156,31 +1147,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > LOG(SimplePipeline, Debug)\n> > << \"Largest stream size is \" << maxStreamSize;\n> > \n> > + /* Cap the number of raw stream configuration */\n> > + unsigned int rawCount = 0;\n> > + PixelFormat requestedRawFormat;\n> > + for (const StreamConfiguration &cfg : config_) {\n> > + if (!isFormatRaw(cfg.pixelFormat))\n> > + continue;\n> > + requestedRawFormat = cfg.pixelFormat;\n> > + rawCount++;\n> > + }\n> > +\n> > + if (rawCount > 1) {\n> > + LOG(SimplePipeline, Error)\n> > + << \"Camera configuration with \"\n> > + << rawCount << \" raw streams not supported\";\n> > + return Invalid;\n> > + }\n> > +\n> \n> This differs in how to check for multiple raw streams in validate() and\n> how to select the config but doesn't seem to do something very\n> different.\n\nack, but this mostly aligns how other pipeline handlers checks as well.\n\n> \n> > /*\n> > - * Find the best configuration for the pipeline using a heuristic. First\n> > - * select the pixel format based on the streams. If there is a raw stream,\n> > - * its format has precedence. If there is no raw stream, the streams are\n> > - * considered ordered from highest to lowest priority. Default to the first\n> > - * pipeline configuration if no streams request a supported pixel format.\n> > + * Find the best configuration for the pipeline using a heuristic.\n> > + * First select the pixel format based on the raw streams followed by\n> > + * non-raw streams (which are considered ordered from highest to lowest\n> > + * priority). Default to the first pipeline configuration if no streams\n> > + * request a supported pixel format.\n> > */\n> > - std::optional rawFormat;\n> > - for (const auto &cfg : config_)\n> > - if (isRaw(cfg)) {\n> > - if (rawFormat) {\n> > - LOG(SimplePipeline, Error)\n> > - << \"Can't capture multiple raw streams\";\n> > - return Invalid;\n> > - }\n> > - rawFormat = cfg.pixelFormat;\n> > - }\n> > + const std::vector *configs =\n> > + &data_->formats_.begin()->second;\n> > \n> > - const std::vector *configs = nullptr;\n> > - if (rawFormat) {\n> > - auto it = data_->formats_.find(rawFormat.value());\n> > - if (it != data_->formats_.end())\n> > - configs = &it->second;\n> > - }\n> > - if (!configs)\n> > + auto rawIter = data_->formats_.find(requestedRawFormat);\n> > + if (rawIter != data_->formats_.end()) {\n> > + configs = &rawIter->second;\n> > + } else {\n> > for (const StreamConfiguration &cfg : config_) {\n> > auto it = data_->formats_.find(cfg.pixelFormat);\n> > if (it != data_->formats_.end()) {\n> > @@ -1188,8 +1185,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > break;\n> > }\n> > }\n> > - if (!configs)\n> > - configs = &data_->formats_.begin()->second;\n> > + }\n> > \n> > /*\n> > * \\todo Pick the best sensor output media bus format when the\n> > @@ -1242,63 +1238,39 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > * require any conversion, similar to raw capture use cases). This is\n> > * left as a future improvement.\n> > */\n> > - needConversion_ = config_.size() > 1 + (rawFormat ? 1 : 0);\n> > + needConversion_ = config_.size() > 1 + rawCount;\n> > \n> > for (unsigned int i = 0; i < config_.size(); ++i) {\n> > StreamConfiguration &cfg = config_[i];\n> > - const bool raw = isRaw(cfg);\n> > + const bool raw = isFormatRaw(cfg.pixelFormat);\n> > \n> > - /* Adjust the pixel format, colour space and size. */\n> > -\n> > - PixelFormat pixelFormat;\n> > + /* Adjust the raw pixel format and size. */\n> > if (raw) {\n> > - pixelFormat = pipeConfig_->captureFormat;\n> > - } else {\n> > - auto it = std::find(pipeConfig_->outputFormats.begin(),\n> > - pipeConfig_->outputFormats.end(),\n> > - cfg.pixelFormat);\n> > - if (it == pipeConfig_->outputFormats.end())\n> > - it = pipeConfig_->outputFormats.begin();\n> > - pixelFormat = *it;\n> > - }\n> > - if (cfg.pixelFormat != pixelFormat) {\n> > - LOG(SimplePipeline, Debug)\n> > - << \"Adjusting pixel format of a \"\n> > - << (raw ? \"raw\" : \"processed\")\n> > - << \" stream from \" << cfg.pixelFormat\n> > - << \" to \" << pixelFormat;\n> > - cfg.pixelFormat = pixelFormat;\n> > - status = Adjusted;\n> > - }\n> > + if (cfg.pixelFormat != pipeConfig_->captureFormat ||\n> > + cfg.size != pipeConfig_->captureSize) {\n> > + cfg.pixelFormat = pipeConfig_->captureFormat;\n> > + cfg.size = pipeConfig_->captureSize;\n> > \n> > - /*\n> > - * Best effort to fix the color space. If the color space is not set,\n> > - * set it according to the pixel format, which may not be correct (pixel\n> > - * formats and color spaces are different things, although somewhat\n> > - * related) but we don't have a better option at the moment. Then in any\n> > - * case, perform the standard pixel format based color space adjustment.\n> > - */\n> > - if (!cfg.colorSpace) {\n> > - const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);\n> > - switch (info.colourEncoding) {\n> > - case PixelFormatInfo::ColourEncodingRGB:\n> > - cfg.colorSpace = ColorSpace::Srgb;\n> > - break;\n> > - case libcamera::PixelFormatInfo::ColourEncodingYUV:\n> > - cfg.colorSpace = ColorSpace::Sycc;\n> > - break;\n> > - default:\n> > - cfg.colorSpace = ColorSpace::Raw;\n> > + LOG(SimplePipeline, Debug)\n> > + << \"Adjusting raw stream to \"\n> > + << cfg.toString();\n> > + status = Adjusted;\n> > }\n> > - LOG(SimplePipeline, Debug)\n> > - << \"Unspecified color space set to \"\n> > - << cfg.colorSpace.value().toString();\n> > - status = Adjusted;\n> > }\n> > - if (cfg.colorSpace->adjust(pixelFormat)) {\n> \n> The colour space adjustment is the primary difference in this diff area.\n> In Umang's version, it's still handled when processing stream roles and\n> it's missing here in validate(). The rest is more similar than the size\n> of the diff output might suggest.\n\nI remember that I commented that colorspace changes could be separated\nout from your series. My series only sets colorSpace::RAW to raw streams\nAFAIR.\n\n> \n> > +\n> > + /* Adjust the non-raw pixel format and size. */\n> > + auto it = std::find(pipeConfig_->outputFormats.begin(),\n> > + pipeConfig_->outputFormats.end(),\n> > + cfg.pixelFormat);\n> > + if (it == pipeConfig_->outputFormats.end())\n> > + it = pipeConfig_->outputFormats.begin();\n> > +\n> > + PixelFormat pixelFormat = *it;\n> > + if (!raw && cfg.pixelFormat != pixelFormat) {\n> > LOG(SimplePipeline, Debug)\n> > - << \"Color space adjusted to \"\n> > - << cfg.colorSpace.value().toString();\n> > + << \"Adjusting pixel format from \"\n> > + << cfg.pixelFormat << \" to \" << pixelFormat;\n> > + cfg.pixelFormat = pixelFormat;\n> > status = Adjusted;\n> > }\n> > \n> > @@ -1321,20 +1293,11 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > \n> > /* \\todo Create a libcamera core class to group format and size */\n> > if (cfg.pixelFormat != pipeConfig_->captureFormat ||\n> > - cfg.size != pipeConfig_->captureSize) {\n> > - if (raw) {\n> > - cfg.pixelFormat = pipeConfig_->captureFormat;\n> > - cfg.size = pipeConfig_->captureSize;\n> > - LOG(SimplePipeline, Debug)\n> > - << \"Adjusting raw configuration to \" << cfg;\n> > - status = Adjusted;\n> > - } else {\n> > - needConversion_ = true;\n> > - }\n> > - }\n> > + cfg.size != pipeConfig_->captureSize)\n> > + needConversion_ = true;\n> > \n> > /* Set the stride and frameSize. */\n> > - if (needConversion_ && !raw) {\n> > + if (needConversion_) {\n> > std::tie(cfg.stride, cfg.frameSize) =\n> > data_->converter_\n> > ? data_->converter_->strideAndFrameSize(cfg.pixelFormat,\n> > @@ -1379,7 +1342,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > \n> > SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)\n> > : PipelineHandler(manager, kMaxQueuedRequestsDevice),\n> > - converter_(nullptr)\n> > + converter_(nullptr),\n> > + swIspEnabled_(false)\n> > {\n> > }\n> > \n> > @@ -1393,78 +1357,64 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span > if (roles.empty())\n> > return config;\n> > \n> > - bool processedRequested = false;\n> > - bool rawRequested = false;\n> > - for (const auto &role : roles)\n> > - if (role == StreamRole::Raw) {\n> > - if (rawRequested) {\n> > - LOG(SimplePipeline, Error)\n> > - << \"Can't capture multiple raw streams\";\n> > - return nullptr;\n> > - }\n> > - rawRequested = true;\n> > - } else {\n> > - processedRequested = true;\n> > - }\n> \n> IIRC Umang suggested that checking for this in validate() is enough and\n> it's not necessary to have a sort of duplicate check in\n> generateConfiguration().\n\nThe problem with validate() behaviour is that it's two fold.\n1) for validation as per docs\n2) for completing the rest of configuration in generateConfiguration.\n\nSometime ago, I tried to ensure that the config->validate() return\nvalue should be checked - but then Laurent told me about 2). Hence I think to\nduplicate the check there hence I agree with this (since,\ngenerateConfiguration() calls validate() - but for 2) not 1).\n\nThis should be changed ideally - but I haven't thought it through.\n\nThere was some discussion here:\nhttps://patchwork.libcamera.org/patch/23806/#34899\n\n> \n> The rest here is similar in both the versions in what it does.\n> \n> > -\n> > - /* Create the formats maps. */\n> > - std::map> processedFormats;\n> > - std::map> rawFormats;\n> > -\n> > - for (const SimpleCameraData::Configuration &cfg : data->configs_) {\n> > - rawFormats[cfg.captureFormat].push_back(cfg.captureSize);\n> > - for (PixelFormat format : cfg.outputFormats)\n> > - processedFormats[format].push_back(cfg.outputSizes);\n> > - }\n> > + /* Create the formats map. */\n> > + std::map> formats;\n> > \n> > - if (processedRequested && processedFormats.empty()) {\n> > - LOG(SimplePipeline, Error)\n> > - << \"Processed stream requsted but no corresponding output configuration found\";\n> > - return nullptr;\n> > - }\n> > - if (rawRequested && rawFormats.empty()) {\n> > - LOG(SimplePipeline, Error)\n> > - << \"Raw stream requsted but no corresponding output configuration found\";\n> > - return nullptr;\n> \n> These checks are not present in Umang's version.\n\nYes, my implementation just tracks rawStream_ is requested or not.\nI didn't feel any need to track processed streams separately.\n\n> \n> > + for (const auto &it : data->formats_) {\n> > + for (const SimpleCameraData::Configuration *cfg : it.second)\n> > + formats[it.first].push_back(cfg->outputSizes);\n> > }\n> > \n> > - auto setUpFormatSizes = [](std::map> &formats) {\n> > - /* Sort the sizes and merge any consecutive overlapping ranges. */\n> > -\n> > - for (auto &[format, sizes] : formats) {\n> > - std::sort(sizes.begin(), sizes.end(),\n> > - [](SizeRange &a, SizeRange &b) {\n> > - return a.min < b.min;\n> > - });\n> > -\n> > - auto cur = sizes.begin();\n> > - auto next = cur;\n> > -\n> > - while (++next != sizes.end()) {\n> > - if (cur->max.width >= next->min.width &&\n> > - cur->max.height >= next->min.height)\n> > - cur->max = next->max;\n> > - else if (++cur != next)\n> > - *cur = *next;\n> > - }\n> > -\n> > - sizes.erase(++cur, sizes.end());\n> > + /* Sort the sizes and merge any consecutive overlapping ranges. */\n> > + for (auto &[format, sizes] : formats) {\n> > + std::sort(sizes.begin(), sizes.end(),\n> > + [](SizeRange &a, SizeRange &b) {\n> > + return a.min < b.min;\n> > + });\n> > +\n> > + auto cur = sizes.begin();\n> > + auto next = cur;\n> > +\n> > + while (++next != sizes.end()) {\n> > + if (cur->max.width >= next->min.width &&\n> > + cur->max.height >= next->min.height)\n> > + cur->max = next->max;\n> > + else if (++cur != next)\n> > + *cur = *next;\n> > }\n> > - };\n> > - setUpFormatSizes(processedFormats);\n> > - setUpFormatSizes(rawFormats);\n> > +\n> > + sizes.erase(++cur, sizes.end());\n> > + }\n\nIMO these are also some major difference implementation-wise here as\nwell, but towards a common goal.\n\n> > \n> > /*\n> > * Create the stream configurations. Take the first entry in the formats\n> > - * map as the default, for lack of a better option.\n> > + * map as the default for each of role (raw or non-raw), for lack of a\n> > + * better option.\n> > *\n> > * \\todo Implement a better way to pick the default format\n> > */\n> > for (StreamRole role : roles) {\n> > - const auto &formats = (role == StreamRole::Raw ? rawFormats : processedFormats);\n> > StreamConfiguration cfg{ StreamFormats{ formats } };\n> > - cfg.pixelFormat = formats.begin()->first;\n> > - cfg.size = formats.begin()->second[0].max;\n> > +\n> > + switch (role) {\n> > + case StreamRole::Raw:\n> > + for (auto &[format, sizes] : formats) {\n> > + if (!isFormatRaw(format))\n> > + continue;\n> > + cfg.pixelFormat = format;\n> > + cfg.size = sizes.back().max;\n> > + cfg.colorSpace = ColorSpace::Raw;\n> > + break;\n> > + }\n> > + break;\n> > + default:\n> > + for (auto &[format, sizes] : formats) {\n> > + if (isFormatRaw(format))\n> > + continue;\n> > + cfg.pixelFormat = format;\n> > + cfg.size = sizes[0].max;\n> > + }\n> > + }\n> \n> In my version, colour space handling is now only in validate(). The\n> pixel format and size selection are similar in both the versions.\n> \n> The rest of the diff are only cosmetic differences.\n> \n> > \n> > config->addConfiguration(cfg);\n> > }\n> > @@ -1532,7 +1482,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > \n> > for (unsigned int i = 0; i < config->size(); ++i) {\n> > StreamConfiguration &cfg = config->at(i);\n> > - bool rawStream = isRaw(cfg);\n> > + bool rawStream = isFormatRaw(cfg.pixelFormat);\n> > \n> > cfg.setStream(&data->streams_[i]);\n> > \n> > @@ -1571,7 +1521,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> > * Export buffers on the converter or capture video node, depending on\n> > * whether the converter is used or not.\n> > */\n> > - if (data->useConversion_ && stream != data->rawStream_)\n> > + if (data->useConversion_ && (stream != data->rawStream_))\n> > return data->converter_\n> > ? data->converter_->exportBuffers(stream, count, buffers)\n> > : data->swIsp_->exportBuffers(stream, count, buffers);\n> > @@ -1593,7 +1543,6 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > << pad->entity()->name() << \" in use\";\n> > return -EBUSY;\n> > }\n> > -\n> > if (data->useConversion_ && !data->rawStream_) {\n> > /*\n> > * When using the converter allocate a fixed number of internal\n> > @@ -1603,10 +1552,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > &data->conversionBuffers_);\n> > } else {\n> > /*\n> > - * Otherwise, prepare for using buffers from either the raw stream, if\n> > - * requested, or the only stream configured.\n> > + * Otherwise, prepare for using buffers from either the raw\n> > + * stream((if requested) or the only stream configured.\n> > */\n> > - Stream *stream = (data->rawStream_ ? data->rawStream_ : &data->streams_[0]);\n> > + Stream *stream;\n> > + if (data->rawStream_)\n> > + stream = data->rawStream_;\n> > + else\n> > + stream = &data->streams_[0];\n> > +\n> > ret = video->importBuffers(stream->configuration().bufferCount);\n> > }\n> > if (ret < 0) {\n> > @@ -1647,9 +1601,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > }\n> > \n> > /* Queue all internal buffers for capture. */\n> > - if (!data->rawStream_)\n> > + if (!data->rawStream_) {\n> > for (std::unique_ptr &buffer : data->conversionBuffers_)\n> > video->queueBuffer(buffer.get());\n> > + }\n> > }\n> > \n> > return 0;\n> > @@ -1700,7 +1655,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> > * queue, it will be handed to the converter in the capture\n> > * completion handler.\n> > */\n> > - if (data->useConversion_ && stream != data->rawStream_) {\n> > + if (data->useConversion_ && (stream != data->rawStream_)) {\n> > buffers.emplace(stream, buffer);\n> > metadataRequired = !!data->swIsp_;\n> > } else {\n> > @@ -1836,13 +1791,13 @@ bool SimplePipelineHandler::matchDevice(MediaDevice *media,\n> > /*\n> > * When the software ISP is enabled, the simple pipeline handler\n> > * exposes the raw stream, giving a total of two streams. This\n> > - * is mutually exclusive with the presence of a converter.\n> > + * is mutally exclusive with the presence of a converter.\n> > */\n> > ASSERT(!converter_);\n> > numStreams = 2;\n> > + swIspEnabled_ = true;\n> > }\n> > \n> > - swIspEnabled_ = info.swIspEnabled;\n> > const GlobalConfiguration &configuration = cameraManager()->_d()->configuration();\n> > for (GlobalConfiguration::Configuration entry :\n> > configuration.configuration()[\"pipelines\"][\"simple\"][\"supported_devices\"]\n>","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 757B0C3259\n\tfor ;\n\tWed, 22 Oct 2025 11:54:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 223EA606D6;\n\tWed, 22 Oct 2025 13:54:01 +0200 (CEST)","from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D6A89606B0\n\tfor ;\n\tWed, 22 Oct 2025 13:53:58 +0200 (CEST)","from [31.221.16.154] (helo=uajain)\n\tby fanzine2.igalia.com with esmtpsa \n\t(Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256)\n\t(Exim) id 1vBXPg-00D0Ho-MI; Wed, 22 Oct 2025 13:53:56 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=igalia.com header.i=@igalia.com\n\theader.b=\"HtDhkJR2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com;\n\ts=20170329;\n\th=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:\n\tSubject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID:\n\tContent-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc\n\t:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe:\n\tList-Post:List-Owner:List-Archive;\n\tbh=+ZWHM5++N3sO2sQrwDUfaHPt4If4ZOkUjaNz1i9yzvM=;\n\tb=HtDhkJR2gfVHQu8+L2a+2jtNnn\n\tstCSU+H8BEi6xFp+Wg87R9duEPC+cQaDwF29zpDJ7PAreW0X9+SnKu/onE1PiL38URVkxB5j4l/Pj\n\t7g5hdkv3g5eSv/ZMxrdu0/at4Cj4rdJ/0qvyIv9hZ0CwlGWLPtIiWxxpKvQLB+viYOJwYndiqBTuB\n\tOX1njBku1QEaLc8zrsVag2h2vqoU77bxORikehTNpk7kwKIY5bZGcWZfUttqccKDSKV2TzKczjkvd\n\ttFPhtcigcQ7IlwNL0C2W6CLJKhsbo7cY4+tZVnR1JDmzDHb+gn3gfcvbPaFoGm2lWbMkQjD+sid8i\n\tXD/mIhgQ==;","Date":"Wed, 22 Oct 2025 17:24:25 +0530","From":"Umang Jain ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, Laurent Pinchart\n\t, Kieran Bingham\n\t, =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?=\n\t, Paul Elder\n\t","Subject":"Re: [PATCH v13 0/8] Enable raw streams with software ISP","Message-ID":"<5kjtvmsrtwjxuffle7u4yxj3wkwqsr7d5hejt6icufkr765nm3@daxvltodcrny>","References":"<20251021182716.29274-1-mzamazal@redhat.com>\n\t<85sefcgl02.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<85sefcgl02.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"NeoMutt/20250905-dirty","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36602,"web_url":"https://patchwork.libcamera.org/comment/36602/","msgid":"<85y0opsfdp.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-11-01T19:48:50","subject":"Re: [PATCH v13 0/8] Enable raw streams with software ISP","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Umang,\n\nUmang Jain writes:\n\n> Hi Milan,\n>\n> Thanks for the setting up the diff.\n>\n> On Tue, Oct 21, 2025 at 08:36:29PM +0200, Milan Zamazal wrote:\n>> There is another branch of this series by Umang. Since there was no\n>> preference expressed which branch to use for the next version, I\n>> continue with mine.\n>> \n>> While I tried to incorporate some changes from Umang's branch\n>> previously, some differences remain, which I think require more\n>> discussion. I rebased Umang's branch on the same master version, made a\n>> diff between the two branches and commented on it below. The diff shows\n>> an update of the posted patches to Umang's branch, i.e. the original\n>> version in the diff is this v13, the new version is the rebased Umang's\n>> branch.\n>> \n>> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> > index 607b07949..529d7344d 100644\n>> > --- a/src/libcamera/pipeline/simple/simple.cpp\n>> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> > @@ -11,7 +11,6 @@\n>> > #include \n>> > #include \n>> > #include \n>> > -#include \n>> > #include \n>> > #include \n>> > #include \n>> > @@ -26,9 +25,7 @@\n>> > #include \n>> > \n>> > #include \n>> > -#include \n>> > #include \n>> > -#include \n>> > #include \n>> > #include \n>> > #include \n>> > @@ -40,7 +37,6 @@\n>> > #include \"libcamera/internal/converter.h\"\n>> > #include \"libcamera/internal/delayed_controls.h\"\n>> > #include \"libcamera/internal/device_enumerator.h\"\n>> > -#include \"libcamera/internal/formats.h\"\n>> > #include \"libcamera/internal/global_configuration.h\"\n>> > #include \"libcamera/internal/media_device.h\"\n>> > #include \"libcamera/internal/pipeline_handler.h\"\n>> > @@ -267,9 +263,9 @@ static const SimplePipelineInfo supportedDevices[] = {\n>> > { \"sun6i-csi\", {}, false },\n>> > };\n>> > \n>> > -bool isRaw(const StreamConfiguration &cfg)\n>> > +bool isFormatRaw(const libcamera::PixelFormat &pixFmt)\n>> \n>> isFormatRaw -> isRaw + argument type change was done in v12. Umang's\n>> version uses in one (different) place format that is not stored in the\n>> configuration.\n>> \n>> > {\n>> > - return libcamera::PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==\n>> > + return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==\n>> > libcamera::PixelFormatInfo::ColourEncodingRAW;\n>> > }\n>> > \n>> > @@ -372,11 +368,6 @@ private:\n>> > void ispStatsReady(uint32_t frame, uint32_t bufferId);\n>> > void metadataReady(uint32_t frame, const ControlList &metadata);\n>> > void setSensorControls(const ControlList &sensorControls);\n>> \n>> A significant difference between the two versions is the buffer\n>> handling.\n>> \n>> My version distinguishes between raw/non-raw cases at all the particular\n>> places:\n>\n> IMO, tracking the raw stream if requested in data->rawStream_ is\n> sufficent. I didn't find any need to split raw/non-raw cases \n> as user testing with cam. Did you find any issues with that in testing\n> the cases? I would also make a list of various cam user testing commands\n> and test both implementation against that. I did that last time, but it\n> was less formal.\n>\n> If there are no behaviour change in terms of user testing, maybe we\n> need a tie-breaker ? \n\nIt's easy to test typical regular cases but it's difficult to test the\nerror paths.\n\n>> > -\n>> > - bool processedRequested() const\n>> > - {\n>> > - return streams_.size() - (rawStream_ ? 1 : 0) > 0;\n>> > - }\n>> > };\n>> > \n>> > class SimpleCameraConfiguration : public CameraConfiguration\n>> > @@ -884,13 +875,10 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n>> > * point converting an erroneous buffer.\n>> > */\n>> > if (buffer->metadata().status != FrameMetadata::FrameSuccess) {\n>> > - if (rawStream_) {\n>> > + if (!useConversion_) {\n>> > /* No conversion, just complete the request. */\n>> > Request *request = buffer->request();\n>> > pipe->completeBuffer(request, buffer);\n>> > - SimpleFrameInfo *info = frameInfo_.find(request->sequence());\n>> > - if (info)\n>> > - info->metadataRequired = false;\n>> > tryCompleteRequest(request);\n>> > return;\n>> > }\n>> > @@ -900,8 +888,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n>> > * buffer for capture (unless the stream is being stopped), and\n>> > * complete the request with all the user-facing buffers.\n>> > */\n>> > - if (buffer->metadata().status != FrameMetadata::FrameCancelled &&\n>> > - !rawStream_)\n>> > + if (buffer->metadata().status != FrameMetadata::FrameCancelled)\n>> > video_->queueBuffer(buffer);\n>> > \n>> > if (conversionQueue_.empty())\n>> > @@ -957,7 +944,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n>> > \n>> > if (converter_)\n>> > converter_->queueBuffers(buffer, conversionQueue_.front().outputs);\n>> > - else if (processedRequested())\n>> > + else\n>> > /*\n>> > * request->sequence() cannot be retrieved from `buffer' inside\n>> > * queueBuffers because unique_ptr's make buffer->request() invalid\n>> > @@ -967,8 +954,6 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n>> > conversionQueue_.front().outputs);\n>> > \n>> > conversionQueue_.pop();\n>> > - if (rawStream_)\n>> > - pipe->completeBuffer(request, buffer);\n>> > return;\n>> > }\n>> > \n>> > @@ -1006,8 +991,14 @@ void SimpleCameraData::tryCompleteRequest(Request *request)\n>> > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer)\n>> > {\n>> > /* Queue the input buffer back for capture. */\n>> > - if (!rawStream_)\n>> > + if (!rawStream_) {\n>> > video_->queueBuffer(buffer);\n>> \n>> While Umang's version has this final buffer completion:\n>> \n>\n> Yes, it's simple, if rawStream_ is requested, you complete the\n> buffer/request (no further processing required) in the else block.\n> Otherwise, you queue for further processing (equivalent to your\n> processedRequests)\n>\n>\n>> > + } else {\n>> > + /* Complete the input buffer. */\n>> > + Request *request = buffer->request();\n>> > + if (pipe()->completeBuffer(request, buffer))\n>> > + tryCompleteRequest(request);\n>> > + }\n\nSo the else block handles the case when both processed and raw streams\nare requested, while my version tries to keep the original path for\nnon-raw. I think it'd be useful if somebody else looked into this,\nthere may be subtle traps.\n\n>> > }\n>> > \n>> > void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n>> > @@ -1156,31 +1147,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>> > LOG(SimplePipeline, Debug)\n>> > << \"Largest stream size is \" << maxStreamSize;\n>> > \n>> > + /* Cap the number of raw stream configuration */\n>> > + unsigned int rawCount = 0;\n>> > + PixelFormat requestedRawFormat;\n>> > + for (const StreamConfiguration &cfg : config_) {\n>> > + if (!isFormatRaw(cfg.pixelFormat))\n>> > + continue;\n>> > + requestedRawFormat = cfg.pixelFormat;\n>> > + rawCount++;\n>> > + }\n>> > +\n>> > + if (rawCount > 1) {\n>> > + LOG(SimplePipeline, Error)\n>> > + << \"Camera configuration with \"\n>> > + << rawCount << \" raw streams not supported\";\n>> > + return Invalid;\n>> > + }\n>> > +\n>> \n>> This differs in how to check for multiple raw streams in validate() and\n>> how to select the config but doesn't seem to do something very\n>> different.\n>\n> ack, but this mostly aligns how other pipeline handlers checks as well.\n\nI'll follow your version, I don't think further discussions would bring\nanything important here.\n\n>> > /*\n>> > - * Find the best configuration for the pipeline using a heuristic. First\n>> > - * select the pixel format based on the streams. If there is a raw stream,\n>> > - * its format has precedence. If there is no raw stream, the streams are\n>> > - * considered ordered from highest to lowest priority. Default to the first\n>> > - * pipeline configuration if no streams request a supported pixel format.\n>> > + * Find the best configuration for the pipeline using a heuristic.\n>> > + * First select the pixel format based on the raw streams followed by\n>> > + * non-raw streams (which are considered ordered from highest to lowest\n>> > + * priority). Default to the first pipeline configuration if no streams\n>> > + * request a supported pixel format.\n>> > */\n>> > - std::optional rawFormat;\n>> > - for (const auto &cfg : config_)\n>> > - if (isRaw(cfg)) {\n>> > - if (rawFormat) {\n>> > - LOG(SimplePipeline, Error)\n>> > - << \"Can't capture multiple raw streams\";\n>> > - return Invalid;\n>> > - }\n>> > - rawFormat = cfg.pixelFormat;\n>> > - }\n>> > + const std::vector *configs =\n>> > + &data_->formats_.begin()->second;\n>> > \n>> > - const std::vector *configs = nullptr;\n>> > - if (rawFormat) {\n>> > - auto it = data_->formats_.find(rawFormat.value());\n>> > - if (it != data_->formats_.end())\n>> > - configs = &it->second;\n>> > - }\n>> > - if (!configs)\n>> > + auto rawIter = data_->formats_.find(requestedRawFormat);\n>> > + if (rawIter != data_->formats_.end()) {\n>> > + configs = &rawIter->second;\n>> > + } else {\n>> > for (const StreamConfiguration &cfg : config_) {\n>> > auto it = data_->formats_.find(cfg.pixelFormat);\n>> > if (it != data_->formats_.end()) {\n>> > @@ -1188,8 +1185,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>> > break;\n>> > }\n>> > }\n>> > - if (!configs)\n>> > - configs = &data_->formats_.begin()->second;\n>> > + }\n>> > \n>> > /*\n>> > * \\todo Pick the best sensor output media bus format when the\n>> > @@ -1242,63 +1238,39 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>> > * require any conversion, similar to raw capture use cases). This is\n>> > * left as a future improvement.\n>> > */\n>> > - needConversion_ = config_.size() > 1 + (rawFormat ? 1 : 0);\n>> > + needConversion_ = config_.size() > 1 + rawCount;\n>> > \n>> > for (unsigned int i = 0; i < config_.size(); ++i) {\n>> > StreamConfiguration &cfg = config_[i];\n>> > - const bool raw = isRaw(cfg);\n>> > + const bool raw = isFormatRaw(cfg.pixelFormat);\n>> > \n>> > - /* Adjust the pixel format, colour space and size. */\n>> > -\n>> > - PixelFormat pixelFormat;\n>> > + /* Adjust the raw pixel format and size. */\n>> > if (raw) {\n>> > - pixelFormat = pipeConfig_->captureFormat;\n>> > - } else {\n>> > - auto it = std::find(pipeConfig_->outputFormats.begin(),\n>> > - pipeConfig_->outputFormats.end(),\n>> > - cfg.pixelFormat);\n>> > - if (it == pipeConfig_->outputFormats.end())\n>> > - it = pipeConfig_->outputFormats.begin();\n>> > - pixelFormat = *it;\n>> > - }\n>> > - if (cfg.pixelFormat != pixelFormat) {\n>> > - LOG(SimplePipeline, Debug)\n>> > - << \"Adjusting pixel format of a \"\n>> > - << (raw ? \"raw\" : \"processed\")\n>> > - << \" stream from \" << cfg.pixelFormat\n>> > - << \" to \" << pixelFormat;\n>> > - cfg.pixelFormat = pixelFormat;\n>> > - status = Adjusted;\n>> > - }\n>> > + if (cfg.pixelFormat != pipeConfig_->captureFormat ||\n>> > + cfg.size != pipeConfig_->captureSize) {\n>> > + cfg.pixelFormat = pipeConfig_->captureFormat;\n>> > + cfg.size = pipeConfig_->captureSize;\n>> > \n>> > - /*\n>> > - * Best effort to fix the color space. If the color space is not set,\n>> > - * set it according to the pixel format, which may not be correct (pixel\n>> > - * formats and color spaces are different things, although somewhat\n>> > - * related) but we don't have a better option at the moment. Then in any\n>> > - * case, perform the standard pixel format based color space adjustment.\n>> > - */\n>> > - if (!cfg.colorSpace) {\n>> > - const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);\n>> > - switch (info.colourEncoding) {\n>> > - case PixelFormatInfo::ColourEncodingRGB:\n>> > - cfg.colorSpace = ColorSpace::Srgb;\n>> > - break;\n>> > - case libcamera::PixelFormatInfo::ColourEncodingYUV:\n>> > - cfg.colorSpace = ColorSpace::Sycc;\n>> > - break;\n>> > - default:\n>> > - cfg.colorSpace = ColorSpace::Raw;\n>> > + LOG(SimplePipeline, Debug)\n>> > + << \"Adjusting raw stream to \"\n>> > + << cfg.toString();\n>> > + status = Adjusted;\n>> > }\n>> > - LOG(SimplePipeline, Debug)\n>> > - << \"Unspecified color space set to \"\n>> > - << cfg.colorSpace.value().toString();\n>> > - status = Adjusted;\n>> > }\n>> > - if (cfg.colorSpace->adjust(pixelFormat)) {\n>> \n>> The colour space adjustment is the primary difference in this diff area.\n>> In Umang's version, it's still handled when processing stream roles and\n>> it's missing here in validate(). The rest is more similar than the size\n>> of the diff output might suggest.\n>\n> I remember that I commented that colorspace changes could be separated\n> out from your series. My series only sets colorSpace::RAW to raw streams\n> AFAIR.\n\nI consider the colour space changes a prerequisite for the raw stream\nchanges. It's the first patch of the series and I'd be happy if at\nleast that one was merged because it fixes an annoying `cam' crash in my\nenvironment.\n\n>> > +\n>> > + /* Adjust the non-raw pixel format and size. */\n>> > + auto it = std::find(pipeConfig_->outputFormats.begin(),\n>> > + pipeConfig_->outputFormats.end(),\n>> > + cfg.pixelFormat);\n>> > + if (it == pipeConfig_->outputFormats.end())\n>> > + it = pipeConfig_->outputFormats.begin();\n>> > +\n>> > + PixelFormat pixelFormat = *it;\n>> > + if (!raw && cfg.pixelFormat != pixelFormat) {\n>> > LOG(SimplePipeline, Debug)\n>> > - << \"Color space adjusted to \"\n>> > - << cfg.colorSpace.value().toString();\n>> > + << \"Adjusting pixel format from \"\n>> > + << cfg.pixelFormat << \" to \" << pixelFormat;\n>> > + cfg.pixelFormat = pixelFormat;\n>> > status = Adjusted;\n>> > }\n>> > \n>> > @@ -1321,20 +1293,11 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>> > \n>> > /* \\todo Create a libcamera core class to group format and size */\n>> > if (cfg.pixelFormat != pipeConfig_->captureFormat ||\n>> > - cfg.size != pipeConfig_->captureSize) {\n>> > - if (raw) {\n>> > - cfg.pixelFormat = pipeConfig_->captureFormat;\n>> > - cfg.size = pipeConfig_->captureSize;\n>> > - LOG(SimplePipeline, Debug)\n>> > - << \"Adjusting raw configuration to \" << cfg;\n>> > - status = Adjusted;\n>> > - } else {\n>> > - needConversion_ = true;\n>> > - }\n>> > - }\n>> > + cfg.size != pipeConfig_->captureSize)\n>> > + needConversion_ = true;\n>> > \n>> > /* Set the stride and frameSize. */\n>> > - if (needConversion_ && !raw) {\n>> > + if (needConversion_) {\n>> > std::tie(cfg.stride, cfg.frameSize) =\n>> > data_->converter_\n>> > ? data_->converter_->strideAndFrameSize(cfg.pixelFormat,\n>> > @@ -1379,7 +1342,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>> > \n>> > SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)\n>> > : PipelineHandler(manager, kMaxQueuedRequestsDevice),\n>> > - converter_(nullptr)\n>> > + converter_(nullptr),\n>> > + swIspEnabled_(false)\n>> > {\n>> > }\n>> > \n>> > @@ -1393,78 +1357,64 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span> > if (roles.empty())\n>> > return config;\n>> > \n>> > - bool processedRequested = false;\n>> > - bool rawRequested = false;\n>> > - for (const auto &role : roles)\n>> > - if (role == StreamRole::Raw) {\n>> > - if (rawRequested) {\n>> > - LOG(SimplePipeline, Error)\n>> > - << \"Can't capture multiple raw streams\";\n>> > - return nullptr;\n>> > - }\n>> > - rawRequested = true;\n>> > - } else {\n>> > - processedRequested = true;\n>> > - }\n>> \n>> IIRC Umang suggested that checking for this in validate() is enough and\n>> it's not necessary to have a sort of duplicate check in\n>> generateConfiguration().\n>\n> The problem with validate() behaviour is that it's two fold.\n> 1) for validation as per docs\n> 2) for completing the rest of configuration in generateConfiguration.\n>\n> Sometime ago, I tried to ensure that the config->validate() return\n> value should be checked - but then Laurent told me about 2). Hence I think to\n> duplicate the check there hence I agree with this (since,\n> generateConfiguration() calls validate() - but for 2) not 1).\n>\n> This should be changed ideally - but I haven't thought it through.\n>\n> There was some discussion here:\n> https://patchwork.libcamera.org/patch/23806/#34899\n\nSo OK to leave this check here if I understand you correctly?\n\n>> \n>> The rest here is similar in both the versions in what it does.\n>> \n>> > -\n>> > - /* Create the formats maps. */\n>> > - std::map> processedFormats;\n>> > - std::map> rawFormats;\n>> > -\n>> > - for (const SimpleCameraData::Configuration &cfg : data->configs_) {\n>> > - rawFormats[cfg.captureFormat].push_back(cfg.captureSize);\n>> > - for (PixelFormat format : cfg.outputFormats)\n>> > - processedFormats[format].push_back(cfg.outputSizes);\n>> > - }\n>> > + /* Create the formats map. */\n>> > + std::map> formats;\n>> > \n>> > - if (processedRequested && processedFormats.empty()) {\n>> > - LOG(SimplePipeline, Error)\n>> > - << \"Processed stream requsted but no corresponding output configuration found\";\n>> > - return nullptr;\n>> > - }\n>> > - if (rawRequested && rawFormats.empty()) {\n>> > - LOG(SimplePipeline, Error)\n>> > - << \"Raw stream requsted but no corresponding output configuration found\";\n>> > - return nullptr;\n>> \n>> These checks are not present in Umang's version.\n>\n> Yes, my implementation just tracks rawStream_ is requested or not.\n> I didn't feel any need to track processed streams separately.\n\nDo you think the checks that matching output configurations exist are useless?\n\n>> > + for (const auto &it : data->formats_) {\n>> > + for (const SimpleCameraData::Configuration *cfg : it.second)\n>> > + formats[it.first].push_back(cfg->outputSizes);\n>> > }\n>> > \n>> > - auto setUpFormatSizes = [](std::map> &formats) {\n>> > - /* Sort the sizes and merge any consecutive overlapping ranges. */\n>> > -\n>> > - for (auto &[format, sizes] : formats) {\n>> > - std::sort(sizes.begin(), sizes.end(),\n>> > - [](SizeRange &a, SizeRange &b) {\n>> > - return a.min < b.min;\n>> > - });\n>> > -\n>> > - auto cur = sizes.begin();\n>> > - auto next = cur;\n>> > -\n>> > - while (++next != sizes.end()) {\n>> > - if (cur->max.width >= next->min.width &&\n>> > - cur->max.height >= next->min.height)\n>> > - cur->max = next->max;\n>> > - else if (++cur != next)\n>> > - *cur = *next;\n>> > - }\n>> > -\n>> > - sizes.erase(++cur, sizes.end());\n>> > + /* Sort the sizes and merge any consecutive overlapping ranges. */\n>> > + for (auto &[format, sizes] : formats) {\n>> > + std::sort(sizes.begin(), sizes.end(),\n>> > + [](SizeRange &a, SizeRange &b) {\n>> > + return a.min < b.min;\n>> > + });\n>> > +\n>> > + auto cur = sizes.begin();\n>> > + auto next = cur;\n>> > +\n>> > + while (++next != sizes.end()) {\n>> > + if (cur->max.width >= next->min.width &&\n>> > + cur->max.height >= next->min.height)\n>> > + cur->max = next->max;\n>> > + else if (++cur != next)\n>> > + *cur = *next;\n>> > }\n>> > - };\n>> > - setUpFormatSizes(processedFormats);\n>> > - setUpFormatSizes(rawFormats);\n>> > +\n>> > + sizes.erase(++cur, sizes.end());\n>> > + }\n>\n> IMO these are also some major difference implementation-wise here as\n> well, but towards a common goal.\n\nYes.\n\n>> > \n>> > /*\n>> > * Create the stream configurations. Take the first entry in the formats\n>> > - * map as the default, for lack of a better option.\n>> > + * map as the default for each of role (raw or non-raw), for lack of a\n>> > + * better option.\n>> > *\n>> > * \\todo Implement a better way to pick the default format\n>> > */\n>> > for (StreamRole role : roles) {\n>> > - const auto &formats = (role == StreamRole::Raw ? rawFormats : processedFormats);\n>> > StreamConfiguration cfg{ StreamFormats{ formats } };\n>> > - cfg.pixelFormat = formats.begin()->first;\n>> > - cfg.size = formats.begin()->second[0].max;\n>> > +\n>> > + switch (role) {\n>> > + case StreamRole::Raw:\n>> > + for (auto &[format, sizes] : formats) {\n>> > + if (!isFormatRaw(format))\n>> > + continue;\n>> > + cfg.pixelFormat = format;\n>> > + cfg.size = sizes.back().max;\n>> > + cfg.colorSpace = ColorSpace::Raw;\n>> > + break;\n>> > + }\n>> > + break;\n>> > + default:\n>> > + for (auto &[format, sizes] : formats) {\n>> > + if (isFormatRaw(format))\n>> > + continue;\n>> > + cfg.pixelFormat = format;\n>> > + cfg.size = sizes[0].max;\n>> > + }\n>> > + }\n>> \n>> In my version, colour space handling is now only in validate(). The\n>> pixel format and size selection are similar in both the versions.\n>> \n>> The rest of the diff are only cosmetic differences.\n>> \n>> > \n>> > config->addConfiguration(cfg);\n>> > }\n>> > @@ -1532,7 +1482,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n>> > \n>> > for (unsigned int i = 0; i < config->size(); ++i) {\n>> > StreamConfiguration &cfg = config->at(i);\n>> > - bool rawStream = isRaw(cfg);\n>> > + bool rawStream = isFormatRaw(cfg.pixelFormat);\n>> > \n>> > cfg.setStream(&data->streams_[i]);\n>> > \n>> > @@ -1571,7 +1521,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n>> > * Export buffers on the converter or capture video node, depending on\n>> > * whether the converter is used or not.\n>> > */\n>> > - if (data->useConversion_ && stream != data->rawStream_)\n>> > + if (data->useConversion_ && (stream != data->rawStream_))\n>> > return data->converter_\n>> > ? data->converter_->exportBuffers(stream, count, buffers)\n>> > : data->swIsp_->exportBuffers(stream, count, buffers);\n>> > @@ -1593,7 +1543,6 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n>> > << pad->entity()->name() << \" in use\";\n>> > return -EBUSY;\n>> > }\n>> > -\n>> > if (data->useConversion_ && !data->rawStream_) {\n>> > /*\n>> > * When using the converter allocate a fixed number of internal\n>> > @@ -1603,10 +1552,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n>> > &data->conversionBuffers_);\n>> > } else {\n>> > /*\n>> > - * Otherwise, prepare for using buffers from either the raw stream, if\n>> > - * requested, or the only stream configured.\n>> > + * Otherwise, prepare for using buffers from either the raw\n>> > + * stream((if requested) or the only stream configured.\n>> > */\n>> > - Stream *stream = (data->rawStream_ ? data->rawStream_ : &data->streams_[0]);\n>> > + Stream *stream;\n>> > + if (data->rawStream_)\n>> > + stream = data->rawStream_;\n>> > + else\n>> > + stream = &data->streams_[0];\n>> > +\n>> > ret = video->importBuffers(stream->configuration().bufferCount);\n>> > }\n>> > if (ret < 0) {\n>> > @@ -1647,9 +1601,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n>> > }\n>> > \n>> > /* Queue all internal buffers for capture. */\n>> > - if (!data->rawStream_)\n>> > + if (!data->rawStream_) {\n>> > for (std::unique_ptr &buffer : data->conversionBuffers_)\n>> > video->queueBuffer(buffer.get());\n>> > + }\n>> > }\n>> > \n>> > return 0;\n>> > @@ -1700,7 +1655,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n>> > * queue, it will be handed to the converter in the capture\n>> > * completion handler.\n>> > */\n>> > - if (data->useConversion_ && stream != data->rawStream_) {\n>> > + if (data->useConversion_ && (stream != data->rawStream_)) {\n>> > buffers.emplace(stream, buffer);\n>> > metadataRequired = !!data->swIsp_;\n>> > } else {\n>> > @@ -1836,13 +1791,13 @@ bool SimplePipelineHandler::matchDevice(MediaDevice *media,\n>> > /*\n>> > * When the software ISP is enabled, the simple pipeline handler\n>> > * exposes the raw stream, giving a total of two streams. This\n>> > - * is mutually exclusive with the presence of a converter.\n>> > + * is mutally exclusive with the presence of a converter.\n>> > */\n>> > ASSERT(!converter_);\n>> > numStreams = 2;\n>> > + swIspEnabled_ = true;\n>> > }\n>> > \n>> > - swIspEnabled_ = info.swIspEnabled;\n>> > const GlobalConfiguration &configuration = cameraManager()->_d()->configuration();\n>> > for (GlobalConfiguration::Configuration entry :\n>> > configuration.configuration()[\"pipelines\"][\"simple\"][\"supported_devices\"]\n>>","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 AA6BAC3241\n\tfor ;\n\tSat, 1 Nov 2025 19:49:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E02496086F;\n\tSat, 1 Nov 2025 20:49:03 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C7E3D606D5\n\tfor ;\n\tSat, 1 Nov 2025 20:49:01 +0100 (CET)","from mail-wm1-f70.google.com (mail-wm1-f70.google.com\n\t[209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-628-LWjisR4lPl2WYFDEudXowQ-1; Sat, 01 Nov 2025 15:48:54 -0400","by mail-wm1-f70.google.com with SMTP id\n\t5b1f17b1804b1-47113538d8cso17834025e9.1\n\tfor ;\n\tSat, 01 Nov 2025 12:48:54 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-4773c4a812csm71483195e9.6.2025.11.01.12.48.51\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 01 Nov 2025 12:48:51 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"N8+tdXjf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1762026540;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=7yCTUd3daS6H/W3nYMgKluo0jCPHObmz8Wdv+P0j4us=;\n\tb=N8+tdXjfg2LHRF6uxqkVjmiP0idFX4Dsjxcl7DPz1CdZ6k8qOkRuCTlBB9o0+sZ6XPbEnE\n\tvOW3efyBcFLsoFIqqczqEl8byy+sUg3zzmNiZ+Cw6Ke3nxlEIJR5xuPjGdBJRMA1HZjHzd\n\tYHmo6gU0fBZbUtFQw76lSynkRaCezxg=","X-MC-Unique":"LWjisR4lPl2WYFDEudXowQ-1","X-Mimecast-MFC-AGG-ID":"LWjisR4lPl2WYFDEudXowQ_1762026533","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1762026533; x=1762631333;\n\th=mime-version:message-id:date:user-agent:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=7yCTUd3daS6H/W3nYMgKluo0jCPHObmz8Wdv+P0j4us=;\n\tb=Dc+QT0kHxOuOGEKs4Xwr2rcl4SW0k17+P3ypM+gh3Ml6dlmHzr9zRWT2K1Gka/l/bB\n\tAwJkLN4XNDScBDGlxMS/DKWUr9c9MggjF0ohSNj5+RMHHskwiT034E35P6WkWayh2Cay\n\tOXxD9mIEKf/gKSwIcdfAKvr2crCvEQkDFQ8LlTOMtsTyhHWgsoRstcRl0LUrSxUKqYkO\n\tmV2ZP2Uwv0oFytqNT0gsuyyV1pwOVTb84xgIomFOzOygnfpnCRdF2G0l9aHZifpYrYn2\n\t39VywEWU8ZlgtkLvv/tLX8qmlro5MDbSoNMuVIEaNfYyCRtZM0jL+02fNWe4EIplTEvj\n\t+hkw==","X-Gm-Message-State":"AOJu0YzT3XhJFiszrHkbWl5iHGeaA28wAugZluNMhvRwOrFHWYMiyjxu\n\tTrDtSQQQP3hx+QlcHFBDR3P8DBYT7GGMIIRiTo7QjKnNKzDxVFtp7pow/A6iKDlQcOfFjg1TyXZ\n\tNaV8HzXbnc8VXTzFYBmNKkQ65iX5UXfYavHTS/FDYWHxCrFfkfSym8tmNBngsyiB/pc8KRFqiIs\n\tk=","X-Gm-Gg":"ASbGncsdukoabrJsZVDfer9ZQZ83hBhK9w9F6uqCAqtRIrjOu9AHg+qCGxJnpKQj5lG\n\tDTJM3BS+rozOvDufl0MsJmJMhQNdDjXFGdcBsBbNnVvR4qYs7CJB8Q1iE+aib+ulwPcn9cQmS4N\n\tn0T1/12Rl59eSrmpLu9TRx3OGoLh9f/Rm8XYwvYPV2zoOC2ZiM1PEJHYNW4w0RQnz55YULd5tPS\n\tFAY/EMRF1vUHS5/45LjdbkQSpRTocKluRBphL2UDjB/+fwm6/KDXCaEmHrOyREWk9vzwdhRg43z\n\tFbo32dY2ivOoR68uvCCR1xY1rUGb5G67ooKL670arj2z41J5sOHedZ/lQprauqAvyyPqCbLA5QJ\n\tIUfgktvP6FuLyvQY67I9uixiXqPjAyk8SvO0ybniNnKVEdC2Mv3c2","X-Received":["by 2002:a05:600c:8b32:b0:46e:37af:f90e with SMTP id\n\t5b1f17b1804b1-4773bfd5c53mr41378445e9.6.1762026532918; \n\tSat, 01 Nov 2025 12:48:52 -0700 (PDT)","by 2002:a05:600c:8b32:b0:46e:37af:f90e with SMTP id\n\t5b1f17b1804b1-4773bfd5c53mr41378245e9.6.1762026532220; \n\tSat, 01 Nov 2025 12:48:52 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEFsUIPvIM83AO2hCZr1sbe5zPsMvPA5E0ZgHkczPmzeWBVJY7VB5Ol7WbMC5V2Xa6i6AeQzQ==","From":"Milan Zamazal ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org, Laurent Pinchart\n\t,\n\tKieran Bingham , =?utf-8?q?Barnab?=\n\t=?utf-8?b?w6FzIFDFkWN6ZQ==?=\n\t, Paul Elder\n\t","Subject":"Re: [PATCH v13 0/8] Enable raw streams with software ISP","In-Reply-To":"<5kjtvmsrtwjxuffle7u4yxj3wkwqsr7d5hejt6icufkr765nm3@daxvltodcrny>\n\t(Umang Jain's message of \"Wed, 22 Oct 2025 17:24:25 +0530\")","References":"<20251021182716.29274-1-mzamazal@redhat.com>\n\t<85sefcgl02.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<5kjtvmsrtwjxuffle7u4yxj3wkwqsr7d5hejt6icufkr765nm3@daxvltodcrny>","User-Agent":"Gnus/5.13 (Gnus v5.13)","Date":"Sat, 01 Nov 2025 20:48:50 +0100","Message-ID":"<85y0opsfdp.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"h4B4_sY53hnwei8hSXd5Tup79HRcue_aEvDvGxNIMvo_1762026533","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36603,"web_url":"https://patchwork.libcamera.org/comment/36603/","msgid":"<85zf95sg80.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-11-01T19:30:39","subject":"Re: [PATCH v13 0/8] Enable raw streams with software ISP","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Umang,\n\nUmang Jain writes:\n\n> Hi Milan,\n>\n> Thanks for the setting up the diff.\n>\n> On Tue, Oct 21, 2025 at 08:36:29PM +0200, Milan Zamazal wrote:\n>> There is another branch of this series by Umang. Since there was no\n>> preference expressed which branch to use for the next version, I\n>> continue with mine.\n>> \n>> While I tried to incorporate some changes from Umang's branch\n>> previously, some differences remain, which I think require more\n>> discussion. I rebased Umang's branch on the same master version, made a\n>> diff between the two branches and commented on it below. The diff shows\n>> an update of the posted patches to Umang's branch, i.e. the original\n>> version in the diff is this v13, the new version is the rebased Umang's\n>> branch.\n>> \n>> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> > index 607b07949..529d7344d 100644\n>> > --- a/src/libcamera/pipeline/simple/simple.cpp\n>> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> > @@ -11,7 +11,6 @@\n>> > #include \n>> > #include \n>> > #include \n>> > -#include \n>> > #include \n>> > #include \n>> > #include \n>> > @@ -26,9 +25,7 @@\n>> > #include \n>> > \n>> > #include \n>> > -#include \n>> > #include \n>> > -#include \n>> > #include \n>> > #include \n>> > #include \n>> > @@ -40,7 +37,6 @@\n>> > #include \"libcamera/internal/converter.h\"\n>> > #include \"libcamera/internal/delayed_controls.h\"\n>> > #include \"libcamera/internal/device_enumerator.h\"\n>> > -#include \"libcamera/internal/formats.h\"\n>> > #include \"libcamera/internal/global_configuration.h\"\n>> > #include \"libcamera/internal/media_device.h\"\n>> > #include \"libcamera/internal/pipeline_handler.h\"\n>> > @@ -267,9 +263,9 @@ static const SimplePipelineInfo supportedDevices[] = {\n>> > { \"sun6i-csi\", {}, false },\n>> > };\n>> > \n>> > -bool isRaw(const StreamConfiguration &cfg)\n>> > +bool isFormatRaw(const libcamera::PixelFormat &pixFmt)\n>> \n>> isFormatRaw -> isRaw + argument type change was done in v12. Umang's\n>> version uses in one (different) place format that is not stored in the\n>> configuration.\n>> \n>> > {\n>> > - return libcamera::PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==\n>> > + return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==\n>> > libcamera::PixelFormatInfo::ColourEncodingRAW;\n>> > }\n>> > \n>> > @@ -372,11 +368,6 @@ private:\n>> > void ispStatsReady(uint32_t frame, uint32_t bufferId);\n>> > void metadataReady(uint32_t frame, const ControlList &metadata);\n>> > void setSensorControls(const ControlList &sensorControls);\n>> \n>> A significant difference between the two versions is the buffer\n>> handling.\n>> \n>> My version distinguishes between raw/non-raw cases at all the particular\n>> places:\n>\n> IMO, tracking the raw stream if requested in data->rawStream_ is\n> sufficent. I didn't find any need to split raw/non-raw cases \n> as user testing with cam. Did you find any issues with that in testing\n> the cases? I would also make a list of various cam user testing commands\n> and test both implementation against that. I did that last time, but it\n> was less formal.\n>\n> If there are no behaviour change in terms of user testing, maybe we\n> need a tie-breaker ? \n\nIt's easy to test typical regular cases but it's difficult to test the\nerror paths.\n\n>> > -\n>> > - bool processedRequested() const\n>> > - {\n>> > - return streams_.size() - (rawStream_ ? 1 : 0) > 0;\n>> > - }\n>> > };\n>> > \n>> > class SimpleCameraConfiguration : public CameraConfiguration\n>> > @@ -884,13 +875,10 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n>> > * point converting an erroneous buffer.\n>> > */\n>> > if (buffer->metadata().status != FrameMetadata::FrameSuccess) {\n>> > - if (rawStream_) {\n>> > + if (!useConversion_) {\n>> > /* No conversion, just complete the request. */\n>> > Request *request = buffer->request();\n>> > pipe->completeBuffer(request, buffer);\n>> > - SimpleFrameInfo *info = frameInfo_.find(request->sequence());\n>> > - if (info)\n>> > - info->metadataRequired = false;\n>> > tryCompleteRequest(request);\n>> > return;\n>> > }\n>> > @@ -900,8 +888,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n>> > * buffer for capture (unless the stream is being stopped), and\n>> > * complete the request with all the user-facing buffers.\n>> > */\n>> > - if (buffer->metadata().status != FrameMetadata::FrameCancelled &&\n>> > - !rawStream_)\n>> > + if (buffer->metadata().status != FrameMetadata::FrameCancelled)\n>> > video_->queueBuffer(buffer);\n>> > \n>> > if (conversionQueue_.empty())\n>> > @@ -957,7 +944,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n>> > \n>> > if (converter_)\n>> > converter_->queueBuffers(buffer, conversionQueue_.front().outputs);\n>> > - else if (processedRequested())\n>> > + else\n>> > /*\n>> > * request->sequence() cannot be retrieved from `buffer' inside\n>> > * queueBuffers because unique_ptr's make buffer->request() invalid\n>> > @@ -967,8 +954,6 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n>> > conversionQueue_.front().outputs);\n>> > \n>> > conversionQueue_.pop();\n>> > - if (rawStream_)\n>> > - pipe->completeBuffer(request, buffer);\n>> > return;\n>> > }\n>> > \n>> > @@ -1006,8 +991,14 @@ void SimpleCameraData::tryCompleteRequest(Request *request)\n>> > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer)\n>> > {\n>> > /* Queue the input buffer back for capture. */\n>> > - if (!rawStream_)\n>> > + if (!rawStream_) {\n>> > video_->queueBuffer(buffer);\n>> \n>> While Umang's version has this final buffer completion:\n>> \n>\n> Yes, it's simple, if rawStream_ is requested, you complete the\n> buffer/request (no further processing required) in the else block.\n> Otherwise, you queue for further processing (equivalent to your\n> processedRequests)\n>\n>\n>> > + } else {\n>> > + /* Complete the input buffer. */\n>> > + Request *request = buffer->request();\n>> > + if (pipe()->completeBuffer(request, buffer))\n>> > + tryCompleteRequest(request);\n>> > + }\n\nSo the else block handles the case when both processed and raw streams\nare requested, while my version tries to keep the original path for\nnon-raw. I think it'd be useful if somebody else looked into this,\nthere may be subtle traps.\n\n>> > }\n>> > \n>> > void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n>> > @@ -1156,31 +1147,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>> > LOG(SimplePipeline, Debug)\n>> > << \"Largest stream size is \" << maxStreamSize;\n>> > \n>> > + /* Cap the number of raw stream configuration */\n>> > + unsigned int rawCount = 0;\n>> > + PixelFormat requestedRawFormat;\n>> > + for (const StreamConfiguration &cfg : config_) {\n>> > + if (!isFormatRaw(cfg.pixelFormat))\n>> > + continue;\n>> > + requestedRawFormat = cfg.pixelFormat;\n>> > + rawCount++;\n>> > + }\n>> > +\n>> > + if (rawCount > 1) {\n>> > + LOG(SimplePipeline, Error)\n>> > + << \"Camera configuration with \"\n>> > + << rawCount << \" raw streams not supported\";\n>> > + return Invalid;\n>> > + }\n>> > +\n>> \n>> This differs in how to check for multiple raw streams in validate() and\n>> how to select the config but doesn't seem to do something very\n>> different.\n>\n> ack, but this mostly aligns how other pipeline handlers checks as well.\n\nI'll follow your version, I don't think further discussions would bring\nanything important here.\n\n>> > /*\n>> > - * Find the best configuration for the pipeline using a heuristic. First\n>> > - * select the pixel format based on the streams. If there is a raw stream,\n>> > - * its format has precedence. If there is no raw stream, the streams are\n>> > - * considered ordered from highest to lowest priority. Default to the first\n>> > - * pipeline configuration if no streams request a supported pixel format.\n>> > + * Find the best configuration for the pipeline using a heuristic.\n>> > + * First select the pixel format based on the raw streams followed by\n>> > + * non-raw streams (which are considered ordered from highest to lowest\n>> > + * priority). Default to the first pipeline configuration if no streams\n>> > + * request a supported pixel format.\n>> > */\n>> > - std::optional rawFormat;\n>> > - for (const auto &cfg : config_)\n>> > - if (isRaw(cfg)) {\n>> > - if (rawFormat) {\n>> > - LOG(SimplePipeline, Error)\n>> > - << \"Can't capture multiple raw streams\";\n>> > - return Invalid;\n>> > - }\n>> > - rawFormat = cfg.pixelFormat;\n>> > - }\n>> > + const std::vector *configs =\n>> > + &data_->formats_.begin()->second;\n>> > \n>> > - const std::vector *configs = nullptr;\n>> > - if (rawFormat) {\n>> > - auto it = data_->formats_.find(rawFormat.value());\n>> > - if (it != data_->formats_.end())\n>> > - configs = &it->second;\n>> > - }\n>> > - if (!configs)\n>> > + auto rawIter = data_->formats_.find(requestedRawFormat);\n>> > + if (rawIter != data_->formats_.end()) {\n>> > + configs = &rawIter->second;\n>> > + } else {\n>> > for (const StreamConfiguration &cfg : config_) {\n>> > auto it = data_->formats_.find(cfg.pixelFormat);\n>> > if (it != data_->formats_.end()) {\n>> > @@ -1188,8 +1185,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>> > break;\n>> > }\n>> > }\n>> > - if (!configs)\n>> > - configs = &data_->formats_.begin()->second;\n>> > + }\n>> > \n>> > /*\n>> > * \\todo Pick the best sensor output media bus format when the\n>> > @@ -1242,63 +1238,39 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>> > * require any conversion, similar to raw capture use cases). This is\n>> > * left as a future improvement.\n>> > */\n>> > - needConversion_ = config_.size() > 1 + (rawFormat ? 1 : 0);\n>> > + needConversion_ = config_.size() > 1 + rawCount;\n>> > \n>> > for (unsigned int i = 0; i < config_.size(); ++i) {\n>> > StreamConfiguration &cfg = config_[i];\n>> > - const bool raw = isRaw(cfg);\n>> > + const bool raw = isFormatRaw(cfg.pixelFormat);\n>> > \n>> > - /* Adjust the pixel format, colour space and size. */\n>> > -\n>> > - PixelFormat pixelFormat;\n>> > + /* Adjust the raw pixel format and size. */\n>> > if (raw) {\n>> > - pixelFormat = pipeConfig_->captureFormat;\n>> > - } else {\n>> > - auto it = std::find(pipeConfig_->outputFormats.begin(),\n>> > - pipeConfig_->outputFormats.end(),\n>> > - cfg.pixelFormat);\n>> > - if (it == pipeConfig_->outputFormats.end())\n>> > - it = pipeConfig_->outputFormats.begin();\n>> > - pixelFormat = *it;\n>> > - }\n>> > - if (cfg.pixelFormat != pixelFormat) {\n>> > - LOG(SimplePipeline, Debug)\n>> > - << \"Adjusting pixel format of a \"\n>> > - << (raw ? \"raw\" : \"processed\")\n>> > - << \" stream from \" << cfg.pixelFormat\n>> > - << \" to \" << pixelFormat;\n>> > - cfg.pixelFormat = pixelFormat;\n>> > - status = Adjusted;\n>> > - }\n>> > + if (cfg.pixelFormat != pipeConfig_->captureFormat ||\n>> > + cfg.size != pipeConfig_->captureSize) {\n>> > + cfg.pixelFormat = pipeConfig_->captureFormat;\n>> > + cfg.size = pipeConfig_->captureSize;\n>> > \n>> > - /*\n>> > - * Best effort to fix the color space. If the color space is not set,\n>> > - * set it according to the pixel format, which may not be correct (pixel\n>> > - * formats and color spaces are different things, although somewhat\n>> > - * related) but we don't have a better option at the moment. Then in any\n>> > - * case, perform the standard pixel format based color space adjustment.\n>> > - */\n>> > - if (!cfg.colorSpace) {\n>> > - const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);\n>> > - switch (info.colourEncoding) {\n>> > - case PixelFormatInfo::ColourEncodingRGB:\n>> > - cfg.colorSpace = ColorSpace::Srgb;\n>> > - break;\n>> > - case libcamera::PixelFormatInfo::ColourEncodingYUV:\n>> > - cfg.colorSpace = ColorSpace::Sycc;\n>> > - break;\n>> > - default:\n>> > - cfg.colorSpace = ColorSpace::Raw;\n>> > + LOG(SimplePipeline, Debug)\n>> > + << \"Adjusting raw stream to \"\n>> > + << cfg.toString();\n>> > + status = Adjusted;\n>> > }\n>> > - LOG(SimplePipeline, Debug)\n>> > - << \"Unspecified color space set to \"\n>> > - << cfg.colorSpace.value().toString();\n>> > - status = Adjusted;\n>> > }\n>> > - if (cfg.colorSpace->adjust(pixelFormat)) {\n>> \n>> The colour space adjustment is the primary difference in this diff area.\n>> In Umang's version, it's still handled when processing stream roles and\n>> it's missing here in validate(). The rest is more similar than the size\n>> of the diff output might suggest.\n>\n> I remember that I commented that colorspace changes could be separated\n> out from your series. My series only sets colorSpace::RAW to raw streams\n> AFAIR.\n\nI consider the colour space changes a prerequisite for the raw stream\nchanges. It's the first patch of the series and I'd be happy if at\nleast that one was merged because it fixes an annoying `cam' crash in my\nenvironment.\n\n>> > +\n>> > + /* Adjust the non-raw pixel format and size. */\n>> > + auto it = std::find(pipeConfig_->outputFormats.begin(),\n>> > + pipeConfig_->outputFormats.end(),\n>> > + cfg.pixelFormat);\n>> > + if (it == pipeConfig_->outputFormats.end())\n>> > + it = pipeConfig_->outputFormats.begin();\n>> > +\n>> > + PixelFormat pixelFormat = *it;\n>> > + if (!raw && cfg.pixelFormat != pixelFormat) {\n>> > LOG(SimplePipeline, Debug)\n>> > - << \"Color space adjusted to \"\n>> > - << cfg.colorSpace.value().toString();\n>> > + << \"Adjusting pixel format from \"\n>> > + << cfg.pixelFormat << \" to \" << pixelFormat;\n>> > + cfg.pixelFormat = pixelFormat;\n>> > status = Adjusted;\n>> > }\n>> > \n>> > @@ -1321,20 +1293,11 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>> > \n>> > /* \\todo Create a libcamera core class to group format and size */\n>> > if (cfg.pixelFormat != pipeConfig_->captureFormat ||\n>> > - cfg.size != pipeConfig_->captureSize) {\n>> > - if (raw) {\n>> > - cfg.pixelFormat = pipeConfig_->captureFormat;\n>> > - cfg.size = pipeConfig_->captureSize;\n>> > - LOG(SimplePipeline, Debug)\n>> > - << \"Adjusting raw configuration to \" << cfg;\n>> > - status = Adjusted;\n>> > - } else {\n>> > - needConversion_ = true;\n>> > - }\n>> > - }\n>> > + cfg.size != pipeConfig_->captureSize)\n>> > + needConversion_ = true;\n>> > \n>> > /* Set the stride and frameSize. */\n>> > - if (needConversion_ && !raw) {\n>> > + if (needConversion_) {\n>> > std::tie(cfg.stride, cfg.frameSize) =\n>> > data_->converter_\n>> > ? data_->converter_->strideAndFrameSize(cfg.pixelFormat,\n>> > @@ -1379,7 +1342,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>> > \n>> > SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)\n>> > : PipelineHandler(manager, kMaxQueuedRequestsDevice),\n>> > - converter_(nullptr)\n>> > + converter_(nullptr),\n>> > + swIspEnabled_(false)\n>> > {\n>> > }\n>> > \n>> > @@ -1393,78 +1357,64 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span> > if (roles.empty())\n>> > return config;\n>> > \n>> > - bool processedRequested = false;\n>> > - bool rawRequested = false;\n>> > - for (const auto &role : roles)\n>> > - if (role == StreamRole::Raw) {\n>> > - if (rawRequested) {\n>> > - LOG(SimplePipeline, Error)\n>> > - << \"Can't capture multiple raw streams\";\n>> > - return nullptr;\n>> > - }\n>> > - rawRequested = true;\n>> > - } else {\n>> > - processedRequested = true;\n>> > - }\n>> \n>> IIRC Umang suggested that checking for this in validate() is enough and\n>> it's not necessary to have a sort of duplicate check in\n>> generateConfiguration().\n>\n> The problem with validate() behaviour is that it's two fold.\n> 1) for validation as per docs\n> 2) for completing the rest of configuration in generateConfiguration.\n>\n> Sometime ago, I tried to ensure that the config->validate() return\n> value should be checked - but then Laurent told me about 2). Hence I think to\n> duplicate the check there hence I agree with this (since,\n> generateConfiguration() calls validate() - but for 2) not 1).\n>\n> This should be changed ideally - but I haven't thought it through.\n>\n> There was some discussion here:\n> https://patchwork.libcamera.org/patch/23806/#34899\n\nSo OK to leave this check here if I understand you correctly?\n\n>> \n>> The rest here is similar in both the versions in what it does.\n>> \n>> > -\n>> > - /* Create the formats maps. */\n>> > - std::map> processedFormats;\n>> > - std::map> rawFormats;\n>> > -\n>> > - for (const SimpleCameraData::Configuration &cfg : data->configs_) {\n>> > - rawFormats[cfg.captureFormat].push_back(cfg.captureSize);\n>> > - for (PixelFormat format : cfg.outputFormats)\n>> > - processedFormats[format].push_back(cfg.outputSizes);\n>> > - }\n>> > + /* Create the formats map. */\n>> > + std::map> formats;\n>> > \n>> > - if (processedRequested && processedFormats.empty()) {\n>> > - LOG(SimplePipeline, Error)\n>> > - << \"Processed stream requsted but no corresponding output configuration found\";\n>> > - return nullptr;\n>> > - }\n>> > - if (rawRequested && rawFormats.empty()) {\n>> > - LOG(SimplePipeline, Error)\n>> > - << \"Raw stream requsted but no corresponding output configuration found\";\n>> > - return nullptr;\n>> \n>> These checks are not present in Umang's version.\n>\n> Yes, my implementation just tracks rawStream_ is requested or not.\n> I didn't feel any need to track processed streams separately.\n\nDo you think the checks that matching output configurations exist are useless?\n\n>> > + for (const auto &it : data->formats_) {\n>> > + for (const SimpleCameraData::Configuration *cfg : it.second)\n>> > + formats[it.first].push_back(cfg->outputSizes);\n>> > }\n>> > \n>> > - auto setUpFormatSizes = [](std::map> &formats) {\n>> > - /* Sort the sizes and merge any consecutive overlapping ranges. */\n>> > -\n>> > - for (auto &[format, sizes] : formats) {\n>> > - std::sort(sizes.begin(), sizes.end(),\n>> > - [](SizeRange &a, SizeRange &b) {\n>> > - return a.min < b.min;\n>> > - });\n>> > -\n>> > - auto cur = sizes.begin();\n>> > - auto next = cur;\n>> > -\n>> > - while (++next != sizes.end()) {\n>> > - if (cur->max.width >= next->min.width &&\n>> > - cur->max.height >= next->min.height)\n>> > - cur->max = next->max;\n>> > - else if (++cur != next)\n>> > - *cur = *next;\n>> > - }\n>> > -\n>> > - sizes.erase(++cur, sizes.end());\n>> > + /* Sort the sizes and merge any consecutive overlapping ranges. */\n>> > + for (auto &[format, sizes] : formats) {\n>> > + std::sort(sizes.begin(), sizes.end(),\n>> > + [](SizeRange &a, SizeRange &b) {\n>> > + return a.min < b.min;\n>> > + });\n>> > +\n>> > + auto cur = sizes.begin();\n>> > + auto next = cur;\n>> > +\n>> > + while (++next != sizes.end()) {\n>> > + if (cur->max.width >= next->min.width &&\n>> > + cur->max.height >= next->min.height)\n>> > + cur->max = next->max;\n>> > + else if (++cur != next)\n>> > + *cur = *next;\n>> > }\n>> > - };\n>> > - setUpFormatSizes(processedFormats);\n>> > - setUpFormatSizes(rawFormats);\n>> > +\n>> > + sizes.erase(++cur, sizes.end());\n>> > + }\n>\n> IMO these are also some major difference implementation-wise here as\n> well, but towards a common goal.\n\nYes.\n\n>> > \n>> > /*\n>> > * Create the stream configurations. Take the first entry in the formats\n>> > - * map as the default, for lack of a better option.\n>> > + * map as the default for each of role (raw or non-raw), for lack of a\n>> > + * better option.\n>> > *\n>> > * \\todo Implement a better way to pick the default format\n>> > */\n>> > for (StreamRole role : roles) {\n>> > - const auto &formats = (role == StreamRole::Raw ? rawFormats : processedFormats);\n>> > StreamConfiguration cfg{ StreamFormats{ formats } };\n>> > - cfg.pixelFormat = formats.begin()->first;\n>> > - cfg.size = formats.begin()->second[0].max;\n>> > +\n>> > + switch (role) {\n>> > + case StreamRole::Raw:\n>> > + for (auto &[format, sizes] : formats) {\n>> > + if (!isFormatRaw(format))\n>> > + continue;\n>> > + cfg.pixelFormat = format;\n>> > + cfg.size = sizes.back().max;\n>> > + cfg.colorSpace = ColorSpace::Raw;\n>> > + break;\n>> > + }\n>> > + break;\n>> > + default:\n>> > + for (auto &[format, sizes] : formats) {\n>> > + if (isFormatRaw(format))\n>> > + continue;\n>> > + cfg.pixelFormat = format;\n>> > + cfg.size = sizes[0].max;\n>> > + }\n>> > + }\n>> \n>> In my version, colour space handling is now only in validate(). The\n>> pixel format and size selection are similar in both the versions.\n>> \n>> The rest of the diff are only cosmetic differences.\n>> \n>> > \n>> > config->addConfiguration(cfg);\n>> > }\n>> > @@ -1532,7 +1482,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n>> > \n>> > for (unsigned int i = 0; i < config->size(); ++i) {\n>> > StreamConfiguration &cfg = config->at(i);\n>> > - bool rawStream = isRaw(cfg);\n>> > + bool rawStream = isFormatRaw(cfg.pixelFormat);\n>> > \n>> > cfg.setStream(&data->streams_[i]);\n>> > \n>> > @@ -1571,7 +1521,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n>> > * Export buffers on the converter or capture video node, depending on\n>> > * whether the converter is used or not.\n>> > */\n>> > - if (data->useConversion_ && stream != data->rawStream_)\n>> > + if (data->useConversion_ && (stream != data->rawStream_))\n>> > return data->converter_\n>> > ? data->converter_->exportBuffers(stream, count, buffers)\n>> > : data->swIsp_->exportBuffers(stream, count, buffers);\n>> > @@ -1593,7 +1543,6 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n>> > << pad->entity()->name() << \" in use\";\n>> > return -EBUSY;\n>> > }\n>> > -\n>> > if (data->useConversion_ && !data->rawStream_) {\n>> > /*\n>> > * When using the converter allocate a fixed number of internal\n>> > @@ -1603,10 +1552,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n>> > &data->conversionBuffers_);\n>> > } else {\n>> > /*\n>> > - * Otherwise, prepare for using buffers from either the raw stream, if\n>> > - * requested, or the only stream configured.\n>> > + * Otherwise, prepare for using buffers from either the raw\n>> > + * stream((if requested) or the only stream configured.\n>> > */\n>> > - Stream *stream = (data->rawStream_ ? data->rawStream_ : &data->streams_[0]);\n>> > + Stream *stream;\n>> > + if (data->rawStream_)\n>> > + stream = data->rawStream_;\n>> > + else\n>> > + stream = &data->streams_[0];\n>> > +\n>> > ret = video->importBuffers(stream->configuration().bufferCount);\n>> > }\n>> > if (ret < 0) {\n>> > @@ -1647,9 +1601,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n>> > }\n>> > \n>> > /* Queue all internal buffers for capture. */\n>> > - if (!data->rawStream_)\n>> > + if (!data->rawStream_) {\n>> > for (std::unique_ptr &buffer : data->conversionBuffers_)\n>> > video->queueBuffer(buffer.get());\n>> > + }\n>> > }\n>> > \n>> > return 0;\n>> > @@ -1700,7 +1655,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n>> > * queue, it will be handed to the converter in the capture\n>> > * completion handler.\n>> > */\n>> > - if (data->useConversion_ && stream != data->rawStream_) {\n>> > + if (data->useConversion_ && (stream != data->rawStream_)) {\n>> > buffers.emplace(stream, buffer);\n>> > metadataRequired = !!data->swIsp_;\n>> > } else {\n>> > @@ -1836,13 +1791,13 @@ bool SimplePipelineHandler::matchDevice(MediaDevice *media,\n>> > /*\n>> > * When the software ISP is enabled, the simple pipeline handler\n>> > * exposes the raw stream, giving a total of two streams. This\n>> > - * is mutually exclusive with the presence of a converter.\n>> > + * is mutally exclusive with the presence of a converter.\n>> > */\n>> > ASSERT(!converter_);\n>> > numStreams = 2;\n>> > + swIspEnabled_ = true;\n>> > }\n>> > \n>> > - swIspEnabled_ = info.swIspEnabled;\n>> > const GlobalConfiguration &configuration = cameraManager()->_d()->configuration();\n>> > for (GlobalConfiguration::Configuration entry :\n>> > configuration.configuration()[\"pipelines\"][\"simple\"][\"supported_devices\"]\n>>","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 113C1C3241\n\tfor ;\n\tSat, 1 Nov 2025 21:15:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3DAFF6086F;\n\tSat, 1 Nov 2025 22:15:46 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93776606D5\n\tfor ;\n\tSat, 1 Nov 2025 22:15:43 +0100 (CET)","from mail-wm1-f70.google.com (mail-wm1-f70.google.com\n\t[209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-447-R6LZdXMAPPSUMGanB7qkoA-1; Sat, 01 Nov 2025 15:30:44 -0400","by mail-wm1-f70.google.com with SMTP id\n\t5b1f17b1804b1-477113a50fcso24077435e9.1\n\tfor ;\n\tSat, 01 Nov 2025 12:30:44 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-4773c2e677csm63895505e9.3.2025.11.01.12.30.40\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 01 Nov 2025 12:30:41 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"ReFITxQf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1762031742;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=7yCTUd3daS6H/W3nYMgKluo0jCPHObmz8Wdv+P0j4us=;\n\tb=ReFITxQfwOSugrg/Yg33RbBY9hmvo3+TUV2kmnb0cJMLcgQ+4AYs6YaZ/tcOfK7o3bCuGT\n\t5BxnEvz34GbD20z7VFpwYB4aG2G8dvIr6ZlCkSanunmrHyScq6qEIlcrU/vmXn8ARSHbHj\n\tjV0LQeks9gwpdVisbaobslAs85y77Lg=","X-MC-Unique":"R6LZdXMAPPSUMGanB7qkoA-1","X-Mimecast-MFC-AGG-ID":"R6LZdXMAPPSUMGanB7qkoA_1762025443","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1762025443; x=1762630243;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=7yCTUd3daS6H/W3nYMgKluo0jCPHObmz8Wdv+P0j4us=;\n\tb=rdyduQeR1eNN2OM2yydrdOHEL7xljnLAE0IrfeRrK/MGIu42rZeJx/9+H1/RxNxYdD\n\tPvYKp+6dCYvfMXvOj0usnXwP90puLwnLoiYP84dTY2u7u/GGtcFmaaTft3s9jz7ZllOM\n\tgDbQQ3EKx4GANlalnYScZO873tT0aDw4gouiu9fG4rqlnHvDH+MQgmX4IeIjTUZpfeor\n\te84tcdpJpIJGIbomeyV7jJ+jHPbJmwww50A9f2XZtZ053InM6mdrDwKETFzlB3cepPie\n\tNzUr4lyPXOF99764BnxAplMXnqc71Smf+BHLCL3no8PA4l9sE3tV6PDp27nSM0Nd2ZjP\n\tdrkg==","X-Gm-Message-State":"AOJu0Yx3KJoYdmA4c/u3hyacGuK2sy6sN4oySi3ok0hCFdmbuD1jD09J\n\t3aZ4QES3IEAaufxw28kIrVTEOlTPUUQqcHHeXpM6Z1mPbrcsxgr+ug6h7xoUh+F4fb+Z5wd2yOQ\n\tsKJbZMrI+chQZ/p0xUch5VNZoRcUjotheVuWdoOA0UDfr1Y0pf/h+LS8TvTYB8bZ1Kv6wnPXnlg\n\tc=","X-Gm-Gg":"ASbGncvwPTYiYPlplYPEL8tPKa8RaBredIyUfqOu7fgnLkGrljt4Rjqd4/s3Biht8P+\n\tAqfusW+6b1hC/PVKxSjmh4F02dJ0IPjTH4K6YNIgKgiSPzff2+zLt6wUYmEPrUDWLi+zrkGjP/t\n\teLwvvUGVqWRu1bfedAzvRWV9MZb1ELlkAq85V8Usoc/Ie2Pqot5Mvy3VHvciM5iubIGq6Rxx2H2\n\tGk+RsJ6gbjPpQ2lfmPte4fJtxwVjdtVxv5GaIDNrs7pcPBuivW1hr0PhspRET+/YkH0BwJB6aqK\n\t4JFkCAkJ+HCivn/8pGz0KYnMTKl4tequ8+ILqPOOIwek8cmxvsicEE3U5tlUxH7nZJ0jtapt7SL\n\tO86CD0qcck3QJZCXF3PktfQUpcojXQvgH73vyKgCiAbl/DqRXWK6g","X-Received":["by 2002:a05:600c:4e52:b0:475:dae5:d972 with SMTP id\n\t5b1f17b1804b1-4773089e478mr73625385e9.23.1762025442780; \n\tSat, 01 Nov 2025 12:30:42 -0700 (PDT)","by 2002:a05:600c:4e52:b0:475:dae5:d972 with SMTP id\n\t5b1f17b1804b1-4773089e478mr73625125e9.23.1762025442100; \n\tSat, 01 Nov 2025 12:30:42 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IH+M7pPkUoGkSPY+43SRdjm4pLjt7Sv3HwNg3O0ji7d1jYkuQrqC0MwpOlN//i1xWaCEVnfkQ==","From":"Milan Zamazal ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org, Laurent Pinchart\n\t,\n\tKieran Bingham , =?utf-8?q?Barnab?=\n\t=?utf-8?b?w6FzIFDFkWN6ZQ==?=\n\t, Paul Elder\n\t","Subject":"Re: [PATCH v13 0/8] Enable raw streams with software ISP","In-Reply-To":"<5kjtvmsrtwjxuffle7u4yxj3wkwqsr7d5hejt6icufkr765nm3@daxvltodcrny>\n\t(Umang Jain's message of \"Wed, 22 Oct 2025 17:24:25 +0530\")","References":"<20251021182716.29274-1-mzamazal@redhat.com>\n\t<85sefcgl02.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<5kjtvmsrtwjxuffle7u4yxj3wkwqsr7d5hejt6icufkr765nm3@daxvltodcrny>","Date":"Sat, 01 Nov 2025 20:30:39 +0100","Message-ID":"<85zf95sg80.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"wJ_V16zX_bP4GGdQGV-Q3s-o5SZDiitlWKqtGWyeKJ8_1762025443","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]