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

[Elias Naur]

On Mon, 22 Jan 2024 at 05:31, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> On Mon, Jan 22, 2024 at 10:18:44AM +0000, Naushir Patuck wrote:
> > On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel wrote:
> > > On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote:
> > > > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote:
> > > > > 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;
> > >
> > > Could you please update the commit message to reflect this ? I read it
> > > as indicating that embeddedBuffer could be null, while the issue is that
> > > params.buffers.embedded is not initialized.
> > >
> > > > the first change is defensive in case
> > > > findMatchBuffers for some reason doesn't set embeddedBuffer in case none
> > > > is found.
> > >
> > > I'm not sure I'd keep that change. I'll let Naush and David decide what
> > > they like best.
> >

I don't like uninitialized values in general, but I suppose fixing
those is better done
by, say, compiler flags.

> > I agree, let's just keep the change to initialising  params.buffers.embedded.
> > With this, the change itself looks reasonable so
> >
> > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
>
> Elias, would you be able to send a v2 ?
>

Done.

Elias