| Message ID | 20240113142218.28063-16-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi! > > + /* > + * Create SoftwareIsp unconditionally if no converter is used > + * - to be revisited > + */ > + if (!converter_) { > + swIsp_ = SoftwareIspFactoryBase::create(pipe, sensor_->controls()); > + if (!swIsp_) { > + LOG(SimplePipeline, Warning) > + << "Failed to create software ISP, disabling software debayering"; > + swIsp_.reset(); > + } else { > + swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone); > + swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); > + swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); > + > + swIsp_->getSignalSetSensorControls().connect(this, &SimpleCameraData::setSensorControls); > + } > + } > + I guess this needs to be revisited before the merge? Best regards, Pavel
Hi, On 14.01.2024 20:09, Pavel Machek wrote: > Hi! > >> >> + /* >> + * Create SoftwareIsp unconditionally if no converter is used >> + * - to be revisited >> + */ >> + if (!converter_) { >> + swIsp_ = SoftwareIspFactoryBase::create(pipe, sensor_->controls()); >> + if (!swIsp_) { >> + LOG(SimplePipeline, Warning) >> + << "Failed to create software ISP, disabling software debayering"; >> + swIsp_.reset(); >> + } else { >> + swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone); >> + swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); >> + swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); >> + >> + swIsp_->getSignalSetSensorControls().connect(this, &SimpleCameraData::setSensorControls); >> + } >> + } >> + > > I guess this needs to be revisited before the merge? Currently there is a build-time choice of instantiating: -Dpipelines=simple/simple -Dipas=simple/simple or not instantiating: -Dpipelines=simple the Soft ISP and the Soft IPA. Does it need to be a run-time option? How should this work from the user perspective then? For me the only obvious disadvantage of always creating the Soft ISP/IPA if it is enabled in build configuration is that in this case the frames can be captured only in RGB888 or BGR888 formats. Capturing raw bayer data isn't possible, as Soft ISP/IPA always debayers all the raw bayer formats it supports. Thanks, Andrey > Best regards, > Pavel
Hi! > > > + /* > > > + * Create SoftwareIsp unconditionally if no converter is used > > > + * - to be revisited > > > + */ > > > + if (!converter_) { > > > + swIsp_ = SoftwareIspFactoryBase::create(pipe, sensor_->controls()); > > > + if (!swIsp_) { > > > + LOG(SimplePipeline, Warning) > > > + << "Failed to create software ISP, disabling software debayering"; > > > + swIsp_.reset(); > > > + } else { > > > + swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone); > > > + swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); > > > + swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); > > > + > > > + swIsp_->getSignalSetSensorControls().connect(this, &SimpleCameraData::setSensorControls); > > > + } > > > + } > > > + > > > > I guess this needs to be revisited before the merge? > > Currently there is a build-time choice of instantiating: > -Dpipelines=simple/simple -Dipas=simple/simple > or not instantiating: > -Dpipelines=simple > the Soft ISP and the Soft IPA. > > Does it need to be a run-time option? How should this work from the user perspective then? > > For me the only obvious disadvantage of always creating the Soft ISP/IPA if it > is enabled in build configuration is that in this case the frames can be captured > only in RGB888 or BGR888 formats. Capturing raw bayer data isn't possible, as > Soft ISP/IPA always debayers all the raw bayer formats it supports. Well, compile-time option is really bad for distros, right? I'm pretty sure someone out there uses bayer capture (millipixels, for example), and most apps will want RGB888 (cam sdl, for example; likely others). So long-term we'll really want to support both. Not sure it needs to be solved before merge, but since we had a TODO in the comment, I wanted to point it out. Best regards, Pavel
Hi Pavel, On 15.01.2024 11:15, Pavel Machek wrote: > Hi! > >>>> + /* >>>> + * Create SoftwareIsp unconditionally if no converter is used >>>> + * - to be revisited >>>> + */ >>>> + if (!converter_) { >>>> + swIsp_ = SoftwareIspFactoryBase::create(pipe, sensor_->controls()); >>>> + if (!swIsp_) { >>>> + LOG(SimplePipeline, Warning) >>>> + << "Failed to create software ISP, disabling software debayering"; >>>> + swIsp_.reset(); >>>> + } else { >>>> + swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone); >>>> + swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); >>>> + swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); >>>> + >>>> + swIsp_->getSignalSetSensorControls().connect(this, &SimpleCameraData::setSensorControls); >>>> + } >>>> + } >>>> + >>> >>> I guess this needs to be revisited before the merge? >> >> Currently there is a build-time choice of instantiating: >> -Dpipelines=simple/simple -Dipas=simple/simple >> or not instantiating: >> -Dpipelines=simple >> the Soft ISP and the Soft IPA. >> >> Does it need to be a run-time option? How should this work from the user perspective then? >> >> For me the only obvious disadvantage of always creating the Soft ISP/IPA if it >> is enabled in build configuration is that in this case the frames can be captured >> only in RGB888 or BGR888 formats. Capturing raw bayer data isn't possible, as >> Soft ISP/IPA always debayers all the raw bayer formats it supports. > > Well, compile-time option is really bad for distros, right? I'm pretty > sure someone out there uses bayer capture (millipixels, for example), > and most apps will want RGB888 (cam sdl, for example; likely others). OK, so the only concern is the ability to capture the raw frames. This is probably fairly easy to fix. > So long-term we'll really want to support both. Not sure it needs to > be solved before merge, but since we had a TODO in the comment, I > wanted to point it out. I see. Thanks for the remainder! Let me see if I can fix that before the next version of the patch set. Thanks, Andrei > Best regards, > Pavel
Quoting Andrei Konovalov via libcamera-devel (2024-01-15 12:30:24) > Hi Pavel, > > On 15.01.2024 11:15, Pavel Machek wrote: > > Hi! > > > >>>> + /* > >>>> + * Create SoftwareIsp unconditionally if no converter is used > >>>> + * - to be revisited > >>>> + */ > >>>> + if (!converter_) { > >>>> + swIsp_ = SoftwareIspFactoryBase::create(pipe, sensor_->controls()); > >>>> + if (!swIsp_) { > >>>> + LOG(SimplePipeline, Warning) > >>>> + << "Failed to create software ISP, disabling software debayering"; > >>>> + swIsp_.reset(); > >>>> + } else { > >>>> + swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone); > >>>> + swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); > >>>> + swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); > >>>> + > >>>> + swIsp_->getSignalSetSensorControls().connect(this, &SimpleCameraData::setSensorControls); > >>>> + } > >>>> + } > >>>> + > >>> > >>> I guess this needs to be revisited before the merge? > >> > >> Currently there is a build-time choice of instantiating: > >> -Dpipelines=simple/simple -Dipas=simple/simple > >> or not instantiating: > >> -Dpipelines=simple > >> the Soft ISP and the Soft IPA. > >> > >> Does it need to be a run-time option? How should this work from the user perspective then? > >> > >> For me the only obvious disadvantage of always creating the Soft ISP/IPA if it > >> is enabled in build configuration is that in this case the frames can be captured > >> only in RGB888 or BGR888 formats. Capturing raw bayer data isn't possible, as > >> Soft ISP/IPA always debayers all the raw bayer formats it supports. > > > > Well, compile-time option is really bad for distros, right? I'm pretty > > sure someone out there uses bayer capture (millipixels, for example), > > and most apps will want RGB888 (cam sdl, for example; likely others). > > OK, so the only concern is the ability to capture the raw frames. > This is probably fairly easy to fix. > > > So long-term we'll really want to support both. Not sure it needs to > > be solved before merge, but since we had a TODO in the comment, I > > wanted to point it out. > > I see. Thanks for the remainder! Let me see if I can fix that before the > next version of the patch set. Ideally - the SoftISP should always be compiled, and then can be enabled on a platform basis through the table at https://gitlab.freedesktop.org/camera/libcamera-softisp/-/blob/SoftwareISP-v05/src/libcamera/pipeline/simple/simple.cpp#L193 It could also be that in the future we might expect a RAW stream output as well as an RGB888 so that the 3a can run to handle the autoexposure/gains while still outputing RAW images. But that can be later. Just moving to the SoftIPA being compiled with the simple pipeline handler, and enabled on a platform basis is enough to start with I believe. -- Kieran > > Thanks, > Andrei > > > Best regards, > > Pavel
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index fa298746..c76510c2 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -34,6 +34,7 @@ #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/pipeline_handler.h" +#include "libcamera/internal/software_isp.h" #include "libcamera/internal/v4l2_subdevice.h" #include "libcamera/internal/v4l2_videodevice.h" @@ -274,6 +275,7 @@ public: bool useConversion_; std::unique_ptr<Converter> converter_; + std::unique_ptr<SoftwareIsp> swIsp_; private: void tryPipeline(unsigned int code, const Size &size); @@ -281,6 +283,9 @@ private: void conversionInputDone(FrameBuffer *buffer); void conversionOutputDone(FrameBuffer *buffer); + + void ispStatsReady(int dummy); + void setSensorControls(const ControlList &sensorControls); }; class SimpleCameraConfiguration : public CameraConfiguration @@ -509,6 +514,25 @@ int SimpleCameraData::init() } } + /* + * Create SoftwareIsp unconditionally if no converter is used + * - to be revisited + */ + if (!converter_) { + swIsp_ = SoftwareIspFactoryBase::create(pipe, sensor_->controls()); + if (!swIsp_) { + LOG(SimplePipeline, Warning) + << "Failed to create software ISP, disabling software debayering"; + swIsp_.reset(); + } else { + swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone); + swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); + swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); + + swIsp_->getSignalSetSensorControls().connect(this, &SimpleCameraData::setSensorControls); + } + } + video_ = pipe->video(entities_.back().entity); ASSERT(video_); @@ -599,12 +623,20 @@ void SimpleCameraData::tryPipeline(unsigned int code, const Size &size) config.captureFormat = pixelFormat; config.captureSize = format.size; - if (!converter_) { - config.outputFormats = { pixelFormat }; - config.outputSizes = config.captureSize; - } else { + if (converter_) { config.outputFormats = converter_->formats(pixelFormat); config.outputSizes = converter_->sizes(format.size); + } else if (swIsp_) { + config.outputFormats = swIsp_->formats(pixelFormat); + config.outputSizes = swIsp_->sizes(pixelFormat, format.size); + if (config.outputFormats.empty()) { + /* Do not use swIsp for unsupported pixelFormat's */ + config.outputFormats = { pixelFormat }; + config.outputSizes = config.captureSize; + } + } else { + config.outputFormats = { pixelFormat }; + config.outputSizes = config.captureSize; } configs_.push_back(config); @@ -750,9 +782,9 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) } /* - * The converter is in use. Requeue the internal buffer for - * capture (unless the stream is being stopped), and complete - * the request with all the user-facing buffers. + * The converter or Software ISP is in use. Requeue the internal + * 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) video_->queueBuffer(buffer); @@ -798,9 +830,9 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) buffer->metadata().timestamp); /* - * Queue the captured and the request buffer to the converter if format - * conversion is needed. If there's no queued request, just requeue the - * captured buffer for capture. + * Queue the captured and the request buffer to the converter or Software + * ISP if format conversion is needed. If there's no queued request, just + * requeue the captured buffer for capture. */ if (useConversion_) { if (conversionQueue_.empty()) { @@ -808,7 +840,11 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) return; } - converter_->queueBuffers(buffer, conversionQueue_.front()); + if (converter_) + converter_->queueBuffers(buffer, conversionQueue_.front()); + else + swIsp_->queueBuffers(buffer, conversionQueue_.front()); + conversionQueue_.pop(); return; } @@ -834,6 +870,18 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) pipe->completeRequest(request); } +void SimpleCameraData::ispStatsReady([[maybe_unused]] int dummy) +{ + swIsp_->processStats(sensor_->getControls({ V4L2_CID_ANALOGUE_GAIN, + V4L2_CID_EXPOSURE })); +} + +void SimpleCameraData::setSensorControls(const ControlList &sensorControls) +{ + ControlList ctrls(sensorControls); + sensor_->setControls(&ctrls); +} + /* Retrieve all source pads connected to a sink pad through active routes. */ std::vector<const MediaPad *> SimpleCameraData::routedSourcePads(MediaPad *sink) { @@ -1016,8 +1064,10 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() /* Set the stride, frameSize and bufferCount. */ if (needConversion_) { std::tie(cfg.stride, cfg.frameSize) = - data_->converter_->strideAndFrameSize(cfg.pixelFormat, - cfg.size); + (data_->converter_) ? data_->converter_->strideAndFrameSize(cfg.pixelFormat, + cfg.size) + : data_->swIsp_->strideAndFrameSize(cfg.pixelFormat, + cfg.size); if (cfg.stride == 0) return Invalid; } else { @@ -1180,7 +1230,9 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c) inputCfg.stride = captureFormat.planes[0].bpl; inputCfg.bufferCount = kNumInternalBuffers; - return data->converter_->configure(inputCfg, outputCfgs); + return (data->converter_) ? data->converter_->configure(inputCfg, outputCfgs) + : data->swIsp_->configure(inputCfg, outputCfgs, + data->sensor_->controls()); } int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream, @@ -1194,8 +1246,10 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream, * whether the converter is used or not. */ if (data->useConversion_) - return data->converter_->exportBuffers(data->streamIndex(stream), - count, buffers); + return (data->converter_) ? data->converter_->exportBuffers(data->streamIndex(stream), + count, buffers) + : data->swIsp_->exportBuffers(data->streamIndex(stream), + count, buffers); else return data->video_->exportBuffers(count, buffers); } @@ -1240,10 +1294,18 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL } if (data->useConversion_) { - ret = data->converter_->start(); - if (ret < 0) { - stop(camera); - return ret; + if (data->converter_) { + ret = data->converter_->start(); + if (ret < 0) { + stop(camera); + return ret; + } + } else if (data->swIsp_) { + ret = data->swIsp_->start(); + if (ret < 0) { + stop(camera); + return ret; + } } /* Queue all internal buffers for capture. */ @@ -1259,8 +1321,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera) SimpleCameraData *data = cameraData(camera); V4L2VideoDevice *video = data->video_; - if (data->useConversion_) - data->converter_->stop(); + if (data->useConversion_) { + if (data->converter_) + data->converter_->stop(); + else if (data->swIsp_) { + data->swIsp_->stop(); + } + } video->streamOff(); video->releaseBuffers();