[libcamera-devel] [PATCH] pipeline: rpi: always initialize the embedded buffer in tryRunPipeline

[Elias Naur]

On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Elias,
>
> Thank you for the patch.
>
> On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote:
> > Vc4CameraData::findMatchBuffers may return successfully without setting
> > the embedded buffer. Make sure to initialize the buffer and id to avoid
> > accessing garbage data.
>
> How so ? The function starts with
>
>         if (bayerQueue_.empty())
>                 return false;
>
>         /*
>          * Find the embedded data buffer with a matching timestamp to pass to
>          * the IPA. Any embedded buffers with a timestamp lower than the
>          * current bayer buffer will be removed and re-queued to the driver.
>          */
>         uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp;
>         embeddedBuffer = nullptr;
>
> so I don't see how it can leave embeddedBuffer unset if it returns
> successfully.
>
> > Without this change, libcamera v0.2.0 usually crashes with an assertion
> > error:
> >
> >  ipa_base.cpp:397 assertion "it != buffers_.end()" failed in prepareIsp()
>
> I've seen this indeed, and will test your patch. Maybe it's due to the
> second change, where you set params.buffers.embedded to 0 ?
>

Correct: the fix is the second change; the first change is defensive in case
findMatchBuffers for some reason doesn't set embeddedBuffer in case none
is found.

> > Signed-off-by: Elias Naur <mail at eliasnaur.com>
> > ---
> >  src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > index 26102ea7..d76389f3 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs)
> >
> >  void Vc4CameraData::tryRunPipeline()
> >  {
> > -     FrameBuffer *embeddedBuffer;
> > +     FrameBuffer *embeddedBuffer = nullptr;
> >       BayerFrame bayerFrame;
> >
> >       /* If any of our request or buffer queues are empty, we cannot proceed. */
> > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline()
> >       params.requestControls = request->controls();
> >       params.ipaContext = request->sequence();
> >       params.delayContext = bayerFrame.delayContext;
> > +     params.buffers.embedded = 0;
> >
> >       if (embeddedBuffer) {
> >               unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
>
> --
> Regards,
>
> Laurent Pinchart