[libcamera-devel] [PATCH v6 09/15] libcamera: camera: Validate MandatoryStream in queueRequest()

[Naushir Patuck]

Hi Kierna,

On Fri, 27 Jan 2023 at 21:27, Kieran Bingham
<kieran.bingham at 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 at 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
> >