| Message ID | 20230127154322.29019-10-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2023-01-27 15:43:16) > Add some validation in queueRequest() to ensure that a frame buffer is > provided in a Request if the MandatoryStream flag has been set in the > StreamConfiguration for every active stream. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/camera.cpp | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 2d947a442bff..b0b0a5cb5bb3 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1144,6 +1144,18 @@ int Camera::queueRequest(Request *request) > } > } > > + for (auto const stream : d->activeStreams_) { > + const StreamConfiguration &config = stream->configuration(); > + FrameBuffer *buffer = request->findBuffer(stream); > + > + if (!buffer && config.hints & StreamConfiguration::Hint::MandatoryStream) { > + LOG(Camera, Error) > + << "No buffer provided for mandatory stream"; > + request->_d()->reset(); Hrm ... that's interesting. Should resetting the request be left to the application? or done on errors? If we do reset here, maybe we need to reset other errors paths in this function too. (I'm planning to add a couple more checks in here, so I'm interested in if it's better for apps to automatically reset or not). -- Kieran > + return -ENOMEM; > + } > + } > + > d->pipe_->invokeMethod(&PipelineHandler::queueRequest, > ConnectionTypeQueued, request); > > -- > 2.25.1 >
Hi Kierna, On Fri, 27 Jan 2023 at 21:27, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Naushir Patuck via libcamera-devel (2023-01-27 15:43:16) > > Add some validation in queueRequest() to ensure that a frame buffer is > > provided in a Request if the MandatoryStream flag has been set in the > > StreamConfiguration for every active stream. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/camera.cpp | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 2d947a442bff..b0b0a5cb5bb3 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -1144,6 +1144,18 @@ int Camera::queueRequest(Request *request) > > } > > } > > > > + for (auto const stream : d->activeStreams_) { > > + const StreamConfiguration &config = stream->configuration(); > > + FrameBuffer *buffer = request->findBuffer(stream); > > + > > + if (!buffer && config.hints & StreamConfiguration::Hint::MandatoryStream) { > > + LOG(Camera, Error) > > + << "No buffer provided for mandatory stream"; > > + request->_d()->reset(); > > Hrm ... that's interesting. > > Should resetting the request be left to the application? or done on > errors? Yes, I wasn't 100% sure about this, but the Request destructor does a whole bunch of buffer completion/cancellation signal calls, and I did not think that ought to be done when the request has been rejected synchronously. > > If we do reset here, maybe we need to reset other errors paths in this > function too. (I'm planning to add a couple more checks in here, so I'm > interested in if it's better for apps to automatically reset or not). I think it would be appropriate to do the same for the other error paths in this function, but I would like to confirm this is the right thing to do there. Regards, Naush > > -- > Kieran > > > > > > + return -ENOMEM; > > + } > > + } > > + > > d->pipe_->invokeMethod(&PipelineHandler::queueRequest, > > ConnectionTypeQueued, request); > > > > -- > > 2.25.1 > >
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 2d947a442bff..b0b0a5cb5bb3 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -1144,6 +1144,18 @@ int Camera::queueRequest(Request *request) } } + for (auto const stream : d->activeStreams_) { + const StreamConfiguration &config = stream->configuration(); + FrameBuffer *buffer = request->findBuffer(stream); + + if (!buffer && config.hints & StreamConfiguration::Hint::MandatoryStream) { + LOG(Camera, Error) + << "No buffer provided for mandatory stream"; + request->_d()->reset(); + return -ENOMEM; + } + } + d->pipe_->invokeMethod(&PipelineHandler::queueRequest, ConnectionTypeQueued, request);
Add some validation in queueRequest() to ensure that a frame buffer is provided in a Request if the MandatoryStream flag has been set in the StreamConfiguration for every active stream. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/camera.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+)