| Message ID | 20251021182716.29274-9-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Tue, Oct 21, 2025 at 08:27:15PM +0200, Milan Zamazal wrote: > When a raw stream is requested, whether alone or together with a > processed stream, its buffers must be handled outside the software ISP > machinery. They serve as output buffers, even when a processed stream > is produced. > > At most one raw stream and at most one processed stream are supported > and can be combined. An example of producing both raw and processed > files using `cam' application: > > cam -c1 -C100 -Ffile# \ > -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ > -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ > > Note the difference in viewfinder and raw stream sizes due to the fact > that debayering requires enlarging the image width, which enforces > selecting a larger sensor resolution in this case. > > In order to track whether a raw stream is requested and which one it is, > SimpleCameraData::rawStream_ member variable is introduced. > > This is the final step to make raw streams working. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- > 1 file changed, 36 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 0c3284feb..607b07949 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -333,6 +333,7 @@ public: > }; > > std::vector<Stream> streams_; > + Stream *rawStream_; > > /* > * All entities in the pipeline, from the camera sensor to the video > @@ -371,6 +372,11 @@ private: > void ispStatsReady(uint32_t frame, uint32_t bufferId); > void metadataReady(uint32_t frame, const ControlList &metadata); > void setSensorControls(const ControlList &sensorControls); > + > + bool processedRequested() const > + { > + return streams_.size() - (rawStream_ ? 1 : 0) > 0; > + } > }; > > class SimpleCameraConfiguration : public CameraConfiguration > @@ -467,7 +473,7 @@ private: > SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, > unsigned int numStreams, > MediaEntity *sensor) > - : Camera::Private(pipe), streams_(numStreams) > + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) > { > /* > * Find the shortest path from the camera sensor to a video capture > @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > * point converting an erroneous buffer. > */ > if (buffer->metadata().status != FrameMetadata::FrameSuccess) { > - if (!useConversion_) { > + if (rawStream_) { [1] I really don't understand the change here. Also doesn't it break the case when only one processed stream is requested and an error occured ? > /* 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; If this change is trying to adapt the if condition in tryCompleteRequest(), I believe this should be a different patch. For now, I don't see how it relates to raw stream enabling. > tryCompleteRequest(request); > return; > } > @@ -891,7 +900,8 @@ 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) > + if (buffer->metadata().status != FrameMetadata::FrameCancelled && > + !rawStream_) Do you need to check for !rawStream_ again? The codepath would have returned early already if I consider the change above at [1], no? > video_->queueBuffer(buffer); > > if (conversionQueue_.empty()) > @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > */ > if (useConversion_) { > if (conversionQueue_.empty()) { > - video_->queueBuffer(buffer); > + if (!rawStream_) > + video_->queueBuffer(buffer); > return; > } > > if (converter_) > converter_->queueBuffers(buffer, conversionQueue_.front().outputs); > - else > + else if (processedRequested()) Isn't this implicit, if useConversion_=True, you either go through converter_ or softISP. We don't need to check for processedRequested() here. > /* > * request->sequence() cannot be retrieved from `buffer' inside > * queueBuffers because unique_ptr's make buffer->request() invalid > @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > conversionQueue_.front().outputs); > > conversionQueue_.pop(); > + if (rawStream_) > + pipe->completeBuffer(request, buffer); This shouldn't be needed either, the function has pipe->completeBuffer() out of this if block. > return; > } > > @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) > { > /* Queue the input buffer back for capture. */ > - video_->queueBuffer(buffer); > + if (!rawStream_) > + video_->queueBuffer(buffer); Don't you need to also complete the raw buffer in else block ? This would be the case where only raw stream is requested. Or is it done elsewhere? > } > > void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) > @@ -1518,11 +1532,15 @@ 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); > > cfg.setStream(&data->streams_[i]); > > - if (data->useConversion_ && !isRaw(cfg)) > + if (data->useConversion_ && !rawStream) > outputCfgs.push_back(cfg); > + > + if (rawStream) > + data->rawStream_ = &data->streams_[i]; > } > > if (outputCfgs.empty()) > @@ -1553,7 +1571,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_) > + if (data->useConversion_ && stream != data->rawStream_) > return data->converter_ > ? data->converter_->exportBuffers(stream, count, buffers) > : data->swIsp_->exportBuffers(stream, count, buffers); > @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > return -EBUSY; > } > > - if (data->useConversion_) { > + if (data->useConversion_ && !data->rawStream_) { > /* > * When using the converter allocate a fixed number of internal > * buffers. > @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > ret = video->allocateBuffers(kNumInternalBuffers, > &data->conversionBuffers_); > } else { > - /* Otherwise, prepare for using buffers from the only stream. */ > - Stream *stream = &data->streams_[0]; > + /* > + * 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]); > ret = video->importBuffers(stream->configuration().bufferCount); > } > if (ret < 0) { > @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > } > > /* Queue all internal buffers for capture. */ > - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) > - video->queueBuffer(buffer.get()); > + if (!data->rawStream_) > + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) > + video->queueBuffer(buffer.get()); > } > > return 0; > @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > * queue, it will be handed to the converter in the capture > * completion handler. > */ > - if (data->useConversion_) { > + if (data->useConversion_ && stream != data->rawStream_) { > buffers.emplace(stream, buffer); > metadataRequired = !!data->swIsp_; > } else { > -- > 2.51.0 >
I reviewed most of the patches and am OK with them. I prefer a slight rework in 4/8 and have highlighted it (with a reference diff I implemented earlier). Unfortuantely, I did find patch 8/8 hard-to-follow :( But if it's just me, it would be better if someone else should take a look. On Tue, Oct 21, 2025 at 08:27:15PM +0200, Milan Zamazal wrote: > When a raw stream is requested, whether alone or together with a > processed stream, its buffers must be handled outside the software ISP > machinery. They serve as output buffers, even when a processed stream > is produced. > > At most one raw stream and at most one processed stream are supported > and can be combined. An example of producing both raw and processed > files using `cam' application: > > cam -c1 -C100 -Ffile# \ > -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ > -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ > > Note the difference in viewfinder and raw stream sizes due to the fact > that debayering requires enlarging the image width, which enforces > selecting a larger sensor resolution in this case. > > In order to track whether a raw stream is requested and which one it is, > SimpleCameraData::rawStream_ member variable is introduced. > > This is the final step to make raw streams working. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- > 1 file changed, 36 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 0c3284feb..607b07949 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -333,6 +333,7 @@ public: > }; > > std::vector<Stream> streams_; > + Stream *rawStream_; > > /* > * All entities in the pipeline, from the camera sensor to the video > @@ -371,6 +372,11 @@ private: > void ispStatsReady(uint32_t frame, uint32_t bufferId); > void metadataReady(uint32_t frame, const ControlList &metadata); > void setSensorControls(const ControlList &sensorControls); > + > + bool processedRequested() const > + { > + return streams_.size() - (rawStream_ ? 1 : 0) > 0; > + } > }; > > class SimpleCameraConfiguration : public CameraConfiguration > @@ -467,7 +473,7 @@ private: > SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, > unsigned int numStreams, > MediaEntity *sensor) > - : Camera::Private(pipe), streams_(numStreams) > + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) > { > /* > * Find the shortest path from the camera sensor to a video capture > @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > * point converting an erroneous buffer. > */ > if (buffer->metadata().status != FrameMetadata::FrameSuccess) { > - if (!useConversion_) { > + if (rawStream_) { > /* 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; > } > @@ -891,7 +900,8 @@ 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) > + if (buffer->metadata().status != FrameMetadata::FrameCancelled && > + !rawStream_) > video_->queueBuffer(buffer); > > if (conversionQueue_.empty()) > @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > */ > if (useConversion_) { > if (conversionQueue_.empty()) { > - video_->queueBuffer(buffer); > + if (!rawStream_) > + video_->queueBuffer(buffer); > return; > } > > if (converter_) > converter_->queueBuffers(buffer, conversionQueue_.front().outputs); > - else > + else if (processedRequested()) > /* > * request->sequence() cannot be retrieved from `buffer' inside > * queueBuffers because unique_ptr's make buffer->request() invalid > @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > conversionQueue_.front().outputs); > > conversionQueue_.pop(); > + if (rawStream_) > + pipe->completeBuffer(request, buffer); > return; > } > > @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) > { > /* Queue the input buffer back for capture. */ > - video_->queueBuffer(buffer); > + if (!rawStream_) > + video_->queueBuffer(buffer); > } > > void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) > @@ -1518,11 +1532,15 @@ 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); > > cfg.setStream(&data->streams_[i]); > > - if (data->useConversion_ && !isRaw(cfg)) > + if (data->useConversion_ && !rawStream) > outputCfgs.push_back(cfg); > + > + if (rawStream) > + data->rawStream_ = &data->streams_[i]; > } > > if (outputCfgs.empty()) > @@ -1553,7 +1571,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_) > + if (data->useConversion_ && stream != data->rawStream_) > return data->converter_ > ? data->converter_->exportBuffers(stream, count, buffers) > : data->swIsp_->exportBuffers(stream, count, buffers); > @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > return -EBUSY; > } > > - if (data->useConversion_) { > + if (data->useConversion_ && !data->rawStream_) { > /* > * When using the converter allocate a fixed number of internal > * buffers. > @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > ret = video->allocateBuffers(kNumInternalBuffers, > &data->conversionBuffers_); > } else { > - /* Otherwise, prepare for using buffers from the only stream. */ > - Stream *stream = &data->streams_[0]; > + /* > + * 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]); > ret = video->importBuffers(stream->configuration().bufferCount); > } > if (ret < 0) { > @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > } > > /* Queue all internal buffers for capture. */ > - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) > - video->queueBuffer(buffer.get()); > + if (!data->rawStream_) > + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) > + video->queueBuffer(buffer.get()); > } > > return 0; > @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > * queue, it will be handed to the converter in the capture > * completion handler. > */ > - if (data->useConversion_) { > + if (data->useConversion_ && stream != data->rawStream_) { > buffers.emplace(stream, buffer); > metadataRequired = !!data->swIsp_; > } else { > -- > 2.51.0 >
On Tue, Oct 21, 2025 at 08:27:15PM +0200, Milan Zamazal wrote: > When a raw stream is requested, whether alone or together with a > processed stream, its buffers must be handled outside the software ISP > machinery. They serve as output buffers, even when a processed stream > is produced. > > At most one raw stream and at most one processed stream are supported > and can be combined. An example of producing both raw and processed > files using `cam' application: > > cam -c1 -C100 -Ffile# \ > -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ > -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ > > Note the difference in viewfinder and raw stream sizes due to the fact > that debayering requires enlarging the image width, which enforces > selecting a larger sensor resolution in this case. > > In order to track whether a raw stream is requested and which one it is, > SimpleCameraData::rawStream_ member variable is introduced. > > This is the final step to make raw streams working. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- > 1 file changed, 36 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 0c3284feb..607b07949 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -333,6 +333,7 @@ public: > }; > > std::vector<Stream> streams_; > + Stream *rawStream_; > > /* > * All entities in the pipeline, from the camera sensor to the video > @@ -371,6 +372,11 @@ private: > void ispStatsReady(uint32_t frame, uint32_t bufferId); > void metadataReady(uint32_t frame, const ControlList &metadata); > void setSensorControls(const ControlList &sensorControls); > + > + bool processedRequested() const > + { > + return streams_.size() - (rawStream_ ? 1 : 0) > 0; > + } > }; > > class SimpleCameraConfiguration : public CameraConfiguration > @@ -467,7 +473,7 @@ private: > SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, > unsigned int numStreams, > MediaEntity *sensor) > - : Camera::Private(pipe), streams_(numStreams) > + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) > { > /* > * Find the shortest path from the camera sensor to a video capture > @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > * point converting an erroneous buffer. > */ > if (buffer->metadata().status != FrameMetadata::FrameSuccess) { > - if (!useConversion_) { > + if (rawStream_) { > /* 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; > } > @@ -891,7 +900,8 @@ 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) > + if (buffer->metadata().status != FrameMetadata::FrameCancelled && > + !rawStream_) > video_->queueBuffer(buffer); > > if (conversionQueue_.empty()) > @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > */ > if (useConversion_) { > if (conversionQueue_.empty()) { > - video_->queueBuffer(buffer); > + if (!rawStream_) > + video_->queueBuffer(buffer); > return; > } > > if (converter_) > converter_->queueBuffers(buffer, conversionQueue_.front().outputs); > - else > + else if (processedRequested()) > /* > * request->sequence() cannot be retrieved from `buffer' inside > * queueBuffers because unique_ptr's make buffer->request() invalid > @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > conversionQueue_.front().outputs); > > conversionQueue_.pop(); > + if (rawStream_) > + pipe->completeBuffer(request, buffer); > return; > } > > @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) > { > /* Queue the input buffer back for capture. */ > - video_->queueBuffer(buffer); > + if (!rawStream_) > + video_->queueBuffer(buffer); > } > > void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) > @@ -1518,11 +1532,15 @@ 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); > > cfg.setStream(&data->streams_[i]); > > - if (data->useConversion_ && !isRaw(cfg)) > + if (data->useConversion_ && !rawStream) > outputCfgs.push_back(cfg); > + > + if (rawStream) > + data->rawStream_ = &data->streams_[i]; Last comment, just realised: data->rawStream_ should be initialised to nullptr (outside this loop) each time the camera is configured, otherwise it can lead to subtle bugs if the camera configure() is called multiple times with varying configurations. > } > > if (outputCfgs.empty()) > @@ -1553,7 +1571,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_) > + if (data->useConversion_ && stream != data->rawStream_) > return data->converter_ > ? data->converter_->exportBuffers(stream, count, buffers) > : data->swIsp_->exportBuffers(stream, count, buffers); > @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > return -EBUSY; > } > > - if (data->useConversion_) { > + if (data->useConversion_ && !data->rawStream_) { > /* > * When using the converter allocate a fixed number of internal > * buffers. > @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > ret = video->allocateBuffers(kNumInternalBuffers, > &data->conversionBuffers_); > } else { > - /* Otherwise, prepare for using buffers from the only stream. */ > - Stream *stream = &data->streams_[0]; > + /* > + * 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]); > ret = video->importBuffers(stream->configuration().bufferCount); > } > if (ret < 0) { > @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > } > > /* Queue all internal buffers for capture. */ > - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) > - video->queueBuffer(buffer.get()); > + if (!data->rawStream_) > + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) > + video->queueBuffer(buffer.get()); > } > > return 0; > @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > * queue, it will be handed to the converter in the capture > * completion handler. > */ > - if (data->useConversion_) { > + if (data->useConversion_ && stream != data->rawStream_) { > buffers.emplace(stream, buffer); > metadataRequired = !!data->swIsp_; > } else { > -- > 2.51.0 >
Hi Umang, Umang Jain <uajain@igalia.com> writes: > On Tue, Oct 21, 2025 at 08:27:15PM +0200, Milan Zamazal wrote: >> When a raw stream is requested, whether alone or together with a >> processed stream, its buffers must be handled outside the software ISP > >> machinery. They serve as output buffers, even when a processed stream >> is produced. >> >> At most one raw stream and at most one processed stream are supported >> and can be combined. An example of producing both raw and processed >> files using `cam' application: >> >> cam -c1 -C100 -Ffile# \ >> -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ >> -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ >> >> Note the difference in viewfinder and raw stream sizes due to the fact >> that debayering requires enlarging the image width, which enforces >> selecting a larger sensor resolution in this case. >> >> In order to track whether a raw stream is requested and which one it is, >> SimpleCameraData::rawStream_ member variable is introduced. >> >> This is the final step to make raw streams working. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- >> 1 file changed, 36 insertions(+), 14 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 0c3284feb..607b07949 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -333,6 +333,7 @@ public: >> }; >> >> std::vector<Stream> streams_; >> + Stream *rawStream_; >> >> /* >> * All entities in the pipeline, from the camera sensor to the video >> @@ -371,6 +372,11 @@ private: >> void ispStatsReady(uint32_t frame, uint32_t bufferId); >> void metadataReady(uint32_t frame, const ControlList &metadata); >> void setSensorControls(const ControlList &sensorControls); >> + >> + bool processedRequested() const >> + { >> + return streams_.size() - (rawStream_ ? 1 : 0) > 0; >> + } >> }; >> >> class SimpleCameraConfiguration : public CameraConfiguration >> @@ -467,7 +473,7 @@ private: >> SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, >> unsigned int numStreams, >> MediaEntity *sensor) >> - : Camera::Private(pipe), streams_(numStreams) >> + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) >> { >> /* >> * Find the shortest path from the camera sensor to a video capture >> @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >> * point converting an erroneous buffer. >> */ >> if (buffer->metadata().status != FrameMetadata::FrameSuccess) { >> - if (!useConversion_) { >> + if (rawStream_) { > > [1] > > I really don't understand the change here. The idea is that when a raw stream is requested then the buffers must be handled basically as without software ISP because they serve as output buffers (unlike with a pure software ISP case). > Also doesn't it break the case when only one processed stream is > requested and an error occured ? It's the same as before in such a case -- the condition is false. >> /* 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; > > If this change is trying to adapt the if condition in > tryCompleteRequest(), I believe this should be a different > patch. For now, I don't see how it relates to raw stream enabling. I don't understand what you mean exactly here. The purpose of this change is that now, when a processed stream may be available in addition to a raw stream, we must ensure that we won't hang waiting for metadata when an error occurs. >> tryCompleteRequest(request); >> return; >> } >> @@ -891,7 +900,8 @@ 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) >> + if (buffer->metadata().status != FrameMetadata::FrameCancelled && >> + !rawStream_) > > Do you need to check for !rawStream_ again? The codepath would have > returned early already if I consider the change above at [1], no? Right, I'll remove this. >> video_->queueBuffer(buffer); >> >> if (conversionQueue_.empty()) >> @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >> */ >> if (useConversion_) { >> if (conversionQueue_.empty()) { >> - video_->queueBuffer(buffer); >> + if (!rawStream_) >> + video_->queueBuffer(buffer); >> return; >> } >> >> if (converter_) >> converter_->queueBuffers(buffer, conversionQueue_.front().outputs); >> - else >> + else if (processedRequested()) > > Isn't this implicit, if useConversion_=True, you either go through > converter_ or softISP. We don't need to check for processedRequested() > here. Right, this condition is unnecessary, as well as processedRequested method then. >> /* >> * request->sequence() cannot be retrieved from `buffer' inside >> * queueBuffers because unique_ptr's make buffer->request() invalid >> @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >> conversionQueue_.front().outputs); >> >> conversionQueue_.pop(); >> + if (rawStream_) >> + pipe->completeBuffer(request, buffer); > > This shouldn't be needed either, the function has pipe->completeBuffer() > out of this if block. But this won't be called because of the return statement at the following line: >> return; >> } >> >> @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) >> void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) >> { >> /* Queue the input buffer back for capture. */ >> - video_->queueBuffer(buffer); >> + if (!rawStream_) >> + video_->queueBuffer(buffer); > > Don't you need to also complete the raw buffer in else block ? > This would be the case where only raw stream is requested. Or is it done > elsewhere? If only a raw stream is requested then buffers should be handled the same way as before this patch. This method is not called in such a case. >> } >> >> void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) >> @@ -1518,11 +1532,15 @@ 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); >> >> cfg.setStream(&data->streams_[i]); >> >> - if (data->useConversion_ && !isRaw(cfg)) >> + if (data->useConversion_ && !rawStream) >> outputCfgs.push_back(cfg); >> + >> + if (rawStream) >> + data->rawStream_ = &data->streams_[i]; >> } >> >> if (outputCfgs.empty()) >> @@ -1553,7 +1571,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_) >> + if (data->useConversion_ && stream != data->rawStream_) >> return data->converter_ >> ? data->converter_->exportBuffers(stream, count, buffers) >> : data->swIsp_->exportBuffers(stream, count, buffers); >> @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >> return -EBUSY; >> } >> >> - if (data->useConversion_) { >> + if (data->useConversion_ && !data->rawStream_) { >> /* >> * When using the converter allocate a fixed number of internal >> * buffers. >> @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >> ret = video->allocateBuffers(kNumInternalBuffers, >> &data->conversionBuffers_); >> } else { >> - /* Otherwise, prepare for using buffers from the only stream. */ >> - Stream *stream = &data->streams_[0]; >> + /* >> + * 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]); >> ret = video->importBuffers(stream->configuration().bufferCount); >> } >> if (ret < 0) { >> @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >> } >> >> /* Queue all internal buffers for capture. */ >> - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >> - video->queueBuffer(buffer.get()); >> + if (!data->rawStream_) >> + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >> + video->queueBuffer(buffer.get()); >> } >> >> return 0; >> @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) >> * queue, it will be handed to the converter in the capture >> * completion handler. >> */ >> - if (data->useConversion_) { >> + if (data->useConversion_ && stream != data->rawStream_) { >> buffers.emplace(stream, buffer); >> metadataRequired = !!data->swIsp_; >> } else { >> -- >> 2.51.0 >> Umang Jain <uajain@igalia.com> writes: > I reviewed most of the patches and am OK with them. I prefer a slight > rework in 4/8 and have highlighted it (with a reference diff I > implemented earlier). I'll change 4/8 to be close to your implementation, with some small corrections. > Unfortuantely, I did find patch 8/8 hard-to-follow :( I'm not sure what to do to make it easier, maybe the comments above help a bit? > But if it's just me, it would be better if someone else should take a > look. > > > On Tue, Oct 21, 2025 at 08:27:15PM +0200, Milan Zamazal wrote: >> When a raw stream is requested, whether alone or together with a >> processed stream, its buffers must be handled outside the software ISP >> machinery. They serve as output buffers, even when a processed stream >> is produced. >> >> At most one raw stream and at most one processed stream are supported >> and can be combined. An example of producing both raw and processed >> files using `cam' application: >> >> cam -c1 -C100 -Ffile# \ >> -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ >> -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ >> >> Note the difference in viewfinder and raw stream sizes due to the fact >> that debayering requires enlarging the image width, which enforces >> selecting a larger sensor resolution in this case. >> >> In order to track whether a raw stream is requested and which one it is, >> SimpleCameraData::rawStream_ member variable is introduced. >> >> This is the final step to make raw streams working. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- >> 1 file changed, 36 insertions(+), 14 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 0c3284feb..607b07949 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -333,6 +333,7 @@ public: >> }; >> >> std::vector<Stream> streams_; >> + Stream *rawStream_; >> >> /* >> * All entities in the pipeline, from the camera sensor to the video >> @@ -371,6 +372,11 @@ private: >> void ispStatsReady(uint32_t frame, uint32_t bufferId); >> void metadataReady(uint32_t frame, const ControlList &metadata); >> void setSensorControls(const ControlList &sensorControls); >> + >> + bool processedRequested() const >> + { >> + return streams_.size() - (rawStream_ ? 1 : 0) > 0; >> + } >> }; >> >> class SimpleCameraConfiguration : public CameraConfiguration >> @@ -467,7 +473,7 @@ private: >> SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, >> unsigned int numStreams, >> MediaEntity *sensor) >> - : Camera::Private(pipe), streams_(numStreams) >> + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) >> { >> /* >> * Find the shortest path from the camera sensor to a video capture >> @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >> * point converting an erroneous buffer. >> */ >> if (buffer->metadata().status != FrameMetadata::FrameSuccess) { >> - if (!useConversion_) { >> + if (rawStream_) { >> /* 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; >> } >> @@ -891,7 +900,8 @@ 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) >> + if (buffer->metadata().status != FrameMetadata::FrameCancelled && >> + !rawStream_) >> video_->queueBuffer(buffer); >> >> if (conversionQueue_.empty()) >> @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >> */ >> if (useConversion_) { >> if (conversionQueue_.empty()) { >> - video_->queueBuffer(buffer); >> + if (!rawStream_) >> + video_->queueBuffer(buffer); >> return; >> } >> >> if (converter_) >> converter_->queueBuffers(buffer, conversionQueue_.front().outputs); >> - else >> + else if (processedRequested()) >> /* >> * request->sequence() cannot be retrieved from `buffer' inside >> * queueBuffers because unique_ptr's make buffer->request() invalid >> @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >> conversionQueue_.front().outputs); >> >> conversionQueue_.pop(); >> + if (rawStream_) >> + pipe->completeBuffer(request, buffer); >> return; >> } >> >> @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) >> void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) >> { >> /* Queue the input buffer back for capture. */ >> - video_->queueBuffer(buffer); >> + if (!rawStream_) >> + video_->queueBuffer(buffer); >> } >> >> void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) >> @@ -1518,11 +1532,15 @@ 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); >> >> cfg.setStream(&data->streams_[i]); >> >> - if (data->useConversion_ && !isRaw(cfg)) >> + if (data->useConversion_ && !rawStream) >> outputCfgs.push_back(cfg); >> + >> + if (rawStream) >> + data->rawStream_ = &data->streams_[i]; >> } >> >> if (outputCfgs.empty()) >> @@ -1553,7 +1571,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_) >> + if (data->useConversion_ && stream != data->rawStream_) >> return data->converter_ >> ? data->converter_->exportBuffers(stream, count, buffers) >> : data->swIsp_->exportBuffers(stream, count, buffers); >> @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >> return -EBUSY; >> } >> >> - if (data->useConversion_) { >> + if (data->useConversion_ && !data->rawStream_) { >> /* >> * When using the converter allocate a fixed number of internal >> * buffers. >> @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >> ret = video->allocateBuffers(kNumInternalBuffers, >> &data->conversionBuffers_); >> } else { >> - /* Otherwise, prepare for using buffers from the only stream. */ >> - Stream *stream = &data->streams_[0]; >> + /* >> + * 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]); >> ret = video->importBuffers(stream->configuration().bufferCount); >> } >> if (ret < 0) { >> @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >> } >> >> /* Queue all internal buffers for capture. */ >> - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >> - video->queueBuffer(buffer.get()); >> + if (!data->rawStream_) >> + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >> + video->queueBuffer(buffer.get()); >> } >> >> return 0; >> @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) >> * queue, it will be handed to the converter in the capture >> * completion handler. >> */ >> - if (data->useConversion_) { >> + if (data->useConversion_ && stream != data->rawStream_) { >> buffers.emplace(stream, buffer); >> metadataRequired = !!data->swIsp_; >> } else { >> -- >> 2.51.0 >> Umang Jain <uajain@igalia.com> writes: > On Tue, Oct 21, 2025 at 08:27:15PM +0200, Milan Zamazal wrote: >> When a raw stream is requested, whether alone or together with a >> processed stream, its buffers must be handled outside the software ISP > >> machinery. They serve as output buffers, even when a processed stream >> is produced. >> >> At most one raw stream and at most one processed stream are supported >> and can be combined. An example of producing both raw and processed >> files using `cam' application: >> >> cam -c1 -C100 -Ffile# \ >> -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ >> -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ >> >> Note the difference in viewfinder and raw stream sizes due to the fact >> that debayering requires enlarging the image width, which enforces >> selecting a larger sensor resolution in this case. >> >> In order to track whether a raw stream is requested and which one it is, >> SimpleCameraData::rawStream_ member variable is introduced. >> >> This is the final step to make raw streams working. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- >> 1 file changed, 36 insertions(+), 14 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 0c3284feb..607b07949 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -333,6 +333,7 @@ public: >> }; >> >> std::vector<Stream> streams_; >> + Stream *rawStream_; >> >> /* >> * All entities in the pipeline, from the camera sensor to the video >> @@ -371,6 +372,11 @@ private: >> void ispStatsReady(uint32_t frame, uint32_t bufferId); >> void metadataReady(uint32_t frame, const ControlList &metadata); >> void setSensorControls(const ControlList &sensorControls); >> + >> + bool processedRequested() const >> + { >> + return streams_.size() - (rawStream_ ? 1 : 0) > 0; >> + } >> }; >> >> class SimpleCameraConfiguration : public CameraConfiguration >> @@ -467,7 +473,7 @@ private: >> SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, >> unsigned int numStreams, >> MediaEntity *sensor) >> - : Camera::Private(pipe), streams_(numStreams) >> + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) >> { >> /* >> * Find the shortest path from the camera sensor to a video capture >> @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >> * point converting an erroneous buffer. >> */ >> if (buffer->metadata().status != FrameMetadata::FrameSuccess) { >> - if (!useConversion_) { >> + if (rawStream_) { >> /* 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; >> } >> @@ -891,7 +900,8 @@ 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) >> + if (buffer->metadata().status != FrameMetadata::FrameCancelled && >> + !rawStream_) >> video_->queueBuffer(buffer); >> >> if (conversionQueue_.empty()) >> @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >> */ >> if (useConversion_) { >> if (conversionQueue_.empty()) { >> - video_->queueBuffer(buffer); >> + if (!rawStream_) >> + video_->queueBuffer(buffer); >> return; >> } >> >> if (converter_) >> converter_->queueBuffers(buffer, conversionQueue_.front().outputs); >> - else >> + else if (processedRequested()) >> /* >> * request->sequence() cannot be retrieved from `buffer' inside >> * queueBuffers because unique_ptr's make buffer->request() invalid >> @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >> conversionQueue_.front().outputs); >> >> conversionQueue_.pop(); >> + if (rawStream_) >> + pipe->completeBuffer(request, buffer); >> return; >> } >> >> @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) >> void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) >> { >> /* Queue the input buffer back for capture. */ >> - video_->queueBuffer(buffer); >> + if (!rawStream_) >> + video_->queueBuffer(buffer); >> } >> >> void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) >> @@ -1518,11 +1532,15 @@ 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); >> >> cfg.setStream(&data->streams_[i]); >> >> - if (data->useConversion_ && !isRaw(cfg)) >> + if (data->useConversion_ && !rawStream) >> outputCfgs.push_back(cfg); >> + >> + if (rawStream) >> + data->rawStream_ = &data->streams_[i]; > > Last comment, just realised: > > data->rawStream_ should be initialised to nullptr (outside this loop) > each time the camera is configured, otherwise it can lead to subtle bugs > if the camera configure() is called multiple times with varying > configurations. Right, I'll add it. >> } >> >> if (outputCfgs.empty()) >> @@ -1553,7 +1571,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_) >> + if (data->useConversion_ && stream != data->rawStream_) >> return data->converter_ >> ? data->converter_->exportBuffers(stream, count, buffers) >> : data->swIsp_->exportBuffers(stream, count, buffers); >> @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >> return -EBUSY; >> } >> >> - if (data->useConversion_) { >> + if (data->useConversion_ && !data->rawStream_) { >> /* >> * When using the converter allocate a fixed number of internal >> * buffers. >> @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >> ret = video->allocateBuffers(kNumInternalBuffers, >> &data->conversionBuffers_); >> } else { >> - /* Otherwise, prepare for using buffers from the only stream. */ >> - Stream *stream = &data->streams_[0]; >> + /* >> + * 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]); >> ret = video->importBuffers(stream->configuration().bufferCount); >> } >> if (ret < 0) { >> @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >> } >> >> /* Queue all internal buffers for capture. */ >> - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >> - video->queueBuffer(buffer.get()); >> + if (!data->rawStream_) >> + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >> + video->queueBuffer(buffer.get()); >> } >> >> return 0; >> @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) >> * queue, it will be handed to the converter in the capture >> * completion handler. >> */ >> - if (data->useConversion_) { >> + if (data->useConversion_ && stream != data->rawStream_) { >> buffers.emplace(stream, buffer); >> metadataRequired = !!data->swIsp_; >> } else { >> -- >> 2.51.0 >>
On 2025-11-01 19:29, Milan Zamazal wrote: > Hi Umang, > > Umang Jain <uajain@igalia.com> writes: > >> On Tue, Oct 21, 2025 at 08:27:15PM +0200, Milan Zamazal wrote: >>> When a raw stream is requested, whether alone or together with a >>> processed stream, its buffers must be handled outside the software ISP >> >>> machinery. They serve as output buffers, even when a processed stream >>> is produced. >>> >>> At most one raw stream and at most one processed stream are supported >>> and can be combined. An example of producing both raw and processed >>> files using `cam' application: >>> >>> cam -c1 -C100 -Ffile# \ >>> -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ >>> -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ >>> >>> Note the difference in viewfinder and raw stream sizes due to the fact >>> that debayering requires enlarging the image width, which enforces >>> selecting a larger sensor resolution in this case. >>> >>> In order to track whether a raw stream is requested and which one it is, >>> SimpleCameraData::rawStream_ member variable is introduced. >>> >>> This is the final step to make raw streams working. >>> >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- >>> 1 file changed, 36 insertions(+), 14 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >>> index 0c3284feb..607b07949 100644 >>> --- a/src/libcamera/pipeline/simple/simple.cpp >>> +++ b/src/libcamera/pipeline/simple/simple.cpp >>> @@ -333,6 +333,7 @@ public: >>> }; >>> >>> std::vector<Stream> streams_; >>> + Stream *rawStream_; >>> >>> /* >>> * All entities in the pipeline, from the camera sensor to the video >>> @@ -371,6 +372,11 @@ private: >>> void ispStatsReady(uint32_t frame, uint32_t bufferId); >>> void metadataReady(uint32_t frame, const ControlList &metadata); >>> void setSensorControls(const ControlList &sensorControls); >>> + >>> + bool processedRequested() const >>> + { >>> + return streams_.size() - (rawStream_ ? 1 : 0) > 0; >>> + } >>> }; >>> >>> class SimpleCameraConfiguration : public CameraConfiguration >>> @@ -467,7 +473,7 @@ private: >>> SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, >>> unsigned int numStreams, >>> MediaEntity *sensor) >>> - : Camera::Private(pipe), streams_(numStreams) >>> + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) >>> { >>> /* >>> * Find the shortest path from the camera sensor to a video capture >>> @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>> * point converting an erroneous buffer. >>> */ >>> if (buffer->metadata().status != FrameMetadata::FrameSuccess) { >>> - if (!useConversion_) { >>> + if (rawStream_) { >> >> [1] >> >> I really don't understand the change here. > > The idea is that when a raw stream is requested then the buffers must be > handled basically as without software ISP because they serve as output > buffers (unlike with a pure software ISP case). > >> Also doesn't it break the case when only one processed stream is >> requested and an error occured ? > > It's the same as before in such a case -- the condition is false. How come? In case of only processed request = 1, useConversion_ (same value of needConversion_) = false as needConversion_ = config_.size() > 1 so if (!useConversion_) is, if (true) whereas with your change in case of processed request = 1, if (rawStream_) is, if (nullptr) = if (false) The condition is not same as before. > >>> /* 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; >> >> If this change is trying to adapt the if condition in >> tryCompleteRequest(), I believe this should be a different >> patch. For now, I don't see how it relates to raw stream enabling. > > I don't understand what you mean exactly here. The purpose of this > change is that now, when a processed stream may be available in addition > to a raw stream, we must ensure that we won't hang waiting for metadata > when an error occurs. > >>> tryCompleteRequest(request); >>> return; >>> } >>> @@ -891,7 +900,8 @@ 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) >>> + if (buffer->metadata().status != FrameMetadata::FrameCancelled && >>> + !rawStream_) >> >> Do you need to check for !rawStream_ again? The codepath would have >> returned early already if I consider the change above at [1], no? > > Right, I'll remove this. > >>> video_->queueBuffer(buffer); >>> >>> if (conversionQueue_.empty()) >>> @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>> */ >>> if (useConversion_) { >>> if (conversionQueue_.empty()) { >>> - video_->queueBuffer(buffer); >>> + if (!rawStream_) >>> + video_->queueBuffer(buffer); >>> return; >>> } >>> >>> if (converter_) >>> converter_->queueBuffers(buffer, conversionQueue_.front().outputs); >>> - else >>> + else if (processedRequested()) >> >> Isn't this implicit, if useConversion_=True, you either go through >> converter_ or softISP. We don't need to check for processedRequested() >> here. > > Right, this condition is unnecessary, as well as processedRequested > method then. > >>> /* >>> * request->sequence() cannot be retrieved from `buffer' inside >>> * queueBuffers because unique_ptr's make buffer->request() invalid >>> @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>> conversionQueue_.front().outputs); >>> >>> conversionQueue_.pop(); >>> + if (rawStream_) >>> + pipe->completeBuffer(request, buffer); >> >> This shouldn't be needed either, the function has pipe->completeBuffer() >> out of this if block. > > But this won't be called because of the return statement at the > following line: > >>> return; >>> } >>> >>> @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) >>> void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) >>> { >>> /* Queue the input buffer back for capture. */ >>> - video_->queueBuffer(buffer); >>> + if (!rawStream_) >>> + video_->queueBuffer(buffer); >> >> Don't you need to also complete the raw buffer in else block ? >> This would be the case where only raw stream is requested. Or is it done >> elsewhere? > > If only a raw stream is requested then buffers should be handled the > same way as before this patch. This method is not called in such a > case. > >>> } >>> >>> void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) >>> @@ -1518,11 +1532,15 @@ 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); >>> >>> cfg.setStream(&data->streams_[i]); >>> >>> - if (data->useConversion_ && !isRaw(cfg)) >>> + if (data->useConversion_ && !rawStream) >>> outputCfgs.push_back(cfg); >>> + >>> + if (rawStream) >>> + data->rawStream_ = &data->streams_[i]; >>> } >>> >>> if (outputCfgs.empty()) >>> @@ -1553,7 +1571,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_) >>> + if (data->useConversion_ && stream != data->rawStream_) >>> return data->converter_ >>> ? data->converter_->exportBuffers(stream, count, buffers) >>> : data->swIsp_->exportBuffers(stream, count, buffers); >>> @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>> return -EBUSY; >>> } >>> >>> - if (data->useConversion_) { >>> + if (data->useConversion_ && !data->rawStream_) { >>> /* >>> * When using the converter allocate a fixed number of internal >>> * buffers. >>> @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>> ret = video->allocateBuffers(kNumInternalBuffers, >>> &data->conversionBuffers_); >>> } else { >>> - /* Otherwise, prepare for using buffers from the only stream. */ >>> - Stream *stream = &data->streams_[0]; >>> + /* >>> + * 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]); >>> ret = video->importBuffers(stream->configuration().bufferCount); >>> } >>> if (ret < 0) { >>> @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>> } >>> >>> /* Queue all internal buffers for capture. */ >>> - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >>> - video->queueBuffer(buffer.get()); >>> + if (!data->rawStream_) >>> + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >>> + video->queueBuffer(buffer.get()); >>> } >>> >>> return 0; >>> @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) >>> * queue, it will be handed to the converter in the capture >>> * completion handler. >>> */ >>> - if (data->useConversion_) { >>> + if (data->useConversion_ && stream != data->rawStream_) { >>> buffers.emplace(stream, buffer); >>> metadataRequired = !!data->swIsp_; >>> } else { >>> -- >>> 2.51.0 >>> > > Umang Jain <uajain@igalia.com> writes: > >> I reviewed most of the patches and am OK with them. I prefer a slight >> rework in 4/8 and have highlighted it (with a reference diff I >> implemented earlier). > > I'll change 4/8 to be close to your implementation, with some small > corrections. > >> Unfortuantely, I did find patch 8/8 hard-to-follow :( > > I'm not sure what to do to make it easier, maybe the comments above help > a bit? > >> But if it's just me, it would be better if someone else should take a >> look. >> >> >> On Tue, Oct 21, 2025 at 08:27:15PM +0200, Milan Zamazal wrote: >>> When a raw stream is requested, whether alone or together with a >>> processed stream, its buffers must be handled outside the software ISP >>> machinery. They serve as output buffers, even when a processed stream >>> is produced. >>> >>> At most one raw stream and at most one processed stream are supported >>> and can be combined. An example of producing both raw and processed >>> files using `cam' application: >>> >>> cam -c1 -C100 -Ffile# \ >>> -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ >>> -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ >>> >>> Note the difference in viewfinder and raw stream sizes due to the fact >>> that debayering requires enlarging the image width, which enforces >>> selecting a larger sensor resolution in this case. >>> >>> In order to track whether a raw stream is requested and which one it is, >>> SimpleCameraData::rawStream_ member variable is introduced. >>> >>> This is the final step to make raw streams working. >>> >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- >>> 1 file changed, 36 insertions(+), 14 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >>> index 0c3284feb..607b07949 100644 >>> --- a/src/libcamera/pipeline/simple/simple.cpp >>> +++ b/src/libcamera/pipeline/simple/simple.cpp >>> @@ -333,6 +333,7 @@ public: >>> }; >>> >>> std::vector<Stream> streams_; >>> + Stream *rawStream_; >>> >>> /* >>> * All entities in the pipeline, from the camera sensor to the video >>> @@ -371,6 +372,11 @@ private: >>> void ispStatsReady(uint32_t frame, uint32_t bufferId); >>> void metadataReady(uint32_t frame, const ControlList &metadata); >>> void setSensorControls(const ControlList &sensorControls); >>> + >>> + bool processedRequested() const >>> + { >>> + return streams_.size() - (rawStream_ ? 1 : 0) > 0; >>> + } >>> }; >>> >>> class SimpleCameraConfiguration : public CameraConfiguration >>> @@ -467,7 +473,7 @@ private: >>> SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, >>> unsigned int numStreams, >>> MediaEntity *sensor) >>> - : Camera::Private(pipe), streams_(numStreams) >>> + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) >>> { >>> /* >>> * Find the shortest path from the camera sensor to a video capture >>> @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>> * point converting an erroneous buffer. >>> */ >>> if (buffer->metadata().status != FrameMetadata::FrameSuccess) { >>> - if (!useConversion_) { >>> + if (rawStream_) { >>> /* 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; >>> } >>> @@ -891,7 +900,8 @@ 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) >>> + if (buffer->metadata().status != FrameMetadata::FrameCancelled && >>> + !rawStream_) >>> video_->queueBuffer(buffer); >>> >>> if (conversionQueue_.empty()) >>> @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>> */ >>> if (useConversion_) { >>> if (conversionQueue_.empty()) { >>> - video_->queueBuffer(buffer); >>> + if (!rawStream_) >>> + video_->queueBuffer(buffer); >>> return; >>> } >>> >>> if (converter_) >>> converter_->queueBuffers(buffer, conversionQueue_.front().outputs); >>> - else >>> + else if (processedRequested()) >>> /* >>> * request->sequence() cannot be retrieved from `buffer' inside >>> * queueBuffers because unique_ptr's make buffer->request() invalid >>> @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>> conversionQueue_.front().outputs); >>> >>> conversionQueue_.pop(); >>> + if (rawStream_) >>> + pipe->completeBuffer(request, buffer); >>> return; >>> } >>> >>> @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) >>> void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) >>> { >>> /* Queue the input buffer back for capture. */ >>> - video_->queueBuffer(buffer); >>> + if (!rawStream_) >>> + video_->queueBuffer(buffer); >>> } >>> >>> void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) >>> @@ -1518,11 +1532,15 @@ 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); >>> >>> cfg.setStream(&data->streams_[i]); >>> >>> - if (data->useConversion_ && !isRaw(cfg)) >>> + if (data->useConversion_ && !rawStream) >>> outputCfgs.push_back(cfg); >>> + >>> + if (rawStream) >>> + data->rawStream_ = &data->streams_[i]; >>> } >>> >>> if (outputCfgs.empty()) >>> @@ -1553,7 +1571,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_) >>> + if (data->useConversion_ && stream != data->rawStream_) >>> return data->converter_ >>> ? data->converter_->exportBuffers(stream, count, buffers) >>> : data->swIsp_->exportBuffers(stream, count, buffers); >>> @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>> return -EBUSY; >>> } >>> >>> - if (data->useConversion_) { >>> + if (data->useConversion_ && !data->rawStream_) { >>> /* >>> * When using the converter allocate a fixed number of internal >>> * buffers. >>> @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>> ret = video->allocateBuffers(kNumInternalBuffers, >>> &data->conversionBuffers_); >>> } else { >>> - /* Otherwise, prepare for using buffers from the only stream. */ >>> - Stream *stream = &data->streams_[0]; >>> + /* >>> + * 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]); >>> ret = video->importBuffers(stream->configuration().bufferCount); >>> } >>> if (ret < 0) { >>> @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>> } >>> >>> /* Queue all internal buffers for capture. */ >>> - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >>> - video->queueBuffer(buffer.get()); >>> + if (!data->rawStream_) >>> + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >>> + video->queueBuffer(buffer.get()); >>> } >>> >>> return 0; >>> @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) >>> * queue, it will be handed to the converter in the capture >>> * completion handler. >>> */ >>> - if (data->useConversion_) { >>> + if (data->useConversion_ && stream != data->rawStream_) { >>> buffers.emplace(stream, buffer); >>> metadataRequired = !!data->swIsp_; >>> } else { >>> -- >>> 2.51.0 >>> > > Umang Jain <uajain@igalia.com> writes: > >> On Tue, Oct 21, 2025 at 08:27:15PM +0200, Milan Zamazal wrote: >>> When a raw stream is requested, whether alone or together with a >>> processed stream, its buffers must be handled outside the software ISP >> >>> machinery. They serve as output buffers, even when a processed stream >>> is produced. >>> >>> At most one raw stream and at most one processed stream are supported >>> and can be combined. An example of producing both raw and processed >>> files using `cam' application: >>> >>> cam -c1 -C100 -Ffile# \ >>> -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ >>> -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ >>> >>> Note the difference in viewfinder and raw stream sizes due to the fact >>> that debayering requires enlarging the image width, which enforces >>> selecting a larger sensor resolution in this case. >>> >>> In order to track whether a raw stream is requested and which one it is, >>> SimpleCameraData::rawStream_ member variable is introduced. >>> >>> This is the final step to make raw streams working. >>> >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- >>> 1 file changed, 36 insertions(+), 14 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >>> index 0c3284feb..607b07949 100644 >>> --- a/src/libcamera/pipeline/simple/simple.cpp >>> +++ b/src/libcamera/pipeline/simple/simple.cpp >>> @@ -333,6 +333,7 @@ public: >>> }; >>> >>> std::vector<Stream> streams_; >>> + Stream *rawStream_; >>> >>> /* >>> * All entities in the pipeline, from the camera sensor to the video >>> @@ -371,6 +372,11 @@ private: >>> void ispStatsReady(uint32_t frame, uint32_t bufferId); >>> void metadataReady(uint32_t frame, const ControlList &metadata); >>> void setSensorControls(const ControlList &sensorControls); >>> + >>> + bool processedRequested() const >>> + { >>> + return streams_.size() - (rawStream_ ? 1 : 0) > 0; >>> + } >>> }; >>> >>> class SimpleCameraConfiguration : public CameraConfiguration >>> @@ -467,7 +473,7 @@ private: >>> SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, >>> unsigned int numStreams, >>> MediaEntity *sensor) >>> - : Camera::Private(pipe), streams_(numStreams) >>> + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) >>> { >>> /* >>> * Find the shortest path from the camera sensor to a video capture >>> @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>> * point converting an erroneous buffer. >>> */ >>> if (buffer->metadata().status != FrameMetadata::FrameSuccess) { >>> - if (!useConversion_) { >>> + if (rawStream_) { >>> /* 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; >>> } >>> @@ -891,7 +900,8 @@ 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) >>> + if (buffer->metadata().status != FrameMetadata::FrameCancelled && >>> + !rawStream_) >>> video_->queueBuffer(buffer); >>> >>> if (conversionQueue_.empty()) >>> @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>> */ >>> if (useConversion_) { >>> if (conversionQueue_.empty()) { >>> - video_->queueBuffer(buffer); >>> + if (!rawStream_) >>> + video_->queueBuffer(buffer); >>> return; >>> } >>> >>> if (converter_) >>> converter_->queueBuffers(buffer, conversionQueue_.front().outputs); >>> - else >>> + else if (processedRequested()) >>> /* >>> * request->sequence() cannot be retrieved from `buffer' inside >>> * queueBuffers because unique_ptr's make buffer->request() invalid >>> @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>> conversionQueue_.front().outputs); >>> >>> conversionQueue_.pop(); >>> + if (rawStream_) >>> + pipe->completeBuffer(request, buffer); >>> return; >>> } >>> >>> @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) >>> void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) >>> { >>> /* Queue the input buffer back for capture. */ >>> - video_->queueBuffer(buffer); >>> + if (!rawStream_) >>> + video_->queueBuffer(buffer); >>> } >>> >>> void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) >>> @@ -1518,11 +1532,15 @@ 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); >>> >>> cfg.setStream(&data->streams_[i]); >>> >>> - if (data->useConversion_ && !isRaw(cfg)) >>> + if (data->useConversion_ && !rawStream) >>> outputCfgs.push_back(cfg); >>> + >>> + if (rawStream) >>> + data->rawStream_ = &data->streams_[i]; >> >> Last comment, just realised: >> >> data->rawStream_ should be initialised to nullptr (outside this loop) >> each time the camera is configured, otherwise it can lead to subtle bugs >> if the camera configure() is called multiple times with varying >> configurations. > > Right, I'll add it. > >>> } >>> >>> if (outputCfgs.empty()) >>> @@ -1553,7 +1571,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_) >>> + if (data->useConversion_ && stream != data->rawStream_) >>> return data->converter_ >>> ? data->converter_->exportBuffers(stream, count, buffers) >>> : data->swIsp_->exportBuffers(stream, count, buffers); >>> @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>> return -EBUSY; >>> } >>> >>> - if (data->useConversion_) { >>> + if (data->useConversion_ && !data->rawStream_) { >>> /* >>> * When using the converter allocate a fixed number of internal >>> * buffers. >>> @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>> ret = video->allocateBuffers(kNumInternalBuffers, >>> &data->conversionBuffers_); >>> } else { >>> - /* Otherwise, prepare for using buffers from the only stream. */ >>> - Stream *stream = &data->streams_[0]; >>> + /* >>> + * 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]); >>> ret = video->importBuffers(stream->configuration().bufferCount); >>> } >>> if (ret < 0) { >>> @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>> } >>> >>> /* Queue all internal buffers for capture. */ >>> - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >>> - video->queueBuffer(buffer.get()); >>> + if (!data->rawStream_) >>> + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >>> + video->queueBuffer(buffer.get()); >>> } >>> >>> return 0; >>> @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) >>> * queue, it will be handed to the converter in the capture >>> * completion handler. >>> */ >>> - if (data->useConversion_) { >>> + if (data->useConversion_ && stream != data->rawStream_) { >>> buffers.emplace(stream, buffer); >>> metadataRequired = !!data->swIsp_; >>> } else { >>> -- >>> 2.51.0 >>>
uajain <uajain@igalia.com> writes: > On 2025-11-01 19:29, Milan Zamazal wrote: >> Hi Umang, >> > >> Umang Jain <uajain@igalia.com> writes: >> >>> On Tue, Oct 21, 2025 at 08:27:15PM +0200, Milan Zamazal wrote: >>>> When a raw stream is requested, whether alone or together with a >>>> processed stream, its buffers must be handled outside the software ISP >>> >>>> machinery. They serve as output buffers, even when a processed stream >>>> is produced. >>>> >>>> At most one raw stream and at most one processed stream are supported >>>> and can be combined. An example of producing both raw and processed >>>> files using `cam' application: >>>> >>>> cam -c1 -C100 -Ffile# \ >>>> -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ >>>> -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ >>>> >>>> Note the difference in viewfinder and raw stream sizes due to the fact >>>> that debayering requires enlarging the image width, which enforces >>>> selecting a larger sensor resolution in this case. >>>> >>>> In order to track whether a raw stream is requested and which one it is, >>>> SimpleCameraData::rawStream_ member variable is introduced. >>>> >>>> This is the final step to make raw streams working. >>>> >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- >>>> 1 file changed, 36 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >>>> index 0c3284feb..607b07949 100644 >>>> --- a/src/libcamera/pipeline/simple/simple.cpp >>>> +++ b/src/libcamera/pipeline/simple/simple.cpp >>>> @@ -333,6 +333,7 @@ public: >>>> }; >>>> >>>> std::vector<Stream> streams_; >>>> + Stream *rawStream_; >>>> >>>> /* >>>> * All entities in the pipeline, from the camera sensor to the video >>>> @@ -371,6 +372,11 @@ private: >>>> void ispStatsReady(uint32_t frame, uint32_t bufferId); >>>> void metadataReady(uint32_t frame, const ControlList &metadata); >>>> void setSensorControls(const ControlList &sensorControls); >>>> + >>>> + bool processedRequested() const >>>> + { >>>> + return streams_.size() - (rawStream_ ? 1 : 0) > 0; >>>> + } >>>> }; >>>> >>>> class SimpleCameraConfiguration : public CameraConfiguration >>>> @@ -467,7 +473,7 @@ private: >>>> SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, >>>> unsigned int numStreams, >>>> MediaEntity *sensor) >>>> - : Camera::Private(pipe), streams_(numStreams) >>>> + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) >>>> { >>>> /* >>>> * Find the shortest path from the camera sensor to a video capture >>>> @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>>> * point converting an erroneous buffer. >>>> */ >>>> if (buffer->metadata().status != FrameMetadata::FrameSuccess) { >>>> - if (!useConversion_) { >>>> + if (rawStream_) { >>> >>> [1] >>> >>> I really don't understand the change here. >> >> The idea is that when a raw stream is requested then the buffers must be >> handled basically as without software ISP because they serve as output >> buffers (unlike with a pure software ISP case). >> >>> Also doesn't it break the case when only one processed stream is >>> requested and an error occured ? >> >> It's the same as before in such a case -- the condition is false. > > How come? In case of only processed request = 1, > > useConversion_ (same value of needConversion_) = false as > > needConversion_ = config_.size() > 1 > > so > if (!useConversion_) is, if (true) > > whereas with your change in case of processed request = 1, > > if (rawStream_) is, if (nullptr) = if (false) > > The condition is not same as before. You're right, sorry for the confusion. I'll fix the condition in v15. I think I got confused by the fact that with software ISP needConversion_ is true: if (cfg.pixelFormat != pipeConfig_->captureFormat || cfg.size != pipeConfig_->captureSize) needConversion_ = true; But it doesn't necessarily mean that all non-raw (in the sense of isRaw()) streams need conversion so the condition should still check for useConversion. A similar problem may also be present elsewhere. >>>> /* 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; >>> >>> If this change is trying to adapt the if condition in >>> tryCompleteRequest(), I believe this should be a different >>> patch. For now, I don't see how it relates to raw stream enabling. >> >> I don't understand what you mean exactly here. The purpose of this >> change is that now, when a processed stream may be available in addition >> to a raw stream, we must ensure that we won't hang waiting for metadata >> when an error occurs. >> >>>> tryCompleteRequest(request); >>>> return; >>>> } >>>> @@ -891,7 +900,8 @@ 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) >>>> + if (buffer->metadata().status != FrameMetadata::FrameCancelled && >>>> + !rawStream_) >>> >>> Do you need to check for !rawStream_ again? The codepath would have >>> returned early already if I consider the change above at [1], no? >> >> Right, I'll remove this. >> >>>> video_->queueBuffer(buffer); >>>> >>>> if (conversionQueue_.empty()) >>>> @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>>> */ >>>> if (useConversion_) { >>>> if (conversionQueue_.empty()) { >>>> - video_->queueBuffer(buffer); >>>> + if (!rawStream_) >>>> + video_->queueBuffer(buffer); >>>> return; >>>> } >>>> >>>> if (converter_) >>>> converter_->queueBuffers(buffer, conversionQueue_.front().outputs); >>>> - else >>>> + else if (processedRequested()) >>> >>> Isn't this implicit, if useConversion_=True, you either go through >>> converter_ or softISP. We don't need to check for processedRequested() >>> here. >> >> Right, this condition is unnecessary, as well as processedRequested >> method then. >> >>>> /* >>>> * request->sequence() cannot be retrieved from `buffer' inside >>>> * queueBuffers because unique_ptr's make buffer->request() invalid >>>> @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>>> conversionQueue_.front().outputs); >>>> >>>> conversionQueue_.pop(); >>>> + if (rawStream_) >>>> + pipe->completeBuffer(request, buffer); >>> >>> This shouldn't be needed either, the function has pipe->completeBuffer() >>> out of this if block. >> >> But this won't be called because of the return statement at the >> following line: >> >>>> return; >>>> } >>>> >>>> @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) >>>> void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) >>>> { >>>> /* Queue the input buffer back for capture. */ >>>> - video_->queueBuffer(buffer); >>>> + if (!rawStream_) >>>> + video_->queueBuffer(buffer); >>> >>> Don't you need to also complete the raw buffer in else block ? >>> This would be the case where only raw stream is requested. Or is it done >>> elsewhere? >> >> If only a raw stream is requested then buffers should be handled the >> same way as before this patch. This method is not called in such a >> case. >> >>>> } >>>> >>>> void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) >>>> @@ -1518,11 +1532,15 @@ 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); >>>> >>>> cfg.setStream(&data->streams_[i]); >>>> >>>> - if (data->useConversion_ && !isRaw(cfg)) >>>> + if (data->useConversion_ && !rawStream) >>>> outputCfgs.push_back(cfg); >>>> + >>>> + if (rawStream) >>>> + data->rawStream_ = &data->streams_[i]; >>>> } >>>> >>>> if (outputCfgs.empty()) >>>> @@ -1553,7 +1571,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_) >>>> + if (data->useConversion_ && stream != data->rawStream_) >>>> return data->converter_ >>>> ? data->converter_->exportBuffers(stream, count, buffers) >>>> : data->swIsp_->exportBuffers(stream, count, buffers); >>>> @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>>> return -EBUSY; >>>> } >>>> >>>> - if (data->useConversion_) { >>>> + if (data->useConversion_ && !data->rawStream_) { >>>> /* >>>> * When using the converter allocate a fixed number of internal >>>> * buffers. >>>> @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>>> ret = video->allocateBuffers(kNumInternalBuffers, >>>> &data->conversionBuffers_); >>>> } else { >>>> - /* Otherwise, prepare for using buffers from the only stream. */ >>>> - Stream *stream = &data->streams_[0]; >>>> + /* >>>> + * 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]); >>>> ret = video->importBuffers(stream->configuration().bufferCount); >>>> } >>>> if (ret < 0) { >>>> @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>>> } >>>> >>>> /* Queue all internal buffers for capture. */ >>>> - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >>>> - video->queueBuffer(buffer.get()); >>>> + if (!data->rawStream_) >>>> + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >>>> + video->queueBuffer(buffer.get()); >>>> } >>>> >>>> return 0; >>>> @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) >>>> * queue, it will be handed to the converter in the capture >>>> * completion handler. >>>> */ >>>> - if (data->useConversion_) { >>>> + if (data->useConversion_ && stream != data->rawStream_) { >>>> buffers.emplace(stream, buffer); >>>> metadataRequired = !!data->swIsp_; >>>> } else { >>>> -- >>>> 2.51.0 >>>> >> >> Umang Jain <uajain@igalia.com> writes: >> >>> I reviewed most of the patches and am OK with them. I prefer a slight >>> rework in 4/8 and have highlighted it (with a reference diff I >>> implemented earlier). >> >> I'll change 4/8 to be close to your implementation, with some small >> corrections. >> >>> Unfortuantely, I did find patch 8/8 hard-to-follow :( >> >> I'm not sure what to do to make it easier, maybe the comments above help >> a bit? >> >>> But if it's just me, it would be better if someone else should take a >>> look. >>> >>> >>> On Tue, Oct 21, 2025 at 08:27:15PM +0200, Milan Zamazal wrote: >>>> When a raw stream is requested, whether alone or together with a >>>> processed stream, its buffers must be handled outside the software ISP >>>> machinery. They serve as output buffers, even when a processed stream >>>> is produced. >>>> >>>> At most one raw stream and at most one processed stream are supported >>>> and can be combined. An example of producing both raw and processed >>>> files using `cam' application: >>>> >>>> cam -c1 -C100 -Ffile# \ >>>> -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ >>>> -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ >>>> >>>> Note the difference in viewfinder and raw stream sizes due to the fact >>>> that debayering requires enlarging the image width, which enforces >>>> selecting a larger sensor resolution in this case. >>>> >>>> In order to track whether a raw stream is requested and which one it is, >>>> SimpleCameraData::rawStream_ member variable is introduced. >>>> >>>> This is the final step to make raw streams working. >>>> >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- >>>> 1 file changed, 36 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >>>> index 0c3284feb..607b07949 100644 >>>> --- a/src/libcamera/pipeline/simple/simple.cpp >>>> +++ b/src/libcamera/pipeline/simple/simple.cpp >>>> @@ -333,6 +333,7 @@ public: >>>> }; >>>> >>>> std::vector<Stream> streams_; >>>> + Stream *rawStream_; >>>> >>>> /* >>>> * All entities in the pipeline, from the camera sensor to the video >>>> @@ -371,6 +372,11 @@ private: >>>> void ispStatsReady(uint32_t frame, uint32_t bufferId); >>>> void metadataReady(uint32_t frame, const ControlList &metadata); >>>> void setSensorControls(const ControlList &sensorControls); >>>> + >>>> + bool processedRequested() const >>>> + { >>>> + return streams_.size() - (rawStream_ ? 1 : 0) > 0; >>>> + } >>>> }; >>>> >>>> class SimpleCameraConfiguration : public CameraConfiguration >>>> @@ -467,7 +473,7 @@ private: >>>> SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, >>>> unsigned int numStreams, >>>> MediaEntity *sensor) >>>> - : Camera::Private(pipe), streams_(numStreams) >>>> + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) >>>> { >>>> /* >>>> * Find the shortest path from the camera sensor to a video capture >>>> @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>>> * point converting an erroneous buffer. >>>> */ >>>> if (buffer->metadata().status != FrameMetadata::FrameSuccess) { >>>> - if (!useConversion_) { >>>> + if (rawStream_) { >>>> /* 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; >>>> } >>>> @@ -891,7 +900,8 @@ 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) >>>> + if (buffer->metadata().status != FrameMetadata::FrameCancelled && >>>> + !rawStream_) >>>> video_->queueBuffer(buffer); >>>> >>>> if (conversionQueue_.empty()) >>>> @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>>> */ >>>> if (useConversion_) { >>>> if (conversionQueue_.empty()) { >>>> - video_->queueBuffer(buffer); >>>> + if (!rawStream_) >>>> + video_->queueBuffer(buffer); >>>> return; >>>> } >>>> >>>> if (converter_) >>>> converter_->queueBuffers(buffer, conversionQueue_.front().outputs); >>>> - else >>>> + else if (processedRequested()) >>>> /* >>>> * request->sequence() cannot be retrieved from `buffer' inside >>>> * queueBuffers because unique_ptr's make buffer->request() invalid >>>> @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>>> conversionQueue_.front().outputs); >>>> >>>> conversionQueue_.pop(); >>>> + if (rawStream_) >>>> + pipe->completeBuffer(request, buffer); >>>> return; >>>> } >>>> >>>> @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) >>>> void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) >>>> { >>>> /* Queue the input buffer back for capture. */ >>>> - video_->queueBuffer(buffer); >>>> + if (!rawStream_) >>>> + video_->queueBuffer(buffer); >>>> } >>>> >>>> void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) >>>> @@ -1518,11 +1532,15 @@ 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); >>>> >>>> cfg.setStream(&data->streams_[i]); >>>> >>>> - if (data->useConversion_ && !isRaw(cfg)) >>>> + if (data->useConversion_ && !rawStream) >>>> outputCfgs.push_back(cfg); >>>> + >>>> + if (rawStream) >>>> + data->rawStream_ = &data->streams_[i]; >>>> } >>>> >>>> if (outputCfgs.empty()) >>>> @@ -1553,7 +1571,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_) >>>> + if (data->useConversion_ && stream != data->rawStream_) >>>> return data->converter_ >>>> ? data->converter_->exportBuffers(stream, count, buffers) >>>> : data->swIsp_->exportBuffers(stream, count, buffers); >>>> @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>>> return -EBUSY; >>>> } >>>> >>>> - if (data->useConversion_) { >>>> + if (data->useConversion_ && !data->rawStream_) { >>>> /* >>>> * When using the converter allocate a fixed number of internal >>>> * buffers. >>>> @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>>> ret = video->allocateBuffers(kNumInternalBuffers, >>>> &data->conversionBuffers_); >>>> } else { >>>> - /* Otherwise, prepare for using buffers from the only stream. */ >>>> - Stream *stream = &data->streams_[0]; >>>> + /* >>>> + * 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]); >>>> ret = video->importBuffers(stream->configuration().bufferCount); >>>> } >>>> if (ret < 0) { >>>> @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>>> } >>>> >>>> /* Queue all internal buffers for capture. */ >>>> - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >>>> - video->queueBuffer(buffer.get()); >>>> + if (!data->rawStream_) >>>> + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >>>> + video->queueBuffer(buffer.get()); >>>> } >>>> >>>> return 0; >>>> @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) >>>> * queue, it will be handed to the converter in the capture >>>> * completion handler. >>>> */ >>>> - if (data->useConversion_) { >>>> + if (data->useConversion_ && stream != data->rawStream_) { >>>> buffers.emplace(stream, buffer); >>>> metadataRequired = !!data->swIsp_; >>>> } else { >>>> -- >>>> 2.51.0 >>>> >> >> Umang Jain <uajain@igalia.com> writes: >> >>> On Tue, Oct 21, 2025 at 08:27:15PM +0200, Milan Zamazal wrote: >>>> When a raw stream is requested, whether alone or together with a >>>> processed stream, its buffers must be handled outside the software ISP >>> >>>> machinery. They serve as output buffers, even when a processed stream >>>> is produced. >>>> >>>> At most one raw stream and at most one processed stream are supported >>>> and can be combined. An example of producing both raw and processed >>>> files using `cam' application: >>>> >>>> cam -c1 -C100 -Ffile# \ >>>> -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ >>>> -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ >>>> >>>> Note the difference in viewfinder and raw stream sizes due to the fact >>>> that debayering requires enlarging the image width, which enforces >>>> selecting a larger sensor resolution in this case. >>>> >>>> In order to track whether a raw stream is requested and which one it is, >>>> SimpleCameraData::rawStream_ member variable is introduced. >>>> >>>> This is the final step to make raw streams working. >>>> >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- >>>> 1 file changed, 36 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >>>> index 0c3284feb..607b07949 100644 >>>> --- a/src/libcamera/pipeline/simple/simple.cpp >>>> +++ b/src/libcamera/pipeline/simple/simple.cpp >>>> @@ -333,6 +333,7 @@ public: >>>> }; >>>> >>>> std::vector<Stream> streams_; >>>> + Stream *rawStream_; >>>> >>>> /* >>>> * All entities in the pipeline, from the camera sensor to the video >>>> @@ -371,6 +372,11 @@ private: >>>> void ispStatsReady(uint32_t frame, uint32_t bufferId); >>>> void metadataReady(uint32_t frame, const ControlList &metadata); >>>> void setSensorControls(const ControlList &sensorControls); >>>> + >>>> + bool processedRequested() const >>>> + { >>>> + return streams_.size() - (rawStream_ ? 1 : 0) > 0; >>>> + } >>>> }; >>>> >>>> class SimpleCameraConfiguration : public CameraConfiguration >>>> @@ -467,7 +473,7 @@ private: >>>> SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, >>>> unsigned int numStreams, >>>> MediaEntity *sensor) >>>> - : Camera::Private(pipe), streams_(numStreams) >>>> + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) >>>> { >>>> /* >>>> * Find the shortest path from the camera sensor to a video capture >>>> @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>>> * point converting an erroneous buffer. >>>> */ >>>> if (buffer->metadata().status != FrameMetadata::FrameSuccess) { >>>> - if (!useConversion_) { >>>> + if (rawStream_) { >>>> /* 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; >>>> } >>>> @@ -891,7 +900,8 @@ 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) >>>> + if (buffer->metadata().status != FrameMetadata::FrameCancelled && >>>> + !rawStream_) >>>> video_->queueBuffer(buffer); >>>> >>>> if (conversionQueue_.empty()) >>>> @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>>> */ >>>> if (useConversion_) { >>>> if (conversionQueue_.empty()) { >>>> - video_->queueBuffer(buffer); >>>> + if (!rawStream_) >>>> + video_->queueBuffer(buffer); >>>> return; >>>> } >>>> >>>> if (converter_) >>>> converter_->queueBuffers(buffer, conversionQueue_.front().outputs); >>>> - else >>>> + else if (processedRequested()) >>>> /* >>>> * request->sequence() cannot be retrieved from `buffer' inside >>>> * queueBuffers because unique_ptr's make buffer->request() invalid >>>> @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >>>> conversionQueue_.front().outputs); >>>> >>>> conversionQueue_.pop(); >>>> + if (rawStream_) >>>> + pipe->completeBuffer(request, buffer); >>>> return; >>>> } >>>> >>>> @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) >>>> void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) >>>> { >>>> /* Queue the input buffer back for capture. */ >>>> - video_->queueBuffer(buffer); >>>> + if (!rawStream_) >>>> + video_->queueBuffer(buffer); >>>> } >>>> >>>> void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) >>>> @@ -1518,11 +1532,15 @@ 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); >>>> >>>> cfg.setStream(&data->streams_[i]); >>>> >>>> - if (data->useConversion_ && !isRaw(cfg)) >>>> + if (data->useConversion_ && !rawStream) >>>> outputCfgs.push_back(cfg); >>>> + >>>> + if (rawStream) >>>> + data->rawStream_ = &data->streams_[i]; >>> >>> Last comment, just realised: >>> >>> data->rawStream_ should be initialised to nullptr (outside this loop) >>> each time the camera is configured, otherwise it can lead to subtle bugs >>> if the camera configure() is called multiple times with varying >>> configurations. >> >> Right, I'll add it. >> >>>> } >>>> >>>> if (outputCfgs.empty()) >>>> @@ -1553,7 +1571,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_) >>>> + if (data->useConversion_ && stream != data->rawStream_) >>>> return data->converter_ >>>> ? data->converter_->exportBuffers(stream, count, buffers) >>>> : data->swIsp_->exportBuffers(stream, count, buffers); >>>> @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>>> return -EBUSY; >>>> } >>>> >>>> - if (data->useConversion_) { >>>> + if (data->useConversion_ && !data->rawStream_) { >>>> /* >>>> * When using the converter allocate a fixed number of internal >>>> * buffers. >>>> @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>>> ret = video->allocateBuffers(kNumInternalBuffers, >>>> &data->conversionBuffers_); >>>> } else { >>>> - /* Otherwise, prepare for using buffers from the only stream. */ >>>> - Stream *stream = &data->streams_[0]; >>>> + /* >>>> + * 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]); >>>> ret = video->importBuffers(stream->configuration().bufferCount); >>>> } >>>> if (ret < 0) { >>>> @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL >>>> } >>>> >>>> /* Queue all internal buffers for capture. */ >>>> - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >>>> - video->queueBuffer(buffer.get()); >>>> + if (!data->rawStream_) >>>> + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) >>>> + video->queueBuffer(buffer.get()); >>>> } >>>> >>>> return 0; >>>> @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) >>>> * queue, it will be handed to the converter in the capture >>>> * completion handler. >>>> */ >>>> - if (data->useConversion_) { >>>> + if (data->useConversion_ && stream != data->rawStream_) { >>>> buffers.emplace(stream, buffer); >>>> metadataRequired = !!data->swIsp_; >>>> } else { >>>> -- >>>> 2.51.0 >>>>
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 0c3284feb..607b07949 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -333,6 +333,7 @@ public: }; std::vector<Stream> streams_; + Stream *rawStream_; /* * All entities in the pipeline, from the camera sensor to the video @@ -371,6 +372,11 @@ private: void ispStatsReady(uint32_t frame, uint32_t bufferId); void metadataReady(uint32_t frame, const ControlList &metadata); void setSensorControls(const ControlList &sensorControls); + + bool processedRequested() const + { + return streams_.size() - (rawStream_ ? 1 : 0) > 0; + } }; class SimpleCameraConfiguration : public CameraConfiguration @@ -467,7 +473,7 @@ private: SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe, unsigned int numStreams, MediaEntity *sensor) - : Camera::Private(pipe), streams_(numStreams) + : Camera::Private(pipe), streams_(numStreams), rawStream_(nullptr) { /* * Find the shortest path from the camera sensor to a video capture @@ -878,10 +884,13 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) * point converting an erroneous buffer. */ if (buffer->metadata().status != FrameMetadata::FrameSuccess) { - if (!useConversion_) { + if (rawStream_) { /* 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; } @@ -891,7 +900,8 @@ 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) + if (buffer->metadata().status != FrameMetadata::FrameCancelled && + !rawStream_) video_->queueBuffer(buffer); if (conversionQueue_.empty()) @@ -940,13 +950,14 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) */ if (useConversion_) { if (conversionQueue_.empty()) { - video_->queueBuffer(buffer); + if (!rawStream_) + video_->queueBuffer(buffer); return; } if (converter_) converter_->queueBuffers(buffer, conversionQueue_.front().outputs); - else + else if (processedRequested()) /* * request->sequence() cannot be retrieved from `buffer' inside * queueBuffers because unique_ptr's make buffer->request() invalid @@ -956,6 +967,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) conversionQueue_.front().outputs); conversionQueue_.pop(); + if (rawStream_) + pipe->completeBuffer(request, buffer); return; } @@ -993,7 +1006,8 @@ void SimpleCameraData::tryCompleteRequest(Request *request) void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) { /* Queue the input buffer back for capture. */ - video_->queueBuffer(buffer); + if (!rawStream_) + video_->queueBuffer(buffer); } void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) @@ -1518,11 +1532,15 @@ 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); cfg.setStream(&data->streams_[i]); - if (data->useConversion_ && !isRaw(cfg)) + if (data->useConversion_ && !rawStream) outputCfgs.push_back(cfg); + + if (rawStream) + data->rawStream_ = &data->streams_[i]; } if (outputCfgs.empty()) @@ -1553,7 +1571,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_) + if (data->useConversion_ && stream != data->rawStream_) return data->converter_ ? data->converter_->exportBuffers(stream, count, buffers) : data->swIsp_->exportBuffers(stream, count, buffers); @@ -1576,7 +1594,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL return -EBUSY; } - if (data->useConversion_) { + if (data->useConversion_ && !data->rawStream_) { /* * When using the converter allocate a fixed number of internal * buffers. @@ -1584,8 +1602,11 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL ret = video->allocateBuffers(kNumInternalBuffers, &data->conversionBuffers_); } else { - /* Otherwise, prepare for using buffers from the only stream. */ - Stream *stream = &data->streams_[0]; + /* + * 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]); ret = video->importBuffers(stream->configuration().bufferCount); } if (ret < 0) { @@ -1626,8 +1647,9 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL } /* Queue all internal buffers for capture. */ - for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) - video->queueBuffer(buffer.get()); + if (!data->rawStream_) + for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_) + video->queueBuffer(buffer.get()); } return 0; @@ -1678,7 +1700,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) * queue, it will be handed to the converter in the capture * completion handler. */ - if (data->useConversion_) { + if (data->useConversion_ && stream != data->rawStream_) { buffers.emplace(stream, buffer); metadataRequired = !!data->swIsp_; } else {
When a raw stream is requested, whether alone or together with a processed stream, its buffers must be handled outside the software ISP machinery. They serve as output buffers, even when a processed stream is produced. At most one raw stream and at most one processed stream are supported and can be combined. An example of producing both raw and processed files using `cam' application: cam -c1 -C100 -Ffile# \ -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 \ -s role=raw,width=3280,height=2464,pixelformat=SRGGB8 \ Note the difference in viewfinder and raw stream sizes due to the fact that debayering requires enlarging the image width, which enforces selecting a larger sensor resolution in this case. In order to track whether a raw stream is requested and which one it is, SimpleCameraData::rawStream_ member variable is introduced. This is the final step to make raw streams working. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 50 +++++++++++++++++------- 1 file changed, 36 insertions(+), 14 deletions(-)