| 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"] >
This makes raw streams working again in ‘simple’ pipeline when software ISP is enabled for the given device. At most one raw stream and one processed stream (possibly both at once) are supported. An example ‘cam’ invocation requesting a raw stream rather than a debayered stream: cam -c1 -C8 -s role=raw,width=1920,height=1080 -Ffile#.raw Or for both raw and processed streams: cam -c1 -C8 -Ffile# \ -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 When only a raw stream is requested, there are no exposure/gain adjustments applied. This could be improved in future, once software ISP gets a mechanism to gather image statistics without processing and using them to make the adjustments, or once manual exposure controls are added to software ISP. In the meantime, exposure must be changed externally. Changes in v13: - The redundant check for multiple raw streams in SimplePipelineHandler::configure() removed. - Typo in a source code comment fixed. - Information about the number of streams in a commit message corrected. Changes in v12: - Formatting change suggested by Laurent. - The patch setting the number of software ISP streams moved after the patches enabling raw streams. - Colour spaces are set and validated according to the pixel format and only pixel format. Changes in v11: - Use rawStream_ pointer to a raw stream rather than rawRequested_ and processedRequested_ flags, as suggested by Umang. - Selection of pipeline configurations reworked, partially incorporating Umang’s ideas and permitting adjustments of raw stream configurations if they don’t match the processed stream or what’s available from the sensor. - Tested-by tags removed from here because the changes are substantial. v11 works better for me than both v10 and Umang’s RFC but it requires independent testing. Changes in v10: - Missing initialisation of swIspEnabled_ added. - "Add plain output configurations" patch dropped. Raw configurations can be built implicitly from capture parameters. Related adjustments in "Validate raw stream configurations". - "Identify requested stream roles" patch dropped and merged into followup patches; camera configuration is updated in validate() and not in generateConfiguration() now. - A commit title improved as suggested by Umang. Changes in v9: - Fix of calling a wrong output buffer allocator when both raw and processed streams are used. Changes in v8: - A missing `status = Adjusted' added to SimpleCameraConfiguration::validate. - Code comments regarding raw colour space for raw requested roles added. - Dropped raw format helpers. Changes in v7: - Rebased on current master. Changes in v6: - An unnecessary copy of formats avoided. - rawRequested_ and processedrequested_ set to false when there are no roles. - rawRequested_ and processedrequested_ updated in SimpleCameraConfiguration::validate(). Changes in v5: - Possible temporary segfault in the patch adding plain output configurations avoided. - PixelFormatInfo::isRaw() helper added. - SimplePipelineHandler::setUpFormatSizes replaced with a lambda function. - SimpleCameraData::{rawRequested_,processedRequested_} are set in SimpleCameraConfiguration first and copied to SimpleCameraData only after successful configure(). Changes in v4: - Broken range pruning due to passing a value rather than a reference fixed. - New common function isFormatRaw introduced. - The patch assigning colour spaces in the simple pipeline, previously posted separately, included in this series, as the first patch. It can still be handled separately; in any case the rest sort of depends on it. - Setting metadataRequired to false where needed to prevent freezes and assertion errors; related to metadata reporting support merged to master since v3. Changes in v3: - Significantly reworked, with both functional and clarity improvements. The level of guesswork and confusion is hopefully reduced enough now to drop the RFC prefix. - The number of streams is set to 2 only with software ISP. - SimpleCameraData::pipeConfig_ nullptr check patch dropped. - PPM/raw file output patch dropped from this series. Let’s handle this separately as the patch series is already complex enough. Changes in v2: - Completely reworked. - Extended to be able to produce a raw stream together with a processed stream. Milan Zamazal (8): libcamera: software_isp: Assign colour spaces in configurations libcamera: simple: Exclude raw configurations from output conversions libcamera: simple: Handle processed and raw formats separately libcamera: simple: Validate raw stream configurations libcamera: simple: Don't enforce conversion with an added raw stream libcamera: simple: Set the number of software ISP streams to 2 libcamera: simple: Require metadata only when software ISP is used libcamera: simple: Make raw streams working src/libcamera/pipeline/simple/simple.cpp | 271 +++++++++++++++++------ 1 file changed, 207 insertions(+), 64 deletions(-)