Message ID | 20251021182716.29274-1-mzamazal@redhat.com |
---|---|
Headers | show |
Series |
|
Related | show |
There is another branch of this series by Umang. Since there was no preference expressed which branch to use for the next version, I continue with mine. While I tried to incorporate some changes from Umang's branch previously, some differences remain, which I think require more discussion. I rebased Umang's branch on the same master version, made a diff between the two branches and commented on it below. The diff shows an update of the posted patches to Umang's branch, i.e. the original version in the diff is this v13, the new version is the rebased Umang's branch. > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 607b07949..529d7344d 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -11,7 +11,6 @@ > #include <list> > #include <map> > #include <memory> > -#include <optional> > #include <queue> > #include <set> > #include <stdint.h> > @@ -26,9 +25,7 @@ > #include <libcamera/base/log.h> > > #include <libcamera/camera.h> > -#include <libcamera/color_space.h> > #include <libcamera/control_ids.h> > -#include <libcamera/geometry.h> > #include <libcamera/pixel_format.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > @@ -40,7 +37,6 @@ > #include "libcamera/internal/converter.h" > #include "libcamera/internal/delayed_controls.h" > #include "libcamera/internal/device_enumerator.h" > -#include "libcamera/internal/formats.h" > #include "libcamera/internal/global_configuration.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/pipeline_handler.h" > @@ -267,9 +263,9 @@ static const SimplePipelineInfo supportedDevices[] = { > { "sun6i-csi", {}, false }, > }; > > -bool isRaw(const StreamConfiguration &cfg) > +bool isFormatRaw(const libcamera::PixelFormat &pixFmt) isFormatRaw -> isRaw + argument type change was done in v12. Umang's version uses in one (different) place format that is not stored in the configuration. > { > - return libcamera::PixelFormatInfo::info(cfg.pixelFormat).colourEncoding == > + return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding == > libcamera::PixelFormatInfo::ColourEncodingRAW; > } > > @@ -372,11 +368,6 @@ private: > void ispStatsReady(uint32_t frame, uint32_t bufferId); > void metadataReady(uint32_t frame, const ControlList &metadata); > void setSensorControls(const ControlList &sensorControls); A significant difference between the two versions is the buffer handling. My version distinguishes between raw/non-raw cases at all the particular places: > - > - bool processedRequested() const > - { > - return streams_.size() - (rawStream_ ? 1 : 0) > 0; > - } > }; > > class SimpleCameraConfiguration : public CameraConfiguration > @@ -884,13 +875,10 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > * point converting an erroneous buffer. > */ > if (buffer->metadata().status != FrameMetadata::FrameSuccess) { > - if (rawStream_) { > + if (!useConversion_) { > /* No conversion, just complete the request. */ > Request *request = buffer->request(); > pipe->completeBuffer(request, buffer); > - SimpleFrameInfo *info = frameInfo_.find(request->sequence()); > - if (info) > - info->metadataRequired = false; > tryCompleteRequest(request); > return; > } > @@ -900,8 +888,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > * buffer for capture (unless the stream is being stopped), and > * complete the request with all the user-facing buffers. > */ > - if (buffer->metadata().status != FrameMetadata::FrameCancelled && > - !rawStream_) > + if (buffer->metadata().status != FrameMetadata::FrameCancelled) > video_->queueBuffer(buffer); > > if (conversionQueue_.empty()) > @@ -957,7 +944,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > if (converter_) > converter_->queueBuffers(buffer, conversionQueue_.front().outputs); > - else if (processedRequested()) > + else > /* > * request->sequence() cannot be retrieved from `buffer' inside > * queueBuffers because unique_ptr's make buffer->request() invalid > @@ -967,8 +954,6 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > conversionQueue_.front().outputs); > > conversionQueue_.pop(); > - if (rawStream_) > - pipe->completeBuffer(request, buffer); > return; > } > > @@ -1006,8 +991,14 @@ void SimpleCameraData::tryCompleteRequest(Request *request) > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) > { > /* Queue the input buffer back for capture. */ > - if (!rawStream_) > + if (!rawStream_) { > video_->queueBuffer(buffer); While Umang's version has this final buffer completion: > + } else { > + /* Complete the input buffer. */ > + Request *request = buffer->request(); > + if (pipe()->completeBuffer(request, buffer)) > + tryCompleteRequest(request); > + } > } > > void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) > @@ -1156,31 +1147,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > LOG(SimplePipeline, Debug) > << "Largest stream size is " << maxStreamSize; > > + /* Cap the number of raw stream configuration */ > + unsigned int rawCount = 0; > + PixelFormat requestedRawFormat; > + for (const StreamConfiguration &cfg : config_) { > + if (!isFormatRaw(cfg.pixelFormat)) > + continue; > + requestedRawFormat = cfg.pixelFormat; > + rawCount++; > + } > + > + if (rawCount > 1) { > + LOG(SimplePipeline, Error) > + << "Camera configuration with " > + << rawCount << " raw streams not supported"; > + return Invalid; > + } > + This differs in how to check for multiple raw streams in validate() and how to select the config but doesn't seem to do something very different. > /* > - * Find the best configuration for the pipeline using a heuristic. First > - * select the pixel format based on the streams. If there is a raw stream, > - * its format has precedence. If there is no raw stream, the streams are > - * considered ordered from highest to lowest priority. Default to the first > - * pipeline configuration if no streams request a supported pixel format. > + * Find the best configuration for the pipeline using a heuristic. > + * First select the pixel format based on the raw streams followed by > + * non-raw streams (which are considered ordered from highest to lowest > + * priority). Default to the first pipeline configuration if no streams > + * request a supported pixel format. > */ > - std::optional<PixelFormat> rawFormat; > - for (const auto &cfg : config_) > - if (isRaw(cfg)) { > - if (rawFormat) { > - LOG(SimplePipeline, Error) > - << "Can't capture multiple raw streams"; > - return Invalid; > - } > - rawFormat = cfg.pixelFormat; > - } > + const std::vector<const SimpleCameraData::Configuration *> *configs = > + &data_->formats_.begin()->second; > > - const std::vector<const SimpleCameraData::Configuration *> *configs = nullptr; > - if (rawFormat) { > - auto it = data_->formats_.find(rawFormat.value()); > - if (it != data_->formats_.end()) > - configs = &it->second; > - } > - if (!configs) > + auto rawIter = data_->formats_.find(requestedRawFormat); > + if (rawIter != data_->formats_.end()) { > + configs = &rawIter->second; > + } else { > for (const StreamConfiguration &cfg : config_) { > auto it = data_->formats_.find(cfg.pixelFormat); > if (it != data_->formats_.end()) { > @@ -1188,8 +1185,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > break; > } > } > - if (!configs) > - configs = &data_->formats_.begin()->second; > + } > > /* > * \todo Pick the best sensor output media bus format when the > @@ -1242,63 +1238,39 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > * require any conversion, similar to raw capture use cases). This is > * left as a future improvement. > */ > - needConversion_ = config_.size() > 1 + (rawFormat ? 1 : 0); > + needConversion_ = config_.size() > 1 + rawCount; > > for (unsigned int i = 0; i < config_.size(); ++i) { > StreamConfiguration &cfg = config_[i]; > - const bool raw = isRaw(cfg); > + const bool raw = isFormatRaw(cfg.pixelFormat); > > - /* Adjust the pixel format, colour space and size. */ > - > - PixelFormat pixelFormat; > + /* Adjust the raw pixel format and size. */ > if (raw) { > - pixelFormat = pipeConfig_->captureFormat; > - } else { > - auto it = std::find(pipeConfig_->outputFormats.begin(), > - pipeConfig_->outputFormats.end(), > - cfg.pixelFormat); > - if (it == pipeConfig_->outputFormats.end()) > - it = pipeConfig_->outputFormats.begin(); > - pixelFormat = *it; > - } > - if (cfg.pixelFormat != pixelFormat) { > - LOG(SimplePipeline, Debug) > - << "Adjusting pixel format of a " > - << (raw ? "raw" : "processed") > - << " stream from " << cfg.pixelFormat > - << " to " << pixelFormat; > - cfg.pixelFormat = pixelFormat; > - status = Adjusted; > - } > + if (cfg.pixelFormat != pipeConfig_->captureFormat || > + cfg.size != pipeConfig_->captureSize) { > + cfg.pixelFormat = pipeConfig_->captureFormat; > + cfg.size = pipeConfig_->captureSize; > > - /* > - * Best effort to fix the color space. If the color space is not set, > - * set it according to the pixel format, which may not be correct (pixel > - * formats and color spaces are different things, although somewhat > - * related) but we don't have a better option at the moment. Then in any > - * case, perform the standard pixel format based color space adjustment. > - */ > - if (!cfg.colorSpace) { > - const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat); > - switch (info.colourEncoding) { > - case PixelFormatInfo::ColourEncodingRGB: > - cfg.colorSpace = ColorSpace::Srgb; > - break; > - case libcamera::PixelFormatInfo::ColourEncodingYUV: > - cfg.colorSpace = ColorSpace::Sycc; > - break; > - default: > - cfg.colorSpace = ColorSpace::Raw; > + LOG(SimplePipeline, Debug) > + << "Adjusting raw stream to " > + << cfg.toString(); > + status = Adjusted; > } > - LOG(SimplePipeline, Debug) > - << "Unspecified color space set to " > - << cfg.colorSpace.value().toString(); > - status = Adjusted; > } > - if (cfg.colorSpace->adjust(pixelFormat)) { The colour space adjustment is the primary difference in this diff area. In Umang's version, it's still handled when processing stream roles and it's missing here in validate(). The rest is more similar than the size of the diff output might suggest. > + > + /* Adjust the non-raw pixel format and size. */ > + auto it = std::find(pipeConfig_->outputFormats.begin(), > + pipeConfig_->outputFormats.end(), > + cfg.pixelFormat); > + if (it == pipeConfig_->outputFormats.end()) > + it = pipeConfig_->outputFormats.begin(); > + > + PixelFormat pixelFormat = *it; > + if (!raw && cfg.pixelFormat != pixelFormat) { > LOG(SimplePipeline, Debug) > - << "Color space adjusted to " > - << cfg.colorSpace.value().toString(); > + << "Adjusting pixel format from " > + << cfg.pixelFormat << " to " << pixelFormat; > + cfg.pixelFormat = pixelFormat; > status = Adjusted; > } > > @@ -1321,20 +1293,11 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > /* \todo Create a libcamera core class to group format and size */ > if (cfg.pixelFormat != pipeConfig_->captureFormat || > - cfg.size != pipeConfig_->captureSize) { > - if (raw) { > - cfg.pixelFormat = pipeConfig_->captureFormat; > - cfg.size = pipeConfig_->captureSize; > - LOG(SimplePipeline, Debug) > - << "Adjusting raw configuration to " << cfg; > - status = Adjusted; > - } else { > - needConversion_ = true; > - } > - } > + cfg.size != pipeConfig_->captureSize) > + needConversion_ = true; > > /* Set the stride and frameSize. */ > - if (needConversion_ && !raw) { > + if (needConversion_) { > std::tie(cfg.stride, cfg.frameSize) = > data_->converter_ > ? data_->converter_->strideAndFrameSize(cfg.pixelFormat, > @@ -1379,7 +1342,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager) > : PipelineHandler(manager, kMaxQueuedRequestsDevice), > - converter_(nullptr) > + converter_(nullptr), > + swIspEnabled_(false) > { > } > > @@ -1393,78 +1357,64 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo > if (roles.empty()) > return config; > > - bool processedRequested = false; > - bool rawRequested = false; > - for (const auto &role : roles) > - if (role == StreamRole::Raw) { > - if (rawRequested) { > - LOG(SimplePipeline, Error) > - << "Can't capture multiple raw streams"; > - return nullptr; > - } > - rawRequested = true; > - } else { > - processedRequested = true; > - } IIRC Umang suggested that checking for this in validate() is enough and it's not necessary to have a sort of duplicate check in generateConfiguration(). The rest here is similar in both the versions in what it does. > - > - /* Create the formats maps. */ > - std::map<PixelFormat, std::vector<SizeRange>> processedFormats; > - std::map<PixelFormat, std::vector<SizeRange>> rawFormats; > - > - for (const SimpleCameraData::Configuration &cfg : data->configs_) { > - rawFormats[cfg.captureFormat].push_back(cfg.captureSize); > - for (PixelFormat format : cfg.outputFormats) > - processedFormats[format].push_back(cfg.outputSizes); > - } > + /* Create the formats map. */ > + std::map<PixelFormat, std::vector<SizeRange>> formats; > > - if (processedRequested && processedFormats.empty()) { > - LOG(SimplePipeline, Error) > - << "Processed stream requsted but no corresponding output configuration found"; > - return nullptr; > - } > - if (rawRequested && rawFormats.empty()) { > - LOG(SimplePipeline, Error) > - << "Raw stream requsted but no corresponding output configuration found"; > - return nullptr; These checks are not present in Umang's version. > + for (const auto &it : data->formats_) { > + for (const SimpleCameraData::Configuration *cfg : it.second) > + formats[it.first].push_back(cfg->outputSizes); > } > > - auto setUpFormatSizes = [](std::map<PixelFormat, std::vector<SizeRange>> &formats) { > - /* Sort the sizes and merge any consecutive overlapping ranges. */ > - > - for (auto &[format, sizes] : formats) { > - std::sort(sizes.begin(), sizes.end(), > - [](SizeRange &a, SizeRange &b) { > - return a.min < b.min; > - }); > - > - auto cur = sizes.begin(); > - auto next = cur; > - > - while (++next != sizes.end()) { > - if (cur->max.width >= next->min.width && > - cur->max.height >= next->min.height) > - cur->max = next->max; > - else if (++cur != next) > - *cur = *next; > - } > - > - sizes.erase(++cur, sizes.end()); > + /* Sort the sizes and merge any consecutive overlapping ranges. */ > + for (auto &[format, sizes] : formats) { > + std::sort(sizes.begin(), sizes.end(), > + [](SizeRange &a, SizeRange &b) { > + return a.min < b.min; > + }); > + > + auto cur = sizes.begin(); > + auto next = cur; > + > + while (++next != sizes.end()) { > + if (cur->max.width >= next->min.width && > + cur->max.height >= next->min.height) > + cur->max = next->max; > + else if (++cur != next) > + *cur = *next; > } > - }; > - setUpFormatSizes(processedFormats); > - setUpFormatSizes(rawFormats); > + > + sizes.erase(++cur, sizes.end()); > + } > > /* > * Create the stream configurations. Take the first entry in the formats > - * map as the default, for lack of a better option. > + * map as the default for each of role (raw or non-raw), for lack of a > + * better option. > * > * \todo Implement a better way to pick the default format > */ > for (StreamRole role : roles) { > - const auto &formats = (role == StreamRole::Raw ? rawFormats : processedFormats); > StreamConfiguration cfg{ StreamFormats{ formats } }; > - cfg.pixelFormat = formats.begin()->first; > - cfg.size = formats.begin()->second[0].max; > + > + switch (role) { > + case StreamRole::Raw: > + for (auto &[format, sizes] : formats) { > + if (!isFormatRaw(format)) > + continue; > + cfg.pixelFormat = format; > + cfg.size = sizes.back().max; > + cfg.colorSpace = ColorSpace::Raw; > + break; > + } > + break; > + default: > + for (auto &[format, sizes] : formats) { > + if (isFormatRaw(format)) > + continue; > + cfg.pixelFormat = format; > + cfg.size = sizes[0].max; > + } > + } In my version, colour space handling is now only in validate(). The pixel format and size selection are similar in both the versions. The rest of the diff are only cosmetic differences. > > config->addConfiguration(cfg); > } > @@ -1532,7 +1482,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c) > > for (unsigned int i = 0; i < config->size(); ++i) { > StreamConfiguration &cfg = config->at(i); > - bool rawStream = isRaw(cfg); > + bool rawStream = isFormatRaw(cfg.pixelFormat); > > cfg.setStream(&data->streams_[i]); > > @@ -1571,7 +1521,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream, > * Export buffers on the converter or capture video node, depending on > * whether the converter is used or not. > */ > - if (data->useConversion_ && stream != data->rawStream_) > + if (data->useConversion_ && (stream != data->rawStream_)) > return data->converter_ > ? data->converter_->exportBuffers(stream, count, buffers) > : data->swIsp_->exportBuffers(stream, count, buffers); > @@ -1593,7 +1543,6 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > << pad->entity()->name() << " in use"; > return -EBUSY; > } > - > if (data->useConversion_ && !data->rawStream_) { > /* > * When using the converter allocate a fixed number of internal > @@ -1603,10 +1552,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > &data->conversionBuffers_); > } else { > /* > - * Otherwise, prepare for using buffers from either the raw stream, if > - * requested, or the only stream configured. > + * Otherwise, prepare for using buffers from either the raw > + * stream((if requested) or the only stream configured. > */ > - Stream *stream = (data->rawStream_ ? data->rawStream_ : &data->streams_[0]); > + Stream *stream; > + if (data->rawStream_) > + stream = data->rawStream_; > + else > + stream = &data->streams_[0]; > + > ret = video->importBuffers(stream->configuration().bufferCount); > } > if (ret < 0) { > @@ -1647,9 +1601,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > } > > /* Queue all internal buffers for capture. */ > - if (!data->rawStream_) > + if (!data->rawStream_) { > for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) > video->queueBuffer(buffer.get()); > + } > } > > return 0; > @@ -1700,7 +1655,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > * queue, it will be handed to the converter in the capture > * completion handler. > */ > - if (data->useConversion_ && stream != data->rawStream_) { > + if (data->useConversion_ && (stream != data->rawStream_)) { > buffers.emplace(stream, buffer); > metadataRequired = !!data->swIsp_; > } else { > @@ -1836,13 +1791,13 @@ bool SimplePipelineHandler::matchDevice(MediaDevice *media, > /* > * When the software ISP is enabled, the simple pipeline handler > * exposes the raw stream, giving a total of two streams. This > - * is mutually exclusive with the presence of a converter. > + * is mutally exclusive with the presence of a converter. > */ > ASSERT(!converter_); > numStreams = 2; > + swIspEnabled_ = true; > } > > - swIspEnabled_ = info.swIspEnabled; > const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); > for (GlobalConfiguration::Configuration entry : > configuration.configuration()["pipelines"]["simple"]["supported_devices"]
Quoting Milan Zamazal (2025-10-21 19:36:29) > There is another branch of this series by Umang. Since there was no > preference expressed which branch to use for the next version, I > continue with mine. > > While I tried to incorporate some changes from Umang's branch > previously, some differences remain, which I think require more > discussion. I rebased Umang's branch on the same master version, made a > diff between the two branches and commented on it below. The diff shows > an update of the posted patches to Umang's branch, i.e. the original > version in the diff is this v13, the new version is the rebased Umang's > branch. Thanks, I had to do a few updates to camshark to test this series. The fixes are available at: - https://gitlab.freedesktop.org/camera/camshark/-/merge_requests/14 But for this series at least - I can capture and view the raws. Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 607b07949..529d7344d 100644 > > --- a/src/libcamera/pipeline/simple/simple.cpp > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > @@ -11,7 +11,6 @@ > > #include <list> > > #include <map> > > #include <memory> > > -#include <optional> > > #include <queue> > > #include <set> > > #include <stdint.h> > > @@ -26,9 +25,7 @@ > > #include <libcamera/base/log.h> > > > > #include <libcamera/camera.h> > > -#include <libcamera/color_space.h> > > #include <libcamera/control_ids.h> > > -#include <libcamera/geometry.h> > > #include <libcamera/pixel_format.h> > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > @@ -40,7 +37,6 @@ > > #include "libcamera/internal/converter.h" > > #include "libcamera/internal/delayed_controls.h" > > #include "libcamera/internal/device_enumerator.h" > > -#include "libcamera/internal/formats.h" > > #include "libcamera/internal/global_configuration.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/pipeline_handler.h" > > @@ -267,9 +263,9 @@ static const SimplePipelineInfo supportedDevices[] = { > > { "sun6i-csi", {}, false }, > > }; > > > > -bool isRaw(const StreamConfiguration &cfg) > > +bool isFormatRaw(const libcamera::PixelFormat &pixFmt) > > isFormatRaw -> isRaw + argument type change was done in v12. Umang's > version uses in one (different) place format that is not stored in the > configuration. > > > { > > - return libcamera::PixelFormatInfo::info(cfg.pixelFormat).colourEncoding == > > + return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding == > > libcamera::PixelFormatInfo::ColourEncodingRAW; > > } > > > > @@ -372,11 +368,6 @@ private: > > void ispStatsReady(uint32_t frame, uint32_t bufferId); > > void metadataReady(uint32_t frame, const ControlList &metadata); > > void setSensorControls(const ControlList &sensorControls); > > A significant difference between the two versions is the buffer > handling. > > My version distinguishes between raw/non-raw cases at all the particular > places: > > > - > > - bool processedRequested() const > > - { > > - return streams_.size() - (rawStream_ ? 1 : 0) > 0; > > - } > > }; > > > > class SimpleCameraConfiguration : public CameraConfiguration > > @@ -884,13 +875,10 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > * point converting an erroneous buffer. > > */ > > if (buffer->metadata().status != FrameMetadata::FrameSuccess) { > > - if (rawStream_) { > > + if (!useConversion_) { > > /* No conversion, just complete the request. */ > > Request *request = buffer->request(); > > pipe->completeBuffer(request, buffer); > > - SimpleFrameInfo *info = frameInfo_.find(request->sequence()); > > - if (info) > > - info->metadataRequired = false; > > tryCompleteRequest(request); > > return; > > } > > @@ -900,8 +888,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > * buffer for capture (unless the stream is being stopped), and > > * complete the request with all the user-facing buffers. > > */ > > - if (buffer->metadata().status != FrameMetadata::FrameCancelled && > > - !rawStream_) > > + if (buffer->metadata().status != FrameMetadata::FrameCancelled) > > video_->queueBuffer(buffer); > > > > if (conversionQueue_.empty()) > > @@ -957,7 +944,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > > > if (converter_) > > converter_->queueBuffers(buffer, conversionQueue_.front().outputs); > > - else if (processedRequested()) > > + else > > /* > > * request->sequence() cannot be retrieved from `buffer' inside > > * queueBuffers because unique_ptr's make buffer->request() invalid > > @@ -967,8 +954,6 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > conversionQueue_.front().outputs); > > > > conversionQueue_.pop(); > > - if (rawStream_) > > - pipe->completeBuffer(request, buffer); > > return; > > } > > > > @@ -1006,8 +991,14 @@ void SimpleCameraData::tryCompleteRequest(Request *request) > > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) > > { > > /* Queue the input buffer back for capture. */ > > - if (!rawStream_) > > + if (!rawStream_) { > > video_->queueBuffer(buffer); > > While Umang's version has this final buffer completion: > > > + } else { > > + /* Complete the input buffer. */ > > + Request *request = buffer->request(); > > + if (pipe()->completeBuffer(request, buffer)) > > + tryCompleteRequest(request); > > + } > > } > > > > void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) > > @@ -1156,31 +1147,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > LOG(SimplePipeline, Debug) > > << "Largest stream size is " << maxStreamSize; > > > > + /* Cap the number of raw stream configuration */ > > + unsigned int rawCount = 0; > > + PixelFormat requestedRawFormat; > > + for (const StreamConfiguration &cfg : config_) { > > + if (!isFormatRaw(cfg.pixelFormat)) > > + continue; > > + requestedRawFormat = cfg.pixelFormat; > > + rawCount++; > > + } > > + > > + if (rawCount > 1) { > > + LOG(SimplePipeline, Error) > > + << "Camera configuration with " > > + << rawCount << " raw streams not supported"; > > + return Invalid; > > + } > > + > > This differs in how to check for multiple raw streams in validate() and > how to select the config but doesn't seem to do something very > different. > > > /* > > - * Find the best configuration for the pipeline using a heuristic. First > > - * select the pixel format based on the streams. If there is a raw stream, > > - * its format has precedence. If there is no raw stream, the streams are > > - * considered ordered from highest to lowest priority. Default to the first > > - * pipeline configuration if no streams request a supported pixel format. > > + * Find the best configuration for the pipeline using a heuristic. > > + * First select the pixel format based on the raw streams followed by > > + * non-raw streams (which are considered ordered from highest to lowest > > + * priority). Default to the first pipeline configuration if no streams > > + * request a supported pixel format. > > */ > > - std::optional<PixelFormat> rawFormat; > > - for (const auto &cfg : config_) > > - if (isRaw(cfg)) { > > - if (rawFormat) { > > - LOG(SimplePipeline, Error) > > - << "Can't capture multiple raw streams"; > > - return Invalid; > > - } > > - rawFormat = cfg.pixelFormat; > > - } > > + const std::vector<const SimpleCameraData::Configuration *> *configs = > > + &data_->formats_.begin()->second; > > > > - const std::vector<const SimpleCameraData::Configuration *> *configs = nullptr; > > - if (rawFormat) { > > - auto it = data_->formats_.find(rawFormat.value()); > > - if (it != data_->formats_.end()) > > - configs = &it->second; > > - } > > - if (!configs) > > + auto rawIter = data_->formats_.find(requestedRawFormat); > > + if (rawIter != data_->formats_.end()) { > > + configs = &rawIter->second; > > + } else { > > for (const StreamConfiguration &cfg : config_) { > > auto it = data_->formats_.find(cfg.pixelFormat); > > if (it != data_->formats_.end()) { > > @@ -1188,8 +1185,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > break; > > } > > } > > - if (!configs) > > - configs = &data_->formats_.begin()->second; > > + } > > > > /* > > * \todo Pick the best sensor output media bus format when the > > @@ -1242,63 +1238,39 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > * require any conversion, similar to raw capture use cases). This is > > * left as a future improvement. > > */ > > - needConversion_ = config_.size() > 1 + (rawFormat ? 1 : 0); > > + needConversion_ = config_.size() > 1 + rawCount; > > > > for (unsigned int i = 0; i < config_.size(); ++i) { > > StreamConfiguration &cfg = config_[i]; > > - const bool raw = isRaw(cfg); > > + const bool raw = isFormatRaw(cfg.pixelFormat); > > > > - /* Adjust the pixel format, colour space and size. */ > > - > > - PixelFormat pixelFormat; > > + /* Adjust the raw pixel format and size. */ > > if (raw) { > > - pixelFormat = pipeConfig_->captureFormat; > > - } else { > > - auto it = std::find(pipeConfig_->outputFormats.begin(), > > - pipeConfig_->outputFormats.end(), > > - cfg.pixelFormat); > > - if (it == pipeConfig_->outputFormats.end()) > > - it = pipeConfig_->outputFormats.begin(); > > - pixelFormat = *it; > > - } > > - if (cfg.pixelFormat != pixelFormat) { > > - LOG(SimplePipeline, Debug) > > - << "Adjusting pixel format of a " > > - << (raw ? "raw" : "processed") > > - << " stream from " << cfg.pixelFormat > > - << " to " << pixelFormat; > > - cfg.pixelFormat = pixelFormat; > > - status = Adjusted; > > - } > > + if (cfg.pixelFormat != pipeConfig_->captureFormat || > > + cfg.size != pipeConfig_->captureSize) { > > + cfg.pixelFormat = pipeConfig_->captureFormat; > > + cfg.size = pipeConfig_->captureSize; > > > > - /* > > - * Best effort to fix the color space. If the color space is not set, > > - * set it according to the pixel format, which may not be correct (pixel > > - * formats and color spaces are different things, although somewhat > > - * related) but we don't have a better option at the moment. Then in any > > - * case, perform the standard pixel format based color space adjustment. > > - */ > > - if (!cfg.colorSpace) { > > - const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat); > > - switch (info.colourEncoding) { > > - case PixelFormatInfo::ColourEncodingRGB: > > - cfg.colorSpace = ColorSpace::Srgb; > > - break; > > - case libcamera::PixelFormatInfo::ColourEncodingYUV: > > - cfg.colorSpace = ColorSpace::Sycc; > > - break; > > - default: > > - cfg.colorSpace = ColorSpace::Raw; > > + LOG(SimplePipeline, Debug) > > + << "Adjusting raw stream to " > > + << cfg.toString(); > > + status = Adjusted; > > } > > - LOG(SimplePipeline, Debug) > > - << "Unspecified color space set to " > > - << cfg.colorSpace.value().toString(); > > - status = Adjusted; > > } > > - if (cfg.colorSpace->adjust(pixelFormat)) { > > The colour space adjustment is the primary difference in this diff area. > In Umang's version, it's still handled when processing stream roles and > it's missing here in validate(). The rest is more similar than the size > of the diff output might suggest. > > > + > > + /* Adjust the non-raw pixel format and size. */ > > + auto it = std::find(pipeConfig_->outputFormats.begin(), > > + pipeConfig_->outputFormats.end(), > > + cfg.pixelFormat); > > + if (it == pipeConfig_->outputFormats.end()) > > + it = pipeConfig_->outputFormats.begin(); > > + > > + PixelFormat pixelFormat = *it; > > + if (!raw && cfg.pixelFormat != pixelFormat) { > > LOG(SimplePipeline, Debug) > > - << "Color space adjusted to " > > - << cfg.colorSpace.value().toString(); > > + << "Adjusting pixel format from " > > + << cfg.pixelFormat << " to " << pixelFormat; > > + cfg.pixelFormat = pixelFormat; > > status = Adjusted; > > } > > > > @@ -1321,20 +1293,11 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > > > /* \todo Create a libcamera core class to group format and size */ > > if (cfg.pixelFormat != pipeConfig_->captureFormat || > > - cfg.size != pipeConfig_->captureSize) { > > - if (raw) { > > - cfg.pixelFormat = pipeConfig_->captureFormat; > > - cfg.size = pipeConfig_->captureSize; > > - LOG(SimplePipeline, Debug) > > - << "Adjusting raw configuration to " << cfg; > > - status = Adjusted; > > - } else { > > - needConversion_ = true; > > - } > > - } > > + cfg.size != pipeConfig_->captureSize) > > + needConversion_ = true; > > > > /* Set the stride and frameSize. */ > > - if (needConversion_ && !raw) { > > + if (needConversion_) { > > std::tie(cfg.stride, cfg.frameSize) = > > data_->converter_ > > ? data_->converter_->strideAndFrameSize(cfg.pixelFormat, > > @@ -1379,7 +1342,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > > > SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager) > > : PipelineHandler(manager, kMaxQueuedRequestsDevice), > > - converter_(nullptr) > > + converter_(nullptr), > > + swIspEnabled_(false) > > { > > } > > > > @@ -1393,78 +1357,64 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo > > if (roles.empty()) > > return config; > > > > - bool processedRequested = false; > > - bool rawRequested = false; > > - for (const auto &role : roles) > > - if (role == StreamRole::Raw) { > > - if (rawRequested) { > > - LOG(SimplePipeline, Error) > > - << "Can't capture multiple raw streams"; > > - return nullptr; > > - } > > - rawRequested = true; > > - } else { > > - processedRequested = true; > > - } > > IIRC Umang suggested that checking for this in validate() is enough and > it's not necessary to have a sort of duplicate check in > generateConfiguration(). > > The rest here is similar in both the versions in what it does. > > > - > > - /* Create the formats maps. */ > > - std::map<PixelFormat, std::vector<SizeRange>> processedFormats; > > - std::map<PixelFormat, std::vector<SizeRange>> rawFormats; > > - > > - for (const SimpleCameraData::Configuration &cfg : data->configs_) { > > - rawFormats[cfg.captureFormat].push_back(cfg.captureSize); > > - for (PixelFormat format : cfg.outputFormats) > > - processedFormats[format].push_back(cfg.outputSizes); > > - } > > + /* Create the formats map. */ > > + std::map<PixelFormat, std::vector<SizeRange>> formats; > > > > - if (processedRequested && processedFormats.empty()) { > > - LOG(SimplePipeline, Error) > > - << "Processed stream requsted but no corresponding output configuration found"; > > - return nullptr; > > - } > > - if (rawRequested && rawFormats.empty()) { > > - LOG(SimplePipeline, Error) > > - << "Raw stream requsted but no corresponding output configuration found"; > > - return nullptr; > > These checks are not present in Umang's version. > > > + for (const auto &it : data->formats_) { > > + for (const SimpleCameraData::Configuration *cfg : it.second) > > + formats[it.first].push_back(cfg->outputSizes); > > } > > > > - auto setUpFormatSizes = [](std::map<PixelFormat, std::vector<SizeRange>> &formats) { > > - /* Sort the sizes and merge any consecutive overlapping ranges. */ > > - > > - for (auto &[format, sizes] : formats) { > > - std::sort(sizes.begin(), sizes.end(), > > - [](SizeRange &a, SizeRange &b) { > > - return a.min < b.min; > > - }); > > - > > - auto cur = sizes.begin(); > > - auto next = cur; > > - > > - while (++next != sizes.end()) { > > - if (cur->max.width >= next->min.width && > > - cur->max.height >= next->min.height) > > - cur->max = next->max; > > - else if (++cur != next) > > - *cur = *next; > > - } > > - > > - sizes.erase(++cur, sizes.end()); > > + /* Sort the sizes and merge any consecutive overlapping ranges. */ > > + for (auto &[format, sizes] : formats) { > > + std::sort(sizes.begin(), sizes.end(), > > + [](SizeRange &a, SizeRange &b) { > > + return a.min < b.min; > > + }); > > + > > + auto cur = sizes.begin(); > > + auto next = cur; > > + > > + while (++next != sizes.end()) { > > + if (cur->max.width >= next->min.width && > > + cur->max.height >= next->min.height) > > + cur->max = next->max; > > + else if (++cur != next) > > + *cur = *next; > > } > > - }; > > - setUpFormatSizes(processedFormats); > > - setUpFormatSizes(rawFormats); > > + > > + sizes.erase(++cur, sizes.end()); > > + } > > > > /* > > * Create the stream configurations. Take the first entry in the formats > > - * map as the default, for lack of a better option. > > + * map as the default for each of role (raw or non-raw), for lack of a > > + * better option. > > * > > * \todo Implement a better way to pick the default format > > */ > > for (StreamRole role : roles) { > > - const auto &formats = (role == StreamRole::Raw ? rawFormats : processedFormats); > > StreamConfiguration cfg{ StreamFormats{ formats } }; > > - cfg.pixelFormat = formats.begin()->first; > > - cfg.size = formats.begin()->second[0].max; > > + > > + switch (role) { > > + case StreamRole::Raw: > > + for (auto &[format, sizes] : formats) { > > + if (!isFormatRaw(format)) > > + continue; > > + cfg.pixelFormat = format; > > + cfg.size = sizes.back().max; > > + cfg.colorSpace = ColorSpace::Raw; > > + break; > > + } > > + break; > > + default: > > + for (auto &[format, sizes] : formats) { > > + if (isFormatRaw(format)) > > + continue; > > + cfg.pixelFormat = format; > > + cfg.size = sizes[0].max; > > + } > > + } > > In my version, colour space handling is now only in validate(). The > pixel format and size selection are similar in both the versions. > > The rest of the diff are only cosmetic differences. > > > > > config->addConfiguration(cfg); > > } > > @@ -1532,7 +1482,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c) > > > > for (unsigned int i = 0; i < config->size(); ++i) { > > StreamConfiguration &cfg = config->at(i); > > - bool rawStream = isRaw(cfg); > > + bool rawStream = isFormatRaw(cfg.pixelFormat); > > > > cfg.setStream(&data->streams_[i]); > > > > @@ -1571,7 +1521,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream, > > * Export buffers on the converter or capture video node, depending on > > * whether the converter is used or not. > > */ > > - if (data->useConversion_ && stream != data->rawStream_) > > + if (data->useConversion_ && (stream != data->rawStream_)) > > return data->converter_ > > ? data->converter_->exportBuffers(stream, count, buffers) > > : data->swIsp_->exportBuffers(stream, count, buffers); > > @@ -1593,7 +1543,6 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > > << pad->entity()->name() << " in use"; > > return -EBUSY; > > } > > - > > if (data->useConversion_ && !data->rawStream_) { > > /* > > * When using the converter allocate a fixed number of internal > > @@ -1603,10 +1552,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > > &data->conversionBuffers_); > > } else { > > /* > > - * Otherwise, prepare for using buffers from either the raw stream, if > > - * requested, or the only stream configured. > > + * Otherwise, prepare for using buffers from either the raw > > + * stream((if requested) or the only stream configured. > > */ > > - Stream *stream = (data->rawStream_ ? data->rawStream_ : &data->streams_[0]); > > + Stream *stream; > > + if (data->rawStream_) > > + stream = data->rawStream_; > > + else > > + stream = &data->streams_[0]; > > + > > ret = video->importBuffers(stream->configuration().bufferCount); > > } > > if (ret < 0) { > > @@ -1647,9 +1601,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > > } > > > > /* Queue all internal buffers for capture. */ > > - if (!data->rawStream_) > > + if (!data->rawStream_) { > > for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) > > video->queueBuffer(buffer.get()); > > + } > > } > > > > return 0; > > @@ -1700,7 +1655,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > > * queue, it will be handed to the converter in the capture > > * completion handler. > > */ > > - if (data->useConversion_ && stream != data->rawStream_) { > > + if (data->useConversion_ && (stream != data->rawStream_)) { > > buffers.emplace(stream, buffer); > > metadataRequired = !!data->swIsp_; > > } else { > > @@ -1836,13 +1791,13 @@ bool SimplePipelineHandler::matchDevice(MediaDevice *media, > > /* > > * When the software ISP is enabled, the simple pipeline handler > > * exposes the raw stream, giving a total of two streams. This > > - * is mutually exclusive with the presence of a converter. > > + * is mutally exclusive with the presence of a converter. > > */ > > ASSERT(!converter_); > > numStreams = 2; > > + swIspEnabled_ = true; > > } > > > > - swIspEnabled_ = info.swIspEnabled; > > const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); > > for (GlobalConfiguration::Configuration entry : > > configuration.configuration()["pipelines"]["simple"]["supported_devices"] >
Hi Milan, Thanks for the setting up the diff. On Tue, Oct 21, 2025 at 08:36:29PM +0200, Milan Zamazal wrote: > There is another branch of this series by Umang. Since there was no > preference expressed which branch to use for the next version, I > continue with mine. > > While I tried to incorporate some changes from Umang's branch > previously, some differences remain, which I think require more > discussion. I rebased Umang's branch on the same master version, made a > diff between the two branches and commented on it below. The diff shows > an update of the posted patches to Umang's branch, i.e. the original > version in the diff is this v13, the new version is the rebased Umang's > branch. > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 607b07949..529d7344d 100644 > > --- a/src/libcamera/pipeline/simple/simple.cpp > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > @@ -11,7 +11,6 @@ > > #include <list> > > #include <map> > > #include <memory> > > -#include <optional> > > #include <queue> > > #include <set> > > #include <stdint.h> > > @@ -26,9 +25,7 @@ > > #include <libcamera/base/log.h> > > > > #include <libcamera/camera.h> > > -#include <libcamera/color_space.h> > > #include <libcamera/control_ids.h> > > -#include <libcamera/geometry.h> > > #include <libcamera/pixel_format.h> > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > @@ -40,7 +37,6 @@ > > #include "libcamera/internal/converter.h" > > #include "libcamera/internal/delayed_controls.h" > > #include "libcamera/internal/device_enumerator.h" > > -#include "libcamera/internal/formats.h" > > #include "libcamera/internal/global_configuration.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/pipeline_handler.h" > > @@ -267,9 +263,9 @@ static const SimplePipelineInfo supportedDevices[] = { > > { "sun6i-csi", {}, false }, > > }; > > > > -bool isRaw(const StreamConfiguration &cfg) > > +bool isFormatRaw(const libcamera::PixelFormat &pixFmt) > > isFormatRaw -> isRaw + argument type change was done in v12. Umang's > version uses in one (different) place format that is not stored in the > configuration. > > > { > > - return libcamera::PixelFormatInfo::info(cfg.pixelFormat).colourEncoding == > > + return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding == > > libcamera::PixelFormatInfo::ColourEncodingRAW; > > } > > > > @@ -372,11 +368,6 @@ private: > > void ispStatsReady(uint32_t frame, uint32_t bufferId); > > void metadataReady(uint32_t frame, const ControlList &metadata); > > void setSensorControls(const ControlList &sensorControls); > > A significant difference between the two versions is the buffer > handling. > > My version distinguishes between raw/non-raw cases at all the particular > places: IMO, tracking the raw stream if requested in data->rawStream_ is sufficent. I didn't find any need to split raw/non-raw cases as user testing with cam. Did you find any issues with that in testing the cases? I would also make a list of various cam user testing commands and test both implementation against that. I did that last time, but it was less formal. If there are no behaviour change in terms of user testing, maybe we need a tie-breaker ? > > > - > > - bool processedRequested() const > > - { > > - return streams_.size() - (rawStream_ ? 1 : 0) > 0; > > - } > > }; > > > > class SimpleCameraConfiguration : public CameraConfiguration > > @@ -884,13 +875,10 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > * point converting an erroneous buffer. > > */ > > if (buffer->metadata().status != FrameMetadata::FrameSuccess) { > > - if (rawStream_) { > > + if (!useConversion_) { > > /* No conversion, just complete the request. */ > > Request *request = buffer->request(); > > pipe->completeBuffer(request, buffer); > > - SimpleFrameInfo *info = frameInfo_.find(request->sequence()); > > - if (info) > > - info->metadataRequired = false; > > tryCompleteRequest(request); > > return; > > } > > @@ -900,8 +888,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > * buffer for capture (unless the stream is being stopped), and > > * complete the request with all the user-facing buffers. > > */ > > - if (buffer->metadata().status != FrameMetadata::FrameCancelled && > > - !rawStream_) > > + if (buffer->metadata().status != FrameMetadata::FrameCancelled) > > video_->queueBuffer(buffer); > > > > if (conversionQueue_.empty()) > > @@ -957,7 +944,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > > > if (converter_) > > converter_->queueBuffers(buffer, conversionQueue_.front().outputs); > > - else if (processedRequested()) > > + else > > /* > > * request->sequence() cannot be retrieved from `buffer' inside > > * queueBuffers because unique_ptr's make buffer->request() invalid > > @@ -967,8 +954,6 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > conversionQueue_.front().outputs); > > > > conversionQueue_.pop(); > > - if (rawStream_) > > - pipe->completeBuffer(request, buffer); > > return; > > } > > > > @@ -1006,8 +991,14 @@ void SimpleCameraData::tryCompleteRequest(Request *request) > > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) > > { > > /* Queue the input buffer back for capture. */ > > - if (!rawStream_) > > + if (!rawStream_) { > > video_->queueBuffer(buffer); > > While Umang's version has this final buffer completion: > Yes, it's simple, if rawStream_ is requested, you complete the buffer/request (no further processing required) in the else block. Otherwise, you queue for further processing (equivalent to your processedRequests) > > + } else { > > + /* Complete the input buffer. */ > > + Request *request = buffer->request(); > > + if (pipe()->completeBuffer(request, buffer)) > > + tryCompleteRequest(request); > > + } > > } > > > > void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) > > @@ -1156,31 +1147,37 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > LOG(SimplePipeline, Debug) > > << "Largest stream size is " << maxStreamSize; > > > > + /* Cap the number of raw stream configuration */ > > + unsigned int rawCount = 0; > > + PixelFormat requestedRawFormat; > > + for (const StreamConfiguration &cfg : config_) { > > + if (!isFormatRaw(cfg.pixelFormat)) > > + continue; > > + requestedRawFormat = cfg.pixelFormat; > > + rawCount++; > > + } > > + > > + if (rawCount > 1) { > > + LOG(SimplePipeline, Error) > > + << "Camera configuration with " > > + << rawCount << " raw streams not supported"; > > + return Invalid; > > + } > > + > > This differs in how to check for multiple raw streams in validate() and > how to select the config but doesn't seem to do something very > different. ack, but this mostly aligns how other pipeline handlers checks as well. > > > /* > > - * Find the best configuration for the pipeline using a heuristic. First > > - * select the pixel format based on the streams. If there is a raw stream, > > - * its format has precedence. If there is no raw stream, the streams are > > - * considered ordered from highest to lowest priority. Default to the first > > - * pipeline configuration if no streams request a supported pixel format. > > + * Find the best configuration for the pipeline using a heuristic. > > + * First select the pixel format based on the raw streams followed by > > + * non-raw streams (which are considered ordered from highest to lowest > > + * priority). Default to the first pipeline configuration if no streams > > + * request a supported pixel format. > > */ > > - std::optional<PixelFormat> rawFormat; > > - for (const auto &cfg : config_) > > - if (isRaw(cfg)) { > > - if (rawFormat) { > > - LOG(SimplePipeline, Error) > > - << "Can't capture multiple raw streams"; > > - return Invalid; > > - } > > - rawFormat = cfg.pixelFormat; > > - } > > + const std::vector<const SimpleCameraData::Configuration *> *configs = > > + &data_->formats_.begin()->second; > > > > - const std::vector<const SimpleCameraData::Configuration *> *configs = nullptr; > > - if (rawFormat) { > > - auto it = data_->formats_.find(rawFormat.value()); > > - if (it != data_->formats_.end()) > > - configs = &it->second; > > - } > > - if (!configs) > > + auto rawIter = data_->formats_.find(requestedRawFormat); > > + if (rawIter != data_->formats_.end()) { > > + configs = &rawIter->second; > > + } else { > > for (const StreamConfiguration &cfg : config_) { > > auto it = data_->formats_.find(cfg.pixelFormat); > > if (it != data_->formats_.end()) { > > @@ -1188,8 +1185,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > break; > > } > > } > > - if (!configs) > > - configs = &data_->formats_.begin()->second; > > + } > > > > /* > > * \todo Pick the best sensor output media bus format when the > > @@ -1242,63 +1238,39 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > * require any conversion, similar to raw capture use cases). This is > > * left as a future improvement. > > */ > > - needConversion_ = config_.size() > 1 + (rawFormat ? 1 : 0); > > + needConversion_ = config_.size() > 1 + rawCount; > > > > for (unsigned int i = 0; i < config_.size(); ++i) { > > StreamConfiguration &cfg = config_[i]; > > - const bool raw = isRaw(cfg); > > + const bool raw = isFormatRaw(cfg.pixelFormat); > > > > - /* Adjust the pixel format, colour space and size. */ > > - > > - PixelFormat pixelFormat; > > + /* Adjust the raw pixel format and size. */ > > if (raw) { > > - pixelFormat = pipeConfig_->captureFormat; > > - } else { > > - auto it = std::find(pipeConfig_->outputFormats.begin(), > > - pipeConfig_->outputFormats.end(), > > - cfg.pixelFormat); > > - if (it == pipeConfig_->outputFormats.end()) > > - it = pipeConfig_->outputFormats.begin(); > > - pixelFormat = *it; > > - } > > - if (cfg.pixelFormat != pixelFormat) { > > - LOG(SimplePipeline, Debug) > > - << "Adjusting pixel format of a " > > - << (raw ? "raw" : "processed") > > - << " stream from " << cfg.pixelFormat > > - << " to " << pixelFormat; > > - cfg.pixelFormat = pixelFormat; > > - status = Adjusted; > > - } > > + if (cfg.pixelFormat != pipeConfig_->captureFormat || > > + cfg.size != pipeConfig_->captureSize) { > > + cfg.pixelFormat = pipeConfig_->captureFormat; > > + cfg.size = pipeConfig_->captureSize; > > > > - /* > > - * Best effort to fix the color space. If the color space is not set, > > - * set it according to the pixel format, which may not be correct (pixel > > - * formats and color spaces are different things, although somewhat > > - * related) but we don't have a better option at the moment. Then in any > > - * case, perform the standard pixel format based color space adjustment. > > - */ > > - if (!cfg.colorSpace) { > > - const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat); > > - switch (info.colourEncoding) { > > - case PixelFormatInfo::ColourEncodingRGB: > > - cfg.colorSpace = ColorSpace::Srgb; > > - break; > > - case libcamera::PixelFormatInfo::ColourEncodingYUV: > > - cfg.colorSpace = ColorSpace::Sycc; > > - break; > > - default: > > - cfg.colorSpace = ColorSpace::Raw; > > + LOG(SimplePipeline, Debug) > > + << "Adjusting raw stream to " > > + << cfg.toString(); > > + status = Adjusted; > > } > > - LOG(SimplePipeline, Debug) > > - << "Unspecified color space set to " > > - << cfg.colorSpace.value().toString(); > > - status = Adjusted; > > } > > - if (cfg.colorSpace->adjust(pixelFormat)) { > > The colour space adjustment is the primary difference in this diff area. > In Umang's version, it's still handled when processing stream roles and > it's missing here in validate(). The rest is more similar than the size > of the diff output might suggest. I remember that I commented that colorspace changes could be separated out from your series. My series only sets colorSpace::RAW to raw streams AFAIR. > > > + > > + /* Adjust the non-raw pixel format and size. */ > > + auto it = std::find(pipeConfig_->outputFormats.begin(), > > + pipeConfig_->outputFormats.end(), > > + cfg.pixelFormat); > > + if (it == pipeConfig_->outputFormats.end()) > > + it = pipeConfig_->outputFormats.begin(); > > + > > + PixelFormat pixelFormat = *it; > > + if (!raw && cfg.pixelFormat != pixelFormat) { > > LOG(SimplePipeline, Debug) > > - << "Color space adjusted to " > > - << cfg.colorSpace.value().toString(); > > + << "Adjusting pixel format from " > > + << cfg.pixelFormat << " to " << pixelFormat; > > + cfg.pixelFormat = pixelFormat; > > status = Adjusted; > > } > > > > @@ -1321,20 +1293,11 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > > > /* \todo Create a libcamera core class to group format and size */ > > if (cfg.pixelFormat != pipeConfig_->captureFormat || > > - cfg.size != pipeConfig_->captureSize) { > > - if (raw) { > > - cfg.pixelFormat = pipeConfig_->captureFormat; > > - cfg.size = pipeConfig_->captureSize; > > - LOG(SimplePipeline, Debug) > > - << "Adjusting raw configuration to " << cfg; > > - status = Adjusted; > > - } else { > > - needConversion_ = true; > > - } > > - } > > + cfg.size != pipeConfig_->captureSize) > > + needConversion_ = true; > > > > /* Set the stride and frameSize. */ > > - if (needConversion_ && !raw) { > > + if (needConversion_) { > > std::tie(cfg.stride, cfg.frameSize) = > > data_->converter_ > > ? data_->converter_->strideAndFrameSize(cfg.pixelFormat, > > @@ -1379,7 +1342,8 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > > > SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager) > > : PipelineHandler(manager, kMaxQueuedRequestsDevice), > > - converter_(nullptr) > > + converter_(nullptr), > > + swIspEnabled_(false) > > { > > } > > > > @@ -1393,78 +1357,64 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo > > if (roles.empty()) > > return config; > > > > - bool processedRequested = false; > > - bool rawRequested = false; > > - for (const auto &role : roles) > > - if (role == StreamRole::Raw) { > > - if (rawRequested) { > > - LOG(SimplePipeline, Error) > > - << "Can't capture multiple raw streams"; > > - return nullptr; > > - } > > - rawRequested = true; > > - } else { > > - processedRequested = true; > > - } > > IIRC Umang suggested that checking for this in validate() is enough and > it's not necessary to have a sort of duplicate check in > generateConfiguration(). The problem with validate() behaviour is that it's two fold. 1) for validation as per docs 2) for completing the rest of configuration in generateConfiguration. Sometime ago, I tried to ensure that the config->validate() return value should be checked - but then Laurent told me about 2). Hence I think to duplicate the check there hence I agree with this (since, generateConfiguration() calls validate() - but for 2) not 1). This should be changed ideally - but I haven't thought it through. There was some discussion here: https://patchwork.libcamera.org/patch/23806/#34899 > > The rest here is similar in both the versions in what it does. > > > - > > - /* Create the formats maps. */ > > - std::map<PixelFormat, std::vector<SizeRange>> processedFormats; > > - std::map<PixelFormat, std::vector<SizeRange>> rawFormats; > > - > > - for (const SimpleCameraData::Configuration &cfg : data->configs_) { > > - rawFormats[cfg.captureFormat].push_back(cfg.captureSize); > > - for (PixelFormat format : cfg.outputFormats) > > - processedFormats[format].push_back(cfg.outputSizes); > > - } > > + /* Create the formats map. */ > > + std::map<PixelFormat, std::vector<SizeRange>> formats; > > > > - if (processedRequested && processedFormats.empty()) { > > - LOG(SimplePipeline, Error) > > - << "Processed stream requsted but no corresponding output configuration found"; > > - return nullptr; > > - } > > - if (rawRequested && rawFormats.empty()) { > > - LOG(SimplePipeline, Error) > > - << "Raw stream requsted but no corresponding output configuration found"; > > - return nullptr; > > These checks are not present in Umang's version. Yes, my implementation just tracks rawStream_ is requested or not. I didn't feel any need to track processed streams separately. > > > + for (const auto &it : data->formats_) { > > + for (const SimpleCameraData::Configuration *cfg : it.second) > > + formats[it.first].push_back(cfg->outputSizes); > > } > > > > - auto setUpFormatSizes = [](std::map<PixelFormat, std::vector<SizeRange>> &formats) { > > - /* Sort the sizes and merge any consecutive overlapping ranges. */ > > - > > - for (auto &[format, sizes] : formats) { > > - std::sort(sizes.begin(), sizes.end(), > > - [](SizeRange &a, SizeRange &b) { > > - return a.min < b.min; > > - }); > > - > > - auto cur = sizes.begin(); > > - auto next = cur; > > - > > - while (++next != sizes.end()) { > > - if (cur->max.width >= next->min.width && > > - cur->max.height >= next->min.height) > > - cur->max = next->max; > > - else if (++cur != next) > > - *cur = *next; > > - } > > - > > - sizes.erase(++cur, sizes.end()); > > + /* Sort the sizes and merge any consecutive overlapping ranges. */ > > + for (auto &[format, sizes] : formats) { > > + std::sort(sizes.begin(), sizes.end(), > > + [](SizeRange &a, SizeRange &b) { > > + return a.min < b.min; > > + }); > > + > > + auto cur = sizes.begin(); > > + auto next = cur; > > + > > + while (++next != sizes.end()) { > > + if (cur->max.width >= next->min.width && > > + cur->max.height >= next->min.height) > > + cur->max = next->max; > > + else if (++cur != next) > > + *cur = *next; > > } > > - }; > > - setUpFormatSizes(processedFormats); > > - setUpFormatSizes(rawFormats); > > + > > + sizes.erase(++cur, sizes.end()); > > + } IMO these are also some major difference implementation-wise here as well, but towards a common goal. > > > > /* > > * Create the stream configurations. Take the first entry in the formats > > - * map as the default, for lack of a better option. > > + * map as the default for each of role (raw or non-raw), for lack of a > > + * better option. > > * > > * \todo Implement a better way to pick the default format > > */ > > for (StreamRole role : roles) { > > - const auto &formats = (role == StreamRole::Raw ? rawFormats : processedFormats); > > StreamConfiguration cfg{ StreamFormats{ formats } }; > > - cfg.pixelFormat = formats.begin()->first; > > - cfg.size = formats.begin()->second[0].max; > > + > > + switch (role) { > > + case StreamRole::Raw: > > + for (auto &[format, sizes] : formats) { > > + if (!isFormatRaw(format)) > > + continue; > > + cfg.pixelFormat = format; > > + cfg.size = sizes.back().max; > > + cfg.colorSpace = ColorSpace::Raw; > > + break; > > + } > > + break; > > + default: > > + for (auto &[format, sizes] : formats) { > > + if (isFormatRaw(format)) > > + continue; > > + cfg.pixelFormat = format; > > + cfg.size = sizes[0].max; > > + } > > + } > > In my version, colour space handling is now only in validate(). The > pixel format and size selection are similar in both the versions. > > The rest of the diff are only cosmetic differences. > > > > > config->addConfiguration(cfg); > > } > > @@ -1532,7 +1482,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c) > > > > for (unsigned int i = 0; i < config->size(); ++i) { > > StreamConfiguration &cfg = config->at(i); > > - bool rawStream = isRaw(cfg); > > + bool rawStream = isFormatRaw(cfg.pixelFormat); > > > > cfg.setStream(&data->streams_[i]); > > > > @@ -1571,7 +1521,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream, > > * Export buffers on the converter or capture video node, depending on > > * whether the converter is used or not. > > */ > > - if (data->useConversion_ && stream != data->rawStream_) > > + if (data->useConversion_ && (stream != data->rawStream_)) > > return data->converter_ > > ? data->converter_->exportBuffers(stream, count, buffers) > > : data->swIsp_->exportBuffers(stream, count, buffers); > > @@ -1593,7 +1543,6 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > > << pad->entity()->name() << " in use"; > > return -EBUSY; > > } > > - > > if (data->useConversion_ && !data->rawStream_) { > > /* > > * When using the converter allocate a fixed number of internal > > @@ -1603,10 +1552,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > > &data->conversionBuffers_); > > } else { > > /* > > - * Otherwise, prepare for using buffers from either the raw stream, if > > - * requested, or the only stream configured. > > + * Otherwise, prepare for using buffers from either the raw > > + * stream((if requested) or the only stream configured. > > */ > > - Stream *stream = (data->rawStream_ ? data->rawStream_ : &data->streams_[0]); > > + Stream *stream; > > + if (data->rawStream_) > > + stream = data->rawStream_; > > + else > > + stream = &data->streams_[0]; > > + > > ret = video->importBuffers(stream->configuration().bufferCount); > > } > > if (ret < 0) { > > @@ -1647,9 +1601,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > > } > > > > /* Queue all internal buffers for capture. */ > > - if (!data->rawStream_) > > + if (!data->rawStream_) { > > for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) > > video->queueBuffer(buffer.get()); > > + } > > } > > > > return 0; > > @@ -1700,7 +1655,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > > * queue, it will be handed to the converter in the capture > > * completion handler. > > */ > > - if (data->useConversion_ && stream != data->rawStream_) { > > + if (data->useConversion_ && (stream != data->rawStream_)) { > > buffers.emplace(stream, buffer); > > metadataRequired = !!data->swIsp_; > > } else { > > @@ -1836,13 +1791,13 @@ bool SimplePipelineHandler::matchDevice(MediaDevice *media, > > /* > > * When the software ISP is enabled, the simple pipeline handler > > * exposes the raw stream, giving a total of two streams. This > > - * is mutually exclusive with the presence of a converter. > > + * is mutally exclusive with the presence of a converter. > > */ > > ASSERT(!converter_); > > numStreams = 2; > > + swIspEnabled_ = true; > > } > > > > - swIspEnabled_ = info.swIspEnabled; > > const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); > > for (GlobalConfiguration::Configuration entry : > > configuration.configuration()["pipelines"]["simple"]["supported_devices"] >