| Message ID | 20250711175345.90318-6-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, On Fri, Jul 11, 2025 at 07:53:38PM +0200, Milan Zamazal wrote: > SimpleCameraConfiguration::validate() looks for the best configuration. > As part of enabling raw stream support, the method must consider raw > streams in addition to the processed streams. > > If only a processed stream is requested, nothing changes. > > If only a raw stream is requested, the pixel format and output size may > not be adjusted. The patch adds checks for this. Can you explain briefly why is that? My understanding of the current status-quo is that, it should be possible to adjust the Raw streams/configuration as well. Please take a look at some examples: ($) git grep -ni raw | grep -i adjust For sake of completeness, the case where this is not adjust-able is when CameraConfiguration::sensorConfig is passed by the application. > > If both processed and raw streams are requested, things get more > complicated. The raw stream is expected to be passed through intact and > all the adjustments are made for the processed streams. We select a > pipe configuration for the processed streams. > > Note that with both processed and raw streams, the requested sizes must > be mutually matching, including resizing due to debayer requirements. > For example, the following `cam' setup is valid for imx219 > > cam -s role=viewfinder,width=1920,height=1080 \ > -s role=raw,width=3280,height=2464 > > rather than > > cam -s role=viewfinder,width=1920,height=1080 \ > -s role=raw,width=1920,height=1080 > > due to the resolution of 1924x1080 actually selected for debayering to > 1920x1080. It is the application responsibility to select the right > parameters for the raw stream. > > Setting up the right configurations is still not enough to make the raw > streams working. Buffer handling must be changed in the simple > pipeline, which is addressed in followup patches. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 119 ++++++++++++++++------- > 1 file changed, 85 insertions(+), 34 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 37abaa0e0..9d87a7ffa 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -27,6 +27,7 @@ > #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> > @@ -1186,6 +1187,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > << "-" << pipeConfig_->captureFormat > << " for max stream size " << maxStreamSize; > > + bool rawRequested = false; > + bool processedRequested = false; > + for (const auto &cfg : config_) > + if (cfg.colorSpace == ColorSpace::Raw) { > + if (rawRequested) { > + LOG(SimplePipeline, Error) > + << "Can't capture multiple raw streams"; > + return Invalid; > + } > + rawRequested = true; > + } else { > + processedRequested = true; > + } > + > /* > * Adjust the requested streams. > * > @@ -1204,43 +1219,70 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > for (unsigned int i = 0; i < config_.size(); ++i) { > StreamConfiguration &cfg = config_[i]; > > + /* > + * If both processed and raw streams are requested, the pipe > + * configuration is set up for the processed stream. The raw > + * configuration needs to be compared against the capture format and > + * size in such a case. > + */ > + const bool rawStream = cfg.colorSpace == ColorSpace::Raw; > + const bool sideRawStream = rawStream && processedRequested; > + > /* Adjust the 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 (cfg.pixelFormat != pixelFormat) { > - LOG(SimplePipeline, Debug) << "Adjusting pixel format"; > - cfg.pixelFormat = pixelFormat; > - /* > - * Do not touch the colour space for raw requested roles. > - * Even if the pixel format is non-raw (whatever it means), we > - * shouldn't try to interpret the colour space of raw data. > - */ > - if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) > + > + if (!sideRawStream) { > + PixelFormat pixelFormat; > + if (rawStream) { > + 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) { > + if (rawStream) { > + LOG(SimplePipeline, Info) > + << "Raw pixel format " > + << cfg.pixelFormat > + << " doesn't match any of the pipe output formats"; > + return Invalid; > + } > + LOG(SimplePipeline, Debug) > + << "Adjusting pixel format from " << cfg.pixelFormat > + << " to " << pixelFormat; > + cfg.pixelFormat = pixelFormat; > + /* > + * Do not touch the colour space for raw requested roles. > + * Even if the pixel format is non-raw (whatever it means), we > + * shouldn't try to interpret the colour space of raw data. > + */ > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) > + cfg.colorSpace->adjust(pixelFormat); > + status = Adjusted; > + } > + > + 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; > + } > cfg.colorSpace->adjust(pixelFormat); > - status = Adjusted; > - } > - 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; > + status = Adjusted; > } > - cfg.colorSpace->adjust(pixelFormat); > - status = Adjusted; > } > > - if (!pipeConfig_->outputSizes.contains(cfg.size)) { > + if (!rawStream && !pipeConfig_->outputSizes.contains(cfg.size)) { > Size adjustedSize = pipeConfig_->captureSize; > /* > * The converter (when present) may not be able to output > @@ -1259,11 +1301,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > /* \todo Create a libcamera core class to group format and size */ > if (cfg.pixelFormat != pipeConfig_->captureFormat || > - cfg.size != pipeConfig_->captureSize) > + cfg.size != pipeConfig_->captureSize) { > + if (rawStream) { > + LOG(SimplePipeline, Info) > + << "Raw output format " << cfg.pixelFormat > + << " and size " << cfg.size > + << " not matching pipe format " << pipeConfig_->captureFormat > + << " and size " << pipeConfig_->captureSize; > + return Invalid; > + } > needConversion_ = true; > + } > > /* Set the stride, frameSize and bufferCount. */ > - if (needConversion_) { > + if (needConversion_ && !rawStream) { > std::tie(cfg.stride, cfg.frameSize) = > data_->converter_ > ? data_->converter_->strideAndFrameSize(cfg.pixelFormat, > -- > 2.50.1 >
Hi Umang, Umang Jain <uajain@igalia.com> writes: > Hi Milan, > > On Fri, Jul 11, 2025 at 07:53:38PM +0200, Milan Zamazal wrote: >> SimpleCameraConfiguration::validate() looks for the best configuration. >> As part of enabling raw stream support, the method must consider raw >> streams in addition to the processed streams. >> >> If only a processed stream is requested, nothing changes. >> >> If only a raw stream is requested, the pixel format and output size may >> not be adjusted. The patch adds checks for this. > > Can you explain briefly why is that? My understanding of the current > status-quo is that, it should be possible to adjust the Raw > streams/configuration as well. Please take a look at some examples: > ($) git grep -ni raw | grep -i adjust > > For sake of completeness, the case where this is not adjust-able is > when CameraConfiguration::sensorConfig is passed by the application. Right, I missed the difference between an explicitly provided configuration by the application and the case when a different raw format may be selected. I'll try to fix it. >> >> If both processed and raw streams are requested, things get more >> complicated. The raw stream is expected to be passed through intact and >> all the adjustments are made for the processed streams. We select a >> pipe configuration for the processed streams. >> >> Note that with both processed and raw streams, the requested sizes must >> be mutually matching, including resizing due to debayer requirements. >> For example, the following `cam' setup is valid for imx219 >> >> cam -s role=viewfinder,width=1920,height=1080 \ >> -s role=raw,width=3280,height=2464 >> >> rather than >> >> cam -s role=viewfinder,width=1920,height=1080 \ >> -s role=raw,width=1920,height=1080 >> >> due to the resolution of 1924x1080 actually selected for debayering to >> 1920x1080. It is the application responsibility to select the right >> parameters for the raw stream. >> >> Setting up the right configurations is still not enough to make the raw >> streams working. Buffer handling must be changed in the simple >> pipeline, which is addressed in followup patches. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 119 ++++++++++++++++------- >> 1 file changed, 85 insertions(+), 34 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 37abaa0e0..9d87a7ffa 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -27,6 +27,7 @@ >> #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> >> @@ -1186,6 +1187,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> << "-" << pipeConfig_->captureFormat >> << " for max stream size " << maxStreamSize; >> >> + bool rawRequested = false; >> + bool processedRequested = false; >> + for (const auto &cfg : config_) >> + if (cfg.colorSpace == ColorSpace::Raw) { >> + if (rawRequested) { >> + LOG(SimplePipeline, Error) >> + << "Can't capture multiple raw streams"; >> + return Invalid; >> + } >> + rawRequested = true; >> + } else { >> + processedRequested = true; >> + } >> + >> /* >> * Adjust the requested streams. >> * >> @@ -1204,43 +1219,70 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> for (unsigned int i = 0; i < config_.size(); ++i) { >> StreamConfiguration &cfg = config_[i]; >> >> + /* >> + * If both processed and raw streams are requested, the pipe >> + * configuration is set up for the processed stream. The raw >> + * configuration needs to be compared against the capture format and >> + * size in such a case. >> + */ >> + const bool rawStream = cfg.colorSpace == ColorSpace::Raw; >> + const bool sideRawStream = rawStream && processedRequested; >> + >> /* Adjust the 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 (cfg.pixelFormat != pixelFormat) { >> - LOG(SimplePipeline, Debug) << "Adjusting pixel format"; >> - cfg.pixelFormat = pixelFormat; >> - /* >> - * Do not touch the colour space for raw requested roles. >> - * Even if the pixel format is non-raw (whatever it means), we >> - * shouldn't try to interpret the colour space of raw data. >> - */ >> - if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >> + >> + if (!sideRawStream) { >> + PixelFormat pixelFormat; >> + if (rawStream) { >> + 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) { >> + if (rawStream) { >> + LOG(SimplePipeline, Info) >> + << "Raw pixel format " >> + << cfg.pixelFormat >> + << " doesn't match any of the pipe output formats"; >> + return Invalid; >> + } >> + LOG(SimplePipeline, Debug) >> + << "Adjusting pixel format from " << cfg.pixelFormat >> + << " to " << pixelFormat; >> + cfg.pixelFormat = pixelFormat; >> + /* >> + * Do not touch the colour space for raw requested roles. >> + * Even if the pixel format is non-raw (whatever it means), we >> + * shouldn't try to interpret the colour space of raw data. >> + */ >> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >> + cfg.colorSpace->adjust(pixelFormat); >> + status = Adjusted; >> + } >> + >> + 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; >> + } >> cfg.colorSpace->adjust(pixelFormat); >> - status = Adjusted; >> - } >> - 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; >> + status = Adjusted; >> } >> - cfg.colorSpace->adjust(pixelFormat); >> - status = Adjusted; >> } >> >> - if (!pipeConfig_->outputSizes.contains(cfg.size)) { >> + if (!rawStream && !pipeConfig_->outputSizes.contains(cfg.size)) { >> Size adjustedSize = pipeConfig_->captureSize; >> /* >> * The converter (when present) may not be able to output >> @@ -1259,11 +1301,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> >> /* \todo Create a libcamera core class to group format and size */ >> if (cfg.pixelFormat != pipeConfig_->captureFormat || >> - cfg.size != pipeConfig_->captureSize) >> + cfg.size != pipeConfig_->captureSize) { >> + if (rawStream) { >> + LOG(SimplePipeline, Info) >> + << "Raw output format " << cfg.pixelFormat >> + << " and size " << cfg.size >> + << " not matching pipe format " << pipeConfig_->captureFormat >> + << " and size " << pipeConfig_->captureSize; >> + return Invalid; >> + } >> needConversion_ = true; >> + } >> >> /* Set the stride, frameSize and bufferCount. */ >> - if (needConversion_) { >> + if (needConversion_ && !rawStream) { >> std::tie(cfg.stride, cfg.frameSize) = >> data_->converter_ >> ? data_->converter_->strideAndFrameSize(cfg.pixelFormat, >> -- >> 2.50.1 >>
Hi Milan, On 14 July 2025 9:51:54 pm IST, Milan Zamazal <mzamazal@redhat.com> wrote: >Hi Umang, > >Umang Jain <uajain@igalia.com> writes: > >> Hi Milan, >> >> On Fri, Jul 11, 2025 at 07:53:38PM +0200, Milan Zamazal wrote: >>> SimpleCameraConfiguration::validate() looks for the best configuration. >>> As part of enabling raw stream support, the method must consider raw >>> streams in addition to the processed streams. >>> >>> If only a processed stream is requested, nothing changes. >>> >>> If only a raw stream is requested, the pixel format and output size may >>> not be adjusted. The patch adds checks for this. >> >> Can you explain briefly why is that? My understanding of the current >> status-quo is that, it should be possible to adjust the Raw >> streams/configuration as well. Please take a look at some examples: >> ($) git grep -ni raw | grep -i adjust >> >> For sake of completeness, the case where this is not adjust-able is >> when CameraConfiguration::sensorConfig is passed by the application. > >Right, I missed the difference between an explicitly provided >configuration by the application and the case when a different raw >format may be selected. I'll try to fix it. I have a few patches in my branch that are in this direction. I can share the branch if you want (although it's under development, but I have rewritten the validation and generate configuration logic to what I had in mind). > >>> >>> If both processed and raw streams are requested, things get more >>> complicated. The raw stream is expected to be passed through intact and >>> all the adjustments are made for the processed streams. We select a >>> pipe configuration for the processed streams. >>> >>> Note that with both processed and raw streams, the requested sizes must >>> be mutually matching, including resizing due to debayer requirements. >>> For example, the following `cam' setup is valid for imx219 >>> >>> cam -s role=viewfinder,width=1920,height=1080 \ >>> -s role=raw,width=3280,height=2464 >>> >>> rather than >>> >>> cam -s role=viewfinder,width=1920,height=1080 \ >>> -s role=raw,width=1920,height=1080 >>> >>> due to the resolution of 1924x1080 actually selected for debayering to >>> 1920x1080. It is the application responsibility to select the right >>> parameters for the raw stream. >>> >>> Setting up the right configurations is still not enough to make the raw >>> streams working. Buffer handling must be changed in the simple >>> pipeline, which is addressed in followup patches. >>> >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> src/libcamera/pipeline/simple/simple.cpp | 119 ++++++++++++++++------- >>> 1 file changed, 85 insertions(+), 34 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >>> index 37abaa0e0..9d87a7ffa 100644 >>> --- a/src/libcamera/pipeline/simple/simple.cpp >>> +++ b/src/libcamera/pipeline/simple/simple.cpp >>> @@ -27,6 +27,7 @@ >>> #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> >>> @@ -1186,6 +1187,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>> << "-" << pipeConfig_->captureFormat >>> << " for max stream size " << maxStreamSize; >>> >>> + bool rawRequested = false; >>> + bool processedRequested = false; >>> + for (const auto &cfg : config_) >>> + if (cfg.colorSpace == ColorSpace::Raw) { >>> + if (rawRequested) { >>> + LOG(SimplePipeline, Error) >>> + << "Can't capture multiple raw streams"; >>> + return Invalid; >>> + } >>> + rawRequested = true; >>> + } else { >>> + processedRequested = true; >>> + } >>> + >>> /* >>> * Adjust the requested streams. >>> * >>> @@ -1204,43 +1219,70 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>> for (unsigned int i = 0; i < config_.size(); ++i) { >>> StreamConfiguration &cfg = config_[i]; >>> >>> + /* >>> + * If both processed and raw streams are requested, the pipe >>> + * configuration is set up for the processed stream. The raw >>> + * configuration needs to be compared against the capture format and >>> + * size in such a case. >>> + */ >>> + const bool rawStream = cfg.colorSpace == ColorSpace::Raw; >>> + const bool sideRawStream = rawStream && processedRequested; >>> + >>> /* Adjust the 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 (cfg.pixelFormat != pixelFormat) { >>> - LOG(SimplePipeline, Debug) << "Adjusting pixel format"; >>> - cfg.pixelFormat = pixelFormat; >>> - /* >>> - * Do not touch the colour space for raw requested roles. >>> - * Even if the pixel format is non-raw (whatever it means), we >>> - * shouldn't try to interpret the colour space of raw data. >>> - */ >>> - if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >>> + >>> + if (!sideRawStream) { >>> + PixelFormat pixelFormat; >>> + if (rawStream) { >>> + 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) { >>> + if (rawStream) { >>> + LOG(SimplePipeline, Info) >>> + << "Raw pixel format " >>> + << cfg.pixelFormat >>> + << " doesn't match any of the pipe output formats"; >>> + return Invalid; >>> + } >>> + LOG(SimplePipeline, Debug) >>> + << "Adjusting pixel format from " << cfg.pixelFormat >>> + << " to " << pixelFormat; >>> + cfg.pixelFormat = pixelFormat; >>> + /* >>> + * Do not touch the colour space for raw requested roles. >>> + * Even if the pixel format is non-raw (whatever it means), we >>> + * shouldn't try to interpret the colour space of raw data. >>> + */ >>> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >>> + cfg.colorSpace->adjust(pixelFormat); >>> + status = Adjusted; >>> + } >>> + >>> + 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; >>> + } >>> cfg.colorSpace->adjust(pixelFormat); >>> - status = Adjusted; >>> - } >>> - 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; >>> + status = Adjusted; >>> } >>> - cfg.colorSpace->adjust(pixelFormat); >>> - status = Adjusted; >>> } >>> >>> - if (!pipeConfig_->outputSizes.contains(cfg.size)) { >>> + if (!rawStream && !pipeConfig_->outputSizes.contains(cfg.size)) { >>> Size adjustedSize = pipeConfig_->captureSize; >>> /* >>> * The converter (when present) may not be able to output >>> @@ -1259,11 +1301,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>> >>> /* \todo Create a libcamera core class to group format and size */ >>> if (cfg.pixelFormat != pipeConfig_->captureFormat || >>> - cfg.size != pipeConfig_->captureSize) >>> + cfg.size != pipeConfig_->captureSize) { >>> + if (rawStream) { >>> + LOG(SimplePipeline, Info) >>> + << "Raw output format " << cfg.pixelFormat >>> + << " and size " << cfg.size >>> + << " not matching pipe format " << pipeConfig_->captureFormat >>> + << " and size " << pipeConfig_->captureSize; >>> + return Invalid; >>> + } >>> needConversion_ = true; >>> + } >>> >>> /* Set the stride, frameSize and bufferCount. */ >>> - if (needConversion_) { >>> + if (needConversion_ && !rawStream) { >>> std::tie(cfg.stride, cfg.frameSize) = >>> data_->converter_ >>> ? data_->converter_->strideAndFrameSize(cfg.pixelFormat, >>> -- >>> 2.50.1 >>> > Regards, Umang
On Fri, Jul 11, 2025 at 07:53:38PM +0200, Milan Zamazal wrote: > SimpleCameraConfiguration::validate() looks for the best configuration. > As part of enabling raw stream support, the method must consider raw > streams in addition to the processed streams. > > If only a processed stream is requested, nothing changes. > > If only a raw stream is requested, the pixel format and output size may > not be adjusted. The patch adds checks for this. > > If both processed and raw streams are requested, things get more > complicated. The raw stream is expected to be passed through intact and > all the adjustments are made for the processed streams. We select a > pipe configuration for the processed streams. > > Note that with both processed and raw streams, the requested sizes must > be mutually matching, including resizing due to debayer requirements. > For example, the following `cam' setup is valid for imx219 > > cam -s role=viewfinder,width=1920,height=1080 \ > -s role=raw,width=3280,height=2464 > > rather than > > cam -s role=viewfinder,width=1920,height=1080 \ > -s role=raw,width=1920,height=1080 > > due to the resolution of 1924x1080 actually selected for debayering to > 1920x1080. It is the application responsibility to select the right > parameters for the raw stream. > > Setting up the right configurations is still not enough to make the raw > streams working. Buffer handling must be changed in the simple > pipeline, which is addressed in followup patches. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 119 ++++++++++++++++------- > 1 file changed, 85 insertions(+), 34 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 37abaa0e0..9d87a7ffa 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -27,6 +27,7 @@ > #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> > @@ -1186,6 +1187,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > << "-" << pipeConfig_->captureFormat > << " for max stream size " << maxStreamSize; > > + bool rawRequested = false; > + bool processedRequested = false; > + for (const auto &cfg : config_) > + if (cfg.colorSpace == ColorSpace::Raw) { > + if (rawRequested) { > + LOG(SimplePipeline, Error) > + << "Can't capture multiple raw streams"; > + return Invalid; > + } > + rawRequested = true; > + } else { > + processedRequested = true; > + } > + > /* > * Adjust the requested streams. > * > @@ -1204,43 +1219,70 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > for (unsigned int i = 0; i < config_.size(); ++i) { > StreamConfiguration &cfg = config_[i]; > > + /* > + * If both processed and raw streams are requested, the pipe > + * configuration is set up for the processed stream. The raw > + * configuration needs to be compared against the capture format and > + * size in such a case. > + */ > + const bool rawStream = cfg.colorSpace == ColorSpace::Raw; We should always inspect the Stream or the pixelformat to set flags like this. Inspecting colorspace seems flaky, especially when it is a std::optional<> in CameraConfiguration. > + const bool sideRawStream = rawStream && processedRequested; > + > /* Adjust the 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 (cfg.pixelFormat != pixelFormat) { > - LOG(SimplePipeline, Debug) << "Adjusting pixel format"; > - cfg.pixelFormat = pixelFormat; > - /* > - * Do not touch the colour space for raw requested roles. > - * Even if the pixel format is non-raw (whatever it means), we > - * shouldn't try to interpret the colour space of raw data. > - */ > - if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) > + > + if (!sideRawStream) { > + PixelFormat pixelFormat; > + if (rawStream) { > + 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) { > + if (rawStream) { > + LOG(SimplePipeline, Info) > + << "Raw pixel format " > + << cfg.pixelFormat > + << " doesn't match any of the pipe output formats"; > + return Invalid; We can chose the nearest raw format,size if we need to capture a raw stream. simply return Adjusted in that case. > + } > + LOG(SimplePipeline, Debug) > + << "Adjusting pixel format from " << cfg.pixelFormat > + << " to " << pixelFormat; > + cfg.pixelFormat = pixelFormat; > + /* > + * Do not touch the colour space for raw requested roles. > + * Even if the pixel format is non-raw (whatever it means), we > + * shouldn't try to interpret the colour space of raw data. > + */ > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) > + cfg.colorSpace->adjust(pixelFormat); > + status = Adjusted; > + } > + > + 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; > + } > cfg.colorSpace->adjust(pixelFormat); > - status = Adjusted; > - } > - 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; > + status = Adjusted; > } > - cfg.colorSpace->adjust(pixelFormat); > - status = Adjusted; > } I think I have suggested in one of the replies that colorspace handling can be done separately, to progress them faster. In my raw branch, I only set colorspace for raw streams, nothing else. I expect that other colorspace related should be addressed separately. > > - if (!pipeConfig_->outputSizes.contains(cfg.size)) { > + if (!rawStream && !pipeConfig_->outputSizes.contains(cfg.size)) { > Size adjustedSize = pipeConfig_->captureSize; > /* > * The converter (when present) may not be able to output > @@ -1259,11 +1301,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > /* \todo Create a libcamera core class to group format and size */ > if (cfg.pixelFormat != pipeConfig_->captureFormat || > - cfg.size != pipeConfig_->captureSize) > + cfg.size != pipeConfig_->captureSize) { > + if (rawStream) { > + LOG(SimplePipeline, Info) > + << "Raw output format " << cfg.pixelFormat > + << " and size " << cfg.size > + << " not matching pipe format " << pipeConfig_->captureFormat > + << " and size " << pipeConfig_->captureSize; > + return Invalid; > + } > needConversion_ = true; > + } > > /* Set the stride, frameSize and bufferCount. */ > - if (needConversion_) { > + if (needConversion_ && !rawStream) { > std::tie(cfg.stride, cfg.frameSize) = > data_->converter_ > ? data_->converter_->strideAndFrameSize(cfg.pixelFormat, > -- > 2.50.1 >
Hi Umang, Umang Jain <uajain@igalia.com> writes: > On Fri, Jul 11, 2025 at 07:53:38PM +0200, Milan Zamazal wrote: >> SimpleCameraConfiguration::validate() looks for the best configuration. >> As part of enabling raw stream support, the method must consider raw > >> streams in addition to the processed streams. >> >> If only a processed stream is requested, nothing changes. >> >> If only a raw stream is requested, the pixel format and output size may >> not be adjusted. The patch adds checks for this. >> >> If both processed and raw streams are requested, things get more >> complicated. The raw stream is expected to be passed through intact and >> all the adjustments are made for the processed streams. We select a >> pipe configuration for the processed streams. >> >> Note that with both processed and raw streams, the requested sizes must >> be mutually matching, including resizing due to debayer requirements. >> For example, the following `cam' setup is valid for imx219 >> >> cam -s role=viewfinder,width=1920,height=1080 \ >> -s role=raw,width=3280,height=2464 >> >> rather than >> >> cam -s role=viewfinder,width=1920,height=1080 \ >> -s role=raw,width=1920,height=1080 >> >> due to the resolution of 1924x1080 actually selected for debayering to >> 1920x1080. It is the application responsibility to select the right >> parameters for the raw stream. >> >> Setting up the right configurations is still not enough to make the raw >> streams working. Buffer handling must be changed in the simple >> pipeline, which is addressed in followup patches. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 119 ++++++++++++++++------- >> 1 file changed, 85 insertions(+), 34 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 37abaa0e0..9d87a7ffa 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -27,6 +27,7 @@ >> #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> >> @@ -1186,6 +1187,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> << "-" << pipeConfig_->captureFormat >> << " for max stream size " << maxStreamSize; >> >> + bool rawRequested = false; >> + bool processedRequested = false; >> + for (const auto &cfg : config_) >> + if (cfg.colorSpace == ColorSpace::Raw) { >> + if (rawRequested) { >> + LOG(SimplePipeline, Error) >> + << "Can't capture multiple raw streams"; >> + return Invalid; >> + } >> + rawRequested = true; >> + } else { >> + processedRequested = true; >> + } >> + >> /* >> * Adjust the requested streams. >> * >> @@ -1204,43 +1219,70 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> for (unsigned int i = 0; i < config_.size(); ++i) { >> StreamConfiguration &cfg = config_[i]; >> >> + /* >> + * If both processed and raw streams are requested, the pipe >> + * configuration is set up for the processed stream. The raw >> + * configuration needs to be compared against the capture format and >> + * size in such a case. >> + */ >> + const bool rawStream = cfg.colorSpace == ColorSpace::Raw; > > We should always inspect the Stream or the pixelformat to set flags like > this. Inspecting colorspace seems flaky, especially when it is a > std::optional<> in CameraConfiguration. My idea was to (mis)use this to pass the intention about the stream role from generateConfiguration(). But selecting a consistent configuration already in generateConfiguration(), something like what you do in your patch, looks like a better idea. >> + const bool sideRawStream = rawStream && processedRequested; >> + >> /* Adjust the 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 (cfg.pixelFormat != pixelFormat) { >> - LOG(SimplePipeline, Debug) << "Adjusting pixel format"; >> - cfg.pixelFormat = pixelFormat; >> - /* >> - * Do not touch the colour space for raw requested roles. >> - * Even if the pixel format is non-raw (whatever it means), we >> - * shouldn't try to interpret the colour space of raw data. >> - */ >> - if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >> + >> + if (!sideRawStream) { >> + PixelFormat pixelFormat; >> + if (rawStream) { >> + 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) { >> + if (rawStream) { >> + LOG(SimplePipeline, Info) >> + << "Raw pixel format " >> + << cfg.pixelFormat >> + << " doesn't match any of the pipe output formats"; >> + return Invalid; > > We can chose the nearest raw format,size if we need to capture a raw stream. > simply return Adjusted in that case. Yes, unless a specific size for the stream was requested. >> + } >> + LOG(SimplePipeline, Debug) >> + << "Adjusting pixel format from " << cfg.pixelFormat >> + << " to " << pixelFormat; >> + cfg.pixelFormat = pixelFormat; >> + /* >> + * Do not touch the colour space for raw requested roles. >> + * Even if the pixel format is non-raw (whatever it means), we >> + * shouldn't try to interpret the colour space of raw data. >> + */ >> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >> + cfg.colorSpace->adjust(pixelFormat); >> + status = Adjusted; >> + } >> + >> + 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; >> + } >> cfg.colorSpace->adjust(pixelFormat); >> - status = Adjusted; >> - } >> - 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; >> + status = Adjusted; >> } >> - cfg.colorSpace->adjust(pixelFormat); >> - status = Adjusted; >> } > > I think I have suggested in one of the replies that colorspace handling > can be done separately, to progress them faster. The hunk above is just moving the code. > In my raw branch, I only set colorspace for raw streams, nothing else. > I expect that other colorspace related should be addressed separately. The basic colour space assignment is introduced in my "Assign colour spaces in configurations" patch, which is important to prevent crashes when the colour space is unset. >> - if (!pipeConfig_->outputSizes.contains(cfg.size)) { >> + if (!rawStream && !pipeConfig_->outputSizes.contains(cfg.size)) { >> Size adjustedSize = pipeConfig_->captureSize; >> /* >> * The converter (when present) may not be able to output >> @@ -1259,11 +1301,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> >> /* \todo Create a libcamera core class to group format and size */ >> if (cfg.pixelFormat != pipeConfig_->captureFormat || >> - cfg.size != pipeConfig_->captureSize) >> + cfg.size != pipeConfig_->captureSize) { >> + if (rawStream) { >> + LOG(SimplePipeline, Info) >> + << "Raw output format " << cfg.pixelFormat >> + << " and size " << cfg.size >> + << " not matching pipe format " << pipeConfig_->captureFormat >> + << " and size " << pipeConfig_->captureSize; >> + return Invalid; >> + } >> needConversion_ = true; >> + } >> >> /* Set the stride, frameSize and bufferCount. */ >> - if (needConversion_) { >> + if (needConversion_ && !rawStream) { >> std::tie(cfg.stride, cfg.frameSize) = >> data_->converter_ >> ? data_->converter_->strideAndFrameSize(cfg.pixelFormat, >> -- >> 2.50.1 >>
On Mon, Jul 21, 2025 at 10:24:21PM +0200, Milan Zamazal wrote: > Hi Umang, > > Umang Jain <uajain@igalia.com> writes: > > > On Fri, Jul 11, 2025 at 07:53:38PM +0200, Milan Zamazal wrote: > >> SimpleCameraConfiguration::validate() looks for the best configuration. > >> As part of enabling raw stream support, the method must consider raw > > > >> streams in addition to the processed streams. > >> > >> If only a processed stream is requested, nothing changes. > >> > >> If only a raw stream is requested, the pixel format and output size may > >> not be adjusted. The patch adds checks for this. > >> > >> If both processed and raw streams are requested, things get more > >> complicated. The raw stream is expected to be passed through intact and > >> all the adjustments are made for the processed streams. We select a > >> pipe configuration for the processed streams. > >> > >> Note that with both processed and raw streams, the requested sizes must > >> be mutually matching, including resizing due to debayer requirements. > >> For example, the following `cam' setup is valid for imx219 > >> > >> cam -s role=viewfinder,width=1920,height=1080 \ > >> -s role=raw,width=3280,height=2464 > >> > >> rather than > >> > >> cam -s role=viewfinder,width=1920,height=1080 \ > >> -s role=raw,width=1920,height=1080 > >> > >> due to the resolution of 1924x1080 actually selected for debayering to > >> 1920x1080. It is the application responsibility to select the right > >> parameters for the raw stream. > >> > >> Setting up the right configurations is still not enough to make the raw > >> streams working. Buffer handling must be changed in the simple > >> pipeline, which is addressed in followup patches. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/libcamera/pipeline/simple/simple.cpp | 119 ++++++++++++++++------- > >> 1 file changed, 85 insertions(+), 34 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > >> index 37abaa0e0..9d87a7ffa 100644 > >> --- a/src/libcamera/pipeline/simple/simple.cpp > >> +++ b/src/libcamera/pipeline/simple/simple.cpp > >> @@ -27,6 +27,7 @@ > >> #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> > >> @@ -1186,6 +1187,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > >> << "-" << pipeConfig_->captureFormat > >> << " for max stream size " << maxStreamSize; > >> > >> + bool rawRequested = false; > >> + bool processedRequested = false; > >> + for (const auto &cfg : config_) > >> + if (cfg.colorSpace == ColorSpace::Raw) { > >> + if (rawRequested) { > >> + LOG(SimplePipeline, Error) > >> + << "Can't capture multiple raw streams"; > >> + return Invalid; > >> + } > >> + rawRequested = true; > >> + } else { > >> + processedRequested = true; > >> + } > >> + > >> /* > >> * Adjust the requested streams. > >> * > >> @@ -1204,43 +1219,70 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > >> for (unsigned int i = 0; i < config_.size(); ++i) { > >> StreamConfiguration &cfg = config_[i]; > >> > >> + /* > >> + * If both processed and raw streams are requested, the pipe > >> + * configuration is set up for the processed stream. The raw > >> + * configuration needs to be compared against the capture format and > >> + * size in such a case. > >> + */ > >> + const bool rawStream = cfg.colorSpace == ColorSpace::Raw; > > > > We should always inspect the Stream or the pixelformat to set flags like > > this. Inspecting colorspace seems flaky, especially when it is a > > std::optional<> in CameraConfiguration. > > My idea was to (mis)use this to pass the intention about the stream role > from generateConfiguration(). But selecting a consistent configuration > already in generateConfiguration(), something like what you do in your > patch, looks like a better idea. > > >> + const bool sideRawStream = rawStream && processedRequested; > >> + > >> /* Adjust the 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 (cfg.pixelFormat != pixelFormat) { > >> - LOG(SimplePipeline, Debug) << "Adjusting pixel format"; > >> - cfg.pixelFormat = pixelFormat; > >> - /* > >> - * Do not touch the colour space for raw requested roles. > >> - * Even if the pixel format is non-raw (whatever it means), we > >> - * shouldn't try to interpret the colour space of raw data. > >> - */ > >> - if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) > >> + > >> + if (!sideRawStream) { > >> + PixelFormat pixelFormat; > >> + if (rawStream) { > >> + 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) { > >> + if (rawStream) { > >> + LOG(SimplePipeline, Info) > >> + << "Raw pixel format " > >> + << cfg.pixelFormat > >> + << " doesn't match any of the pipe output formats"; > >> + return Invalid; > > > > We can chose the nearest raw format,size if we need to capture a raw stream. > > simply return Adjusted in that case. > > Yes, unless a specific size for the stream was requested. I mean for the size as well. If the specific size is requested, the pipeline is free is to adjust it and return the closest it to satisfy the user requirements. > > >> + } > >> + LOG(SimplePipeline, Debug) > >> + << "Adjusting pixel format from " << cfg.pixelFormat > >> + << " to " << pixelFormat; > >> + cfg.pixelFormat = pixelFormat; > >> + /* > >> + * Do not touch the colour space for raw requested roles. > >> + * Even if the pixel format is non-raw (whatever it means), we > >> + * shouldn't try to interpret the colour space of raw data. > >> + */ > >> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) > >> + cfg.colorSpace->adjust(pixelFormat); > >> + status = Adjusted; > >> + } > >> + > >> + 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; > >> + } > >> cfg.colorSpace->adjust(pixelFormat); > >> - status = Adjusted; > >> - } > >> - 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; > >> + status = Adjusted; > >> } > >> - cfg.colorSpace->adjust(pixelFormat); > >> - status = Adjusted; > >> } > > > > I think I have suggested in one of the replies that colorspace handling > > can be done separately, to progress them faster. > > The hunk above is just moving the code. > > > In my raw branch, I only set colorspace for raw streams, nothing else. > > I expect that other colorspace related should be addressed separately. > > The basic colour space assignment is introduced in my "Assign colour > spaces in configurations" patch, which is important to prevent crashes > when the colour space is unset. Yeah, this should definitely be separate. This is not related to RAW support. > > >> - if (!pipeConfig_->outputSizes.contains(cfg.size)) { > >> + if (!rawStream && !pipeConfig_->outputSizes.contains(cfg.size)) { > >> Size adjustedSize = pipeConfig_->captureSize; > >> /* > >> * The converter (when present) may not be able to output > >> @@ -1259,11 +1301,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > >> > >> /* \todo Create a libcamera core class to group format and size */ > >> if (cfg.pixelFormat != pipeConfig_->captureFormat || > >> - cfg.size != pipeConfig_->captureSize) > >> + cfg.size != pipeConfig_->captureSize) { > >> + if (rawStream) { > >> + LOG(SimplePipeline, Info) > >> + << "Raw output format " << cfg.pixelFormat > >> + << " and size " << cfg.size > >> + << " not matching pipe format " << pipeConfig_->captureFormat > >> + << " and size " << pipeConfig_->captureSize; > >> + return Invalid; > >> + } > >> needConversion_ = true; > >> + } > >> > >> /* Set the stride, frameSize and bufferCount. */ > >> - if (needConversion_) { > >> + if (needConversion_ && !rawStream) { > >> std::tie(cfg.stride, cfg.frameSize) = > >> data_->converter_ > >> ? data_->converter_->strideAndFrameSize(cfg.pixelFormat, > >> -- > >> 2.50.1 > >> >
Umang Jain <uajain@igalia.com> writes: > On Mon, Jul 21, 2025 at 10:24:21PM +0200, Milan Zamazal wrote: >> Hi Umang, >> > >> Umang Jain <uajain@igalia.com> writes: >> >> > On Fri, Jul 11, 2025 at 07:53:38PM +0200, Milan Zamazal wrote: >> >> SimpleCameraConfiguration::validate() looks for the best configuration. >> >> As part of enabling raw stream support, the method must consider raw >> > >> >> streams in addition to the processed streams. >> >> >> >> If only a processed stream is requested, nothing changes. >> >> >> >> If only a raw stream is requested, the pixel format and output size may >> >> not be adjusted. The patch adds checks for this. >> >> >> >> If both processed and raw streams are requested, things get more >> >> complicated. The raw stream is expected to be passed through intact and >> >> all the adjustments are made for the processed streams. We select a >> >> pipe configuration for the processed streams. >> >> >> >> Note that with both processed and raw streams, the requested sizes must >> >> be mutually matching, including resizing due to debayer requirements. >> >> For example, the following `cam' setup is valid for imx219 >> >> >> >> cam -s role=viewfinder,width=1920,height=1080 \ >> >> -s role=raw,width=3280,height=2464 >> >> >> >> rather than >> >> >> >> cam -s role=viewfinder,width=1920,height=1080 \ >> >> -s role=raw,width=1920,height=1080 >> >> >> >> due to the resolution of 1924x1080 actually selected for debayering to >> >> 1920x1080. It is the application responsibility to select the right >> >> parameters for the raw stream. >> >> >> >> Setting up the right configurations is still not enough to make the raw >> >> streams working. Buffer handling must be changed in the simple >> >> pipeline, which is addressed in followup patches. >> >> >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> >> --- >> >> src/libcamera/pipeline/simple/simple.cpp | 119 ++++++++++++++++------- >> >> 1 file changed, 85 insertions(+), 34 deletions(-) >> >> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> >> index 37abaa0e0..9d87a7ffa 100644 >> >> --- a/src/libcamera/pipeline/simple/simple.cpp >> >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> >> @@ -27,6 +27,7 @@ >> >> #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> >> >> @@ -1186,6 +1187,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> >> << "-" << pipeConfig_->captureFormat >> >> << " for max stream size " << maxStreamSize; >> >> >> >> + bool rawRequested = false; >> >> + bool processedRequested = false; >> >> + for (const auto &cfg : config_) >> >> + if (cfg.colorSpace == ColorSpace::Raw) { >> >> + if (rawRequested) { >> >> + LOG(SimplePipeline, Error) >> >> + << "Can't capture multiple raw streams"; >> >> + return Invalid; >> >> + } >> >> + rawRequested = true; >> >> + } else { >> >> + processedRequested = true; >> >> + } >> >> + >> >> /* >> >> * Adjust the requested streams. >> >> * >> >> @@ -1204,43 +1219,70 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> >> for (unsigned int i = 0; i < config_.size(); ++i) { >> >> StreamConfiguration &cfg = config_[i]; >> >> >> >> + /* >> >> + * If both processed and raw streams are requested, the pipe >> >> + * configuration is set up for the processed stream. The raw >> >> + * configuration needs to be compared against the capture format and >> >> + * size in such a case. >> >> + */ >> >> + const bool rawStream = cfg.colorSpace == ColorSpace::Raw; >> > >> > We should always inspect the Stream or the pixelformat to set flags like >> > this. Inspecting colorspace seems flaky, especially when it is a >> > std::optional<> in CameraConfiguration. >> >> My idea was to (mis)use this to pass the intention about the stream role >> from generateConfiguration(). But selecting a consistent configuration >> already in generateConfiguration(), something like what you do in your >> patch, looks like a better idea. >> >> >> + const bool sideRawStream = rawStream && processedRequested; >> >> + >> >> /* Adjust the 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 (cfg.pixelFormat != pixelFormat) { >> >> - LOG(SimplePipeline, Debug) << "Adjusting pixel format"; >> >> - cfg.pixelFormat = pixelFormat; >> >> - /* >> >> - * Do not touch the colour space for raw requested roles. >> >> - * Even if the pixel format is non-raw (whatever it means), we >> >> - * shouldn't try to interpret the colour space of raw data. >> >> - */ >> >> - if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >> >> + >> >> + if (!sideRawStream) { >> >> + PixelFormat pixelFormat; >> >> + if (rawStream) { >> >> + 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) { >> >> + if (rawStream) { >> >> + LOG(SimplePipeline, Info) >> >> + << "Raw pixel format " >> >> + << cfg.pixelFormat >> >> + << " doesn't match any of the pipe output formats"; >> >> + return Invalid; >> > >> > We can chose the nearest raw format,size if we need to capture a raw stream. >> > simply return Adjusted in that case. >> >> Yes, unless a specific size for the stream was requested. > > I mean for the size as well. If the specific size is requested, the > pipeline is free is to adjust it and return the closest it to satisfy > the user requirements. I see, OK. >> >> + } >> >> + LOG(SimplePipeline, Debug) >> >> + << "Adjusting pixel format from " << cfg.pixelFormat >> >> + << " to " << pixelFormat; >> >> + cfg.pixelFormat = pixelFormat; >> >> + /* >> >> + * Do not touch the colour space for raw requested roles. >> >> + * Even if the pixel format is non-raw (whatever it means), we >> >> + * shouldn't try to interpret the colour space of raw data. >> >> + */ >> >> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >> >> + cfg.colorSpace->adjust(pixelFormat); >> >> + status = Adjusted; >> >> + } >> >> + >> >> + 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; >> >> + } >> >> cfg.colorSpace->adjust(pixelFormat); >> >> - status = Adjusted; >> >> - } >> >> - 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; >> >> + status = Adjusted; >> >> } >> >> - cfg.colorSpace->adjust(pixelFormat); >> >> - status = Adjusted; >> >> } >> > >> > I think I have suggested in one of the replies that colorspace handling >> > can be done separately, to progress them faster. >> >> The hunk above is just moving the code. >> >> > In my raw branch, I only set colorspace for raw streams, nothing else. >> > I expect that other colorspace related should be addressed separately. >> >> The basic colour space assignment is introduced in my "Assign colour >> spaces in configurations" patch, which is important to prevent crashes >> when the colour space is unset. > > Yeah, this should definitely be separate. This is not related to RAW > support. > >> >> >> - if (!pipeConfig_->outputSizes.contains(cfg.size)) { >> >> + if (!rawStream && !pipeConfig_->outputSizes.contains(cfg.size)) { >> >> Size adjustedSize = pipeConfig_->captureSize; >> >> /* >> >> * The converter (when present) may not be able to output >> >> @@ -1259,11 +1301,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >> >> >> >> /* \todo Create a libcamera core class to group format and size */ >> >> if (cfg.pixelFormat != pipeConfig_->captureFormat || >> >> - cfg.size != pipeConfig_->captureSize) >> >> + cfg.size != pipeConfig_->captureSize) { >> >> + if (rawStream) { >> >> + LOG(SimplePipeline, Info) >> >> + << "Raw output format " << cfg.pixelFormat >> >> + << " and size " << cfg.size >> >> + << " not matching pipe format " << pipeConfig_->captureFormat >> >> + << " and size " << pipeConfig_->captureSize; >> >> + return Invalid; >> >> + } >> >> needConversion_ = true; >> >> + } >> >> >> >> /* Set the stride, frameSize and bufferCount. */ >> >> - if (needConversion_) { >> >> + if (needConversion_ && !rawStream) { >> >> std::tie(cfg.stride, cfg.frameSize) = >> >> data_->converter_ >> >> ? data_->converter_->strideAndFrameSize(cfg.pixelFormat, >> >> -- >> >> 2.50.1 >> >> >>
Milan Zamazal <mzamazal@redhat.com> writes: > Hi Umang, > > Umang Jain <uajain@igalia.com> writes: > >> On Fri, Jul 11, 2025 at 07:53:38PM +0200, Milan Zamazal wrote: >>> SimpleCameraConfiguration::validate() looks for the best configuration. >>> As part of enabling raw stream support, the method must consider raw >> >>> streams in addition to the processed streams. >>> >>> If only a processed stream is requested, nothing changes. >>> >>> If only a raw stream is requested, the pixel format and output size may >>> not be adjusted. The patch adds checks for this. >>> >>> If both processed and raw streams are requested, things get more >>> complicated. The raw stream is expected to be passed through intact and >>> all the adjustments are made for the processed streams. We select a >>> pipe configuration for the processed streams. >>> >>> Note that with both processed and raw streams, the requested sizes must >>> be mutually matching, including resizing due to debayer requirements. >>> For example, the following `cam' setup is valid for imx219 >>> >>> cam -s role=viewfinder,width=1920,height=1080 \ >>> -s role=raw,width=3280,height=2464 >>> >>> rather than >>> >>> cam -s role=viewfinder,width=1920,height=1080 \ >>> -s role=raw,width=1920,height=1080 >>> >>> due to the resolution of 1924x1080 actually selected for debayering to >>> 1920x1080. It is the application responsibility to select the right >>> parameters for the raw stream. >>> >>> Setting up the right configurations is still not enough to make the raw >>> streams working. Buffer handling must be changed in the simple >>> pipeline, which is addressed in followup patches. >>> >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> src/libcamera/pipeline/simple/simple.cpp | 119 ++++++++++++++++------- >>> 1 file changed, 85 insertions(+), 34 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >>> index 37abaa0e0..9d87a7ffa 100644 >>> --- a/src/libcamera/pipeline/simple/simple.cpp >>> +++ b/src/libcamera/pipeline/simple/simple.cpp >>> @@ -27,6 +27,7 @@ >>> #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> >>> @@ -1186,6 +1187,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>> << "-" << pipeConfig_->captureFormat >>> << " for max stream size " << maxStreamSize; >>> >>> + bool rawRequested = false; >>> + bool processedRequested = false; >>> + for (const auto &cfg : config_) >>> + if (cfg.colorSpace == ColorSpace::Raw) { >>> + if (rawRequested) { >>> + LOG(SimplePipeline, Error) >>> + << "Can't capture multiple raw streams"; >>> + return Invalid; >>> + } >>> + rawRequested = true; >>> + } else { >>> + processedRequested = true; >>> + } >>> + >>> /* >>> * Adjust the requested streams. >>> * >>> @@ -1204,43 +1219,70 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>> for (unsigned int i = 0; i < config_.size(); ++i) { >>> StreamConfiguration &cfg = config_[i]; >>> >>> + /* >>> + * If both processed and raw streams are requested, the pipe >>> + * configuration is set up for the processed stream. The raw >>> + * configuration needs to be compared against the capture format and >>> + * size in such a case. >>> + */ >>> + const bool rawStream = cfg.colorSpace == ColorSpace::Raw; >> >> We should always inspect the Stream or the pixelformat to set flags like >> this. Inspecting colorspace seems flaky, especially when it is a >> std::optional<> in CameraConfiguration. > > My idea was to (mis)use this to pass the intention about the stream role > from generateConfiguration(). But selecting a consistent configuration > already in generateConfiguration(), something like what you do in your > patch, looks like a better idea. Hmm, but it depends on the assumption that raw streams are exactly those with raw pixel formats, which I doubt is right. If it was right then it would work. But if not then we have little means to do otherwise than misusing the colour space. Specifying raw/non-raw in StreamConfiguration would be completely clear but I've been hesitating to do that because it doesn't fit the other pipelines (and I'm not sure if it'd influence applications in any negative way). Am I getting confused too much? >>> + const bool sideRawStream = rawStream && processedRequested; >>> + >>> /* Adjust the 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 (cfg.pixelFormat != pixelFormat) { >>> - LOG(SimplePipeline, Debug) << "Adjusting pixel format"; >>> - cfg.pixelFormat = pixelFormat; >>> - /* >>> - * Do not touch the colour space for raw requested roles. >>> - * Even if the pixel format is non-raw (whatever it means), we >>> - * shouldn't try to interpret the colour space of raw data. >>> - */ >>> - if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >>> + >>> + if (!sideRawStream) { >>> + PixelFormat pixelFormat; >>> + if (rawStream) { >>> + 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) { >>> + if (rawStream) { >>> + LOG(SimplePipeline, Info) >>> + << "Raw pixel format " >>> + << cfg.pixelFormat >>> + << " doesn't match any of the pipe output formats"; >>> + return Invalid; >> >> We can chose the nearest raw format,size if we need to capture a raw stream. >> simply return Adjusted in that case. > > Yes, unless a specific size for the stream was requested. > >>> + } >>> + LOG(SimplePipeline, Debug) >>> + << "Adjusting pixel format from " << cfg.pixelFormat >>> + << " to " << pixelFormat; >>> + cfg.pixelFormat = pixelFormat; >>> + /* >>> + * Do not touch the colour space for raw requested roles. >>> + * Even if the pixel format is non-raw (whatever it means), we >>> + * shouldn't try to interpret the colour space of raw data. >>> + */ >>> + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) >>> + cfg.colorSpace->adjust(pixelFormat); >>> + status = Adjusted; >>> + } >>> + >>> + 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; >>> + } >>> cfg.colorSpace->adjust(pixelFormat); >>> - status = Adjusted; >>> - } >>> - 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; >>> + status = Adjusted; >>> } >>> - cfg.colorSpace->adjust(pixelFormat); >>> - status = Adjusted; >>> } >> >> I think I have suggested in one of the replies that colorspace handling >> can be done separately, to progress them faster. > > The hunk above is just moving the code. > >> In my raw branch, I only set colorspace for raw streams, nothing else. >> I expect that other colorspace related should be addressed separately. > > The basic colour space assignment is introduced in my "Assign colour > spaces in configurations" patch, which is important to prevent crashes > when the colour space is unset. > >>> - if (!pipeConfig_->outputSizes.contains(cfg.size)) { >>> + if (!rawStream && !pipeConfig_->outputSizes.contains(cfg.size)) { >>> Size adjustedSize = pipeConfig_->captureSize; >>> /* >>> * The converter (when present) may not be able to output >>> @@ -1259,11 +1301,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() >>> >>> /* \todo Create a libcamera core class to group format and size */ >>> if (cfg.pixelFormat != pipeConfig_->captureFormat || >>> - cfg.size != pipeConfig_->captureSize) >>> + cfg.size != pipeConfig_->captureSize) { >>> + if (rawStream) { >>> + LOG(SimplePipeline, Info) >>> + << "Raw output format " << cfg.pixelFormat >>> + << " and size " << cfg.size >>> + << " not matching pipe format " << pipeConfig_->captureFormat >>> + << " and size " << pipeConfig_->captureSize; >>> + return Invalid; >>> + } >>> needConversion_ = true; >>> + } >>> >>> /* Set the stride, frameSize and bufferCount. */ >>> - if (needConversion_) { >>> + if (needConversion_ && !rawStream) { >>> std::tie(cfg.stride, cfg.frameSize) = >>> data_->converter_ >>> ? data_->converter_->strideAndFrameSize(cfg.pixelFormat, >>> -- >>> 2.50.1 >>>
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 37abaa0e0..9d87a7ffa 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -27,6 +27,7 @@ #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> @@ -1186,6 +1187,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() << "-" << pipeConfig_->captureFormat << " for max stream size " << maxStreamSize; + bool rawRequested = false; + bool processedRequested = false; + for (const auto &cfg : config_) + if (cfg.colorSpace == ColorSpace::Raw) { + if (rawRequested) { + LOG(SimplePipeline, Error) + << "Can't capture multiple raw streams"; + return Invalid; + } + rawRequested = true; + } else { + processedRequested = true; + } + /* * Adjust the requested streams. * @@ -1204,43 +1219,70 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() for (unsigned int i = 0; i < config_.size(); ++i) { StreamConfiguration &cfg = config_[i]; + /* + * If both processed and raw streams are requested, the pipe + * configuration is set up for the processed stream. The raw + * configuration needs to be compared against the capture format and + * size in such a case. + */ + const bool rawStream = cfg.colorSpace == ColorSpace::Raw; + const bool sideRawStream = rawStream && processedRequested; + /* Adjust the 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 (cfg.pixelFormat != pixelFormat) { - LOG(SimplePipeline, Debug) << "Adjusting pixel format"; - cfg.pixelFormat = pixelFormat; - /* - * Do not touch the colour space for raw requested roles. - * Even if the pixel format is non-raw (whatever it means), we - * shouldn't try to interpret the colour space of raw data. - */ - if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) + + if (!sideRawStream) { + PixelFormat pixelFormat; + if (rawStream) { + 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) { + if (rawStream) { + LOG(SimplePipeline, Info) + << "Raw pixel format " + << cfg.pixelFormat + << " doesn't match any of the pipe output formats"; + return Invalid; + } + LOG(SimplePipeline, Debug) + << "Adjusting pixel format from " << cfg.pixelFormat + << " to " << pixelFormat; + cfg.pixelFormat = pixelFormat; + /* + * Do not touch the colour space for raw requested roles. + * Even if the pixel format is non-raw (whatever it means), we + * shouldn't try to interpret the colour space of raw data. + */ + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) + cfg.colorSpace->adjust(pixelFormat); + status = Adjusted; + } + + 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; + } cfg.colorSpace->adjust(pixelFormat); - status = Adjusted; - } - 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; + status = Adjusted; } - cfg.colorSpace->adjust(pixelFormat); - status = Adjusted; } - if (!pipeConfig_->outputSizes.contains(cfg.size)) { + if (!rawStream && !pipeConfig_->outputSizes.contains(cfg.size)) { Size adjustedSize = pipeConfig_->captureSize; /* * The converter (when present) may not be able to output @@ -1259,11 +1301,20 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() /* \todo Create a libcamera core class to group format and size */ if (cfg.pixelFormat != pipeConfig_->captureFormat || - cfg.size != pipeConfig_->captureSize) + cfg.size != pipeConfig_->captureSize) { + if (rawStream) { + LOG(SimplePipeline, Info) + << "Raw output format " << cfg.pixelFormat + << " and size " << cfg.size + << " not matching pipe format " << pipeConfig_->captureFormat + << " and size " << pipeConfig_->captureSize; + return Invalid; + } needConversion_ = true; + } /* Set the stride, frameSize and bufferCount. */ - if (needConversion_) { + if (needConversion_ && !rawStream) { std::tie(cfg.stride, cfg.frameSize) = data_->converter_ ? data_->converter_->strideAndFrameSize(cfg.pixelFormat,
SimpleCameraConfiguration::validate() looks for the best configuration. As part of enabling raw stream support, the method must consider raw streams in addition to the processed streams. If only a processed stream is requested, nothing changes. If only a raw stream is requested, the pixel format and output size may not be adjusted. The patch adds checks for this. If both processed and raw streams are requested, things get more complicated. The raw stream is expected to be passed through intact and all the adjustments are made for the processed streams. We select a pipe configuration for the processed streams. Note that with both processed and raw streams, the requested sizes must be mutually matching, including resizing due to debayer requirements. For example, the following `cam' setup is valid for imx219 cam -s role=viewfinder,width=1920,height=1080 \ -s role=raw,width=3280,height=2464 rather than cam -s role=viewfinder,width=1920,height=1080 \ -s role=raw,width=1920,height=1080 due to the resolution of 1924x1080 actually selected for debayering to 1920x1080. It is the application responsibility to select the right parameters for the raw stream. Setting up the right configurations is still not enough to make the raw streams working. Buffer handling must be changed in the simple pipeline, which is addressed in followup patches. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 119 ++++++++++++++++------- 1 file changed, 85 insertions(+), 34 deletions(-)