| Message ID | 20260604095105.68798-6-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2026-06-04 10:50:47) > Currently, if a call to queueBuffers fails, the error is ignored. Which > means buffers are not returned back, as would be the case at the end of > normal processing. Let's cancel the request, which also returns the > output buffers. The input buffer is returned as well in case of raw > streams and must be returned manually in case of non-raw streams. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 28 +++++++++++++++++++----- > 1 file changed, 23 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index c6fe12d65..93899699e 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -955,16 +955,34 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > return; > } > > - if (converter_) > - converter_->queueBuffers(buffer, conversionQueue_.front().outputs); > - else > + int ret; > + if (converter_) { > + ret = converter_->queueBuffers(buffer, conversionQueue_.front().outputs); > + } else { > /* > * request->sequence() cannot be retrieved from `buffer' inside > * queueBuffers because unique_ptr's make buffer->request() invalid > * already here. > */ > - swIsp_->queueBuffers(request->sequence(), buffer, > - conversionQueue_.front().outputs); > + ret = swIsp_->queueBuffers(request->sequence(), buffer, > + conversionQueue_.front().outputs); > + } > + > + if (ret < 0) { > + LOG(SimplePipeline, Error) > + << "Failed to queue buffers for conversion: " > + << strerror(-ret); > + /* > + * The buffers were rejected before starting any processing, so the > + * output buffers will not be returned via the normal done > + * signals. Cancel the request; this handles the output buffers and > + * also the input buffer if it's a raw stream buffer. For non-raw > + * streams, return the input buffer to the video device manually. > + */ This feels a bit awkward, I wonder if we could do better here (globally? not just in simple I think unless this is due to something specific to simple?) > + pipe->cancelRequest(request); Is there any difference calling request->_d()->cancel()? Aha, I see, we have to call cancel on the pipeline handler to complete it back to the application. I'm curious why this is only called by simple and virtual... Something seems fishy here ... ? > + if (!rawStream_) > + video_->queueBuffer(buffer); > + } > > conversionQueue_.pop(); > return; > -- > 2.54.0 >
Hi Kieran, thank you for review. Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2026-06-04 10:50:47) >> Currently, if a call to queueBuffers fails, the error is ignored. Which >> means buffers are not returned back, as would be the case at the end of > >> normal processing. Let's cancel the request, which also returns the >> output buffers. The input buffer is returned as well in case of raw >> streams and must be returned manually in case of non-raw streams. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 28 +++++++++++++++++++----- >> 1 file changed, 23 insertions(+), 5 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index c6fe12d65..93899699e 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -955,16 +955,34 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) >> return; >> } >> >> - if (converter_) >> - converter_->queueBuffers(buffer, conversionQueue_.front().outputs); >> - else >> + int ret; >> + if (converter_) { >> + ret = converter_->queueBuffers(buffer, conversionQueue_.front().outputs); >> + } else { >> /* >> * request->sequence() cannot be retrieved from `buffer' inside >> * queueBuffers because unique_ptr's make buffer->request() invalid >> * already here. >> */ >> - swIsp_->queueBuffers(request->sequence(), buffer, >> - conversionQueue_.front().outputs); >> + ret = swIsp_->queueBuffers(request->sequence(), buffer, >> + conversionQueue_.front().outputs); >> + } >> + >> + if (ret < 0) { >> + LOG(SimplePipeline, Error) >> + << "Failed to queue buffers for conversion: " >> + << strerror(-ret); >> + /* >> + * The buffers were rejected before starting any processing, so the >> + * output buffers will not be returned via the normal done >> + * signals. Cancel the request; this handles the output buffers and >> + * also the input buffer if it's a raw stream buffer. For non-raw >> + * streams, return the input buffer to the video device manually. >> + */ > > This feels a bit awkward, Yes. > I wonder if we could do better here (globally? not just in simple I > think unless this is due to something specific to simple?) A tricky question, I'm afraid I don't have an answer. >> + pipe->cancelRequest(request); > > Is there any difference calling request->_d()->cancel()? > > Aha, I see, we have to call cancel on the pipeline handler to complete > it back to the application. > > I'm curious why this is only called by simple and virtual... > > Something seems fishy here ... ? Another tricky question... >> + if (!rawStream_) >> + video_->queueBuffer(buffer); >> + } >> >> conversionQueue_.pop(); >> return; >> -- >> 2.54.0 >>
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index c6fe12d65..93899699e 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -955,16 +955,34 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) return; } - if (converter_) - converter_->queueBuffers(buffer, conversionQueue_.front().outputs); - else + int ret; + if (converter_) { + ret = converter_->queueBuffers(buffer, conversionQueue_.front().outputs); + } else { /* * request->sequence() cannot be retrieved from `buffer' inside * queueBuffers because unique_ptr's make buffer->request() invalid * already here. */ - swIsp_->queueBuffers(request->sequence(), buffer, - conversionQueue_.front().outputs); + ret = swIsp_->queueBuffers(request->sequence(), buffer, + conversionQueue_.front().outputs); + } + + if (ret < 0) { + LOG(SimplePipeline, Error) + << "Failed to queue buffers for conversion: " + << strerror(-ret); + /* + * The buffers were rejected before starting any processing, so the + * output buffers will not be returned via the normal done + * signals. Cancel the request; this handles the output buffers and + * also the input buffer if it's a raw stream buffer. For non-raw + * streams, return the input buffer to the video device manually. + */ + pipe->cancelRequest(request); + if (!rawStream_) + video_->queueBuffer(buffer); + } conversionQueue_.pop(); return;
Currently, if a call to queueBuffers fails, the error is ignored. Which means buffers are not returned back, as would be the case at the end of normal processing. Let's cancel the request, which also returns the output buffers. The input buffer is returned as well in case of raw streams and must be returned manually in case of non-raw streams. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 28 +++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)