| Message ID | 20250804163812.126022-9-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Mon, Aug 04, 2025 at 06:38:11PM +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 | 55 ++++++++++++++++++------ > 1 file changed, 41 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index e5449cd2e..d854d6a72 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -330,6 +330,7 @@ public: > }; > > std::vector<Stream> streams_; > + Stream *rawStream_; > > /* > * All entities in the pipeline, from the camera sensor to the video > @@ -368,6 +369,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 > @@ -460,7 +466,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 > @@ -871,10 +877,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; > } > @@ -884,7 +893,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()) > @@ -933,13 +943,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 > @@ -949,6 +960,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > conversionQueue_.front().outputs); > > conversionQueue_.pop(); > + if (rawStream_) > + pipe->completeBuffer(request, buffer); > return; > } > > @@ -986,7 +999,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) > @@ -1499,11 +1513,20 @@ 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) { > + if (data->rawStream_) { > + LOG(SimplePipeline, Error) << "Multiple raw streams not supported"; > + return -EINVAL; > + } There are 3 places you try to do this validation: - generateConfiguration 3/8 - in validate() 4/8 and now here in configure(). Please refer to the following doc from PipelineHandler::configure() * The configuration is guaranteed to have been validated with * CameraConfiguration::validate(). The pipeline handler implementation shall * not perform further validation and may rely on any custom field stored in its * custom CameraConfiguration derived class. My question here is why? Is it something I am not understanding with this series? There are more comments, the ones I have already comments in previous iterations - which you might have chose to ignore (and that's fine), but still I think the series can be simplied that will gain a very straight-forward review relatively quickly. > + data->rawStream_ = &data->streams_[i]; > + } > } > > if (outputCfgs.empty()) > @@ -1534,7 +1557,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); > @@ -1557,7 +1580,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. > @@ -1565,8 +1588,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) { > @@ -1607,8 +1633,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; > @@ -1659,7 +1686,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.50.1 >
Hi Umang, Umang Jain <uajain@igalia.com> writes: > On Mon, Aug 04, 2025 at 06:38:11PM +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 | 55 ++++++++++++++++++------ >> 1 file changed, 41 insertions(+), 14 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index e5449cd2e..d854d6a72 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -330,6 +330,7 @@ public: >> }; >> >> std::vector<Stream> streams_; >> + Stream *rawStream_; >> >> /* >> * All entities in the pipeline, from the camera sensor to the video >> @@ -368,6 +369,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 >> @@ -460,7 +466,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 >> @@ -871,10 +877,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; >> } >> @@ -884,7 +893,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()) >> @@ -933,13 +943,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 >> @@ -949,6 +960,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >> conversionQueue_.front().outputs); >> >> conversionQueue_.pop(); >> + if (rawStream_) >> + pipe->completeBuffer(request, buffer); >> return; >> } >> >> @@ -986,7 +999,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) >> @@ -1499,11 +1513,20 @@ 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) { >> + if (data->rawStream_) { >> + LOG(SimplePipeline, Error) << "Multiple raw streams not supported"; >> + return -EINVAL; >> + } > > There are 3 places you try to do this validation: > > - generateConfiguration 3/8 > - in validate() 4/8 > > and now here in configure(). Please refer to the following doc from > PipelineHandler::configure() > > * The configuration is guaranteed to have been validated with > * CameraConfiguration::validate(). The pipeline handler implementation shall > * not perform further validation and may rely on any custom field stored in its > * custom CameraConfiguration derived class. > > My question here is why? Is it something I am not understanding with this > series? No, I'll remove this one check in v13. > There are more comments, the ones I have already comments in previous > iterations - which you might have chose to ignore (and that's fine), > but still I think the series can be simplied that will gain a very > straight-forward review relatively quickly. I'm sorry, I got in trouble to track all the stuff in the review. I tried to consolidate the patches based on your comments and patches to make a base for further discussion, but I most likely haven't addressed everything. Could you please remind me about the unresolved comments? >> + data->rawStream_ = &data->streams_[i]; >> + } >> } >> >> if (outputCfgs.empty()) >> @@ -1534,7 +1557,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); >> @@ -1557,7 +1580,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. >> @@ -1565,8 +1588,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) { >> @@ -1607,8 +1633,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; >> @@ -1659,7 +1686,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.50.1 >>
On Wed, Aug 13, 2025 at 12:33:00PM +0200, Milan Zamazal wrote: > Hi Umang, > > Umang Jain <uajain@igalia.com> writes: > > > On Mon, Aug 04, 2025 at 06:38:11PM +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 | 55 ++++++++++++++++++------ > >> 1 file changed, 41 insertions(+), 14 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > >> index e5449cd2e..d854d6a72 100644 > >> --- a/src/libcamera/pipeline/simple/simple.cpp > >> +++ b/src/libcamera/pipeline/simple/simple.cpp > >> @@ -330,6 +330,7 @@ public: > >> }; > >> > >> std::vector<Stream> streams_; > >> + Stream *rawStream_; > >> > >> /* > >> * All entities in the pipeline, from the camera sensor to the video > >> @@ -368,6 +369,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 > >> @@ -460,7 +466,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 > >> @@ -871,10 +877,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; > >> } > >> @@ -884,7 +893,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()) > >> @@ -933,13 +943,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 > >> @@ -949,6 +960,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > >> conversionQueue_.front().outputs); > >> > >> conversionQueue_.pop(); > >> + if (rawStream_) > >> + pipe->completeBuffer(request, buffer); > >> return; > >> } > >> > >> @@ -986,7 +999,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) > >> @@ -1499,11 +1513,20 @@ 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) { > >> + if (data->rawStream_) { > >> + LOG(SimplePipeline, Error) << "Multiple raw streams not supported"; > >> + return -EINVAL; > >> + } > > > > There are 3 places you try to do this validation: > > > > - generateConfiguration 3/8 > > - in validate() 4/8 > > > > and now here in configure(). Please refer to the following doc from > > PipelineHandler::configure() > > > > * The configuration is guaranteed to have been validated with > > * CameraConfiguration::validate(). The pipeline handler implementation shall > > * not perform further validation and may rely on any custom field stored in its > > * custom CameraConfiguration derived class. > > > > My question here is why? Is it something I am not understanding with this > > series? > > No, I'll remove this one check in v13. > > > There are more comments, the ones I have already comments in previous > > iterations - which you might have chose to ignore (and that's fine), > > but still I think the series can be simplied that will gain a very > > straight-forward review relatively quickly. > > I'm sorry, I got in trouble to track all the stuff in the review. I > tried to consolidate the patches based on your comments and patches to > make a base for further discussion, but I most likely haven't addressed > everything. Could you please remind me about the unresolved comments? I cannot make out whether you forgot to address them or you chose not to incorporate those review comments. So, it is not easy for me to tell here. fwiw, the implementation is here[1] and I would say it's built on top of comments I already had in mind, for this series from previous versions. You reported one issue during testing - which I resolved it fairly quickly. So it's is a bit optimised in that sense and I find it better for readability / understanding the flow. So it's really upto you. If you want, at best, I can post the patches for review bashing here or to brainstorming any conflicting ideas. [1]: https://gitlab.freedesktop.org/uajain/libcamera/-/commits/uajain/simple/raw > > >> + data->rawStream_ = &data->streams_[i]; > >> + } > >> } > >> > >> if (outputCfgs.empty()) > >> @@ -1534,7 +1557,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); > >> @@ -1557,7 +1580,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. > >> @@ -1565,8 +1588,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) { > >> @@ -1607,8 +1633,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; > >> @@ -1659,7 +1686,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.50.1 > >> >
Hi Umang, Umang Jain <uajain@igalia.com> writes: > On Wed, Aug 13, 2025 at 12:33:00PM +0200, Milan Zamazal wrote: >> Hi Umang, >> > >> Umang Jain <uajain@igalia.com> writes: >> >> > On Mon, Aug 04, 2025 at 06:38:11PM +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 | 55 ++++++++++++++++++------ >> >> 1 file changed, 41 insertions(+), 14 deletions(-) >> >> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> >> index e5449cd2e..d854d6a72 100644 >> >> --- a/src/libcamera/pipeline/simple/simple.cpp >> >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> >> @@ -330,6 +330,7 @@ public: >> >> }; >> >> >> >> std::vector<Stream> streams_; >> >> + Stream *rawStream_; >> >> >> >> /* >> >> * All entities in the pipeline, from the camera sensor to the video >> >> @@ -368,6 +369,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 >> >> @@ -460,7 +466,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 >> >> @@ -871,10 +877,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; >> >> } >> >> @@ -884,7 +893,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()) >> >> @@ -933,13 +943,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 >> >> @@ -949,6 +960,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >> >> conversionQueue_.front().outputs); >> >> >> >> conversionQueue_.pop(); >> >> + if (rawStream_) >> >> + pipe->completeBuffer(request, buffer); >> >> return; >> >> } >> >> >> >> @@ -986,7 +999,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) >> >> @@ -1499,11 +1513,20 @@ 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) { >> >> + if (data->rawStream_) { >> >> + LOG(SimplePipeline, Error) << "Multiple raw streams not supported"; >> >> + return -EINVAL; >> >> + } >> > >> > There are 3 places you try to do this validation: >> > >> > - generateConfiguration 3/8 >> > - in validate() 4/8 >> > >> > and now here in configure(). Please refer to the following doc from >> > PipelineHandler::configure() >> > >> > * The configuration is guaranteed to have been validated with >> > * CameraConfiguration::validate(). The pipeline handler implementation shall >> > * not perform further validation and may rely on any custom field stored in its >> > * custom CameraConfiguration derived class. >> > >> > My question here is why? Is it something I am not understanding with this >> > series? >> >> No, I'll remove this one check in v13. >> >> > There are more comments, the ones I have already comments in previous >> > iterations - which you might have chose to ignore (and that's fine), >> > but still I think the series can be simplied that will gain a very >> > straight-forward review relatively quickly. >> >> I'm sorry, I got in trouble to track all the stuff in the review. I >> tried to consolidate the patches based on your comments and patches to >> make a base for further discussion, but I most likely haven't addressed >> everything. Could you please remind me about the unresolved comments? > > I cannot make out whether you forgot to address them or you chose > not to incorporate those review comments. So, it is not easy for me > to tell here. If I haven't responded and it's not mentioned in the changelog then it can be raised. > fwiw, the implementation is here[1] and I would say it's built on top > of comments I already had in mind, for this series from previous versions. You > reported one issue during testing - which I resolved it fairly quickly. > So it's is a bit optimised in that sense and I find it better for readability > / understanding the flow. So it's really upto you. If you want, at best, I can > post the patches for review bashing here or to brainstorming any > conflicting ideas. I had thought working with two version could be possible, but it wasn't for me. I looked into your patches to understand what you mean and how it can be done but otherwise I lost track where we're in the review. I'd suggest let's pick one of the series and continue with proper review there (which can include pointing to specific parts of different patches). > [1]: https://gitlab.freedesktop.org/uajain/libcamera/-/commits/uajain/simple/raw > >> >> >> + data->rawStream_ = &data->streams_[i]; >> >> + } >> >> } >> >> >> >> if (outputCfgs.empty()) >> >> @@ -1534,7 +1557,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); >> >> @@ -1557,7 +1580,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. >> >> @@ -1565,8 +1588,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) { >> >> @@ -1607,8 +1633,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; >> >> @@ -1659,7 +1686,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.50.1 >> >> >>
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index e5449cd2e..d854d6a72 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -330,6 +330,7 @@ public: }; std::vector<Stream> streams_; + Stream *rawStream_; /* * All entities in the pipeline, from the camera sensor to the video @@ -368,6 +369,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 @@ -460,7 +466,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 @@ -871,10 +877,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; } @@ -884,7 +893,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()) @@ -933,13 +943,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 @@ -949,6 +960,8 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) conversionQueue_.front().outputs); conversionQueue_.pop(); + if (rawStream_) + pipe->completeBuffer(request, buffer); return; } @@ -986,7 +999,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) @@ -1499,11 +1513,20 @@ 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) { + if (data->rawStream_) { + LOG(SimplePipeline, Error) << "Multiple raw streams not supported"; + return -EINVAL; + } + data->rawStream_ = &data->streams_[i]; + } } if (outputCfgs.empty()) @@ -1534,7 +1557,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); @@ -1557,7 +1580,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. @@ -1565,8 +1588,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) { @@ -1607,8 +1633,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; @@ -1659,7 +1686,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 | 55 ++++++++++++++++++------ 1 file changed, 41 insertions(+), 14 deletions(-)