<div dir="ltr">Hi all,<br><br><br>On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel <<a href="mailto:libcamera-devel@lists.libcamera.org">libcamera-devel@lists.libcamera.org</a>> wrote:<br>><br>> Hi Elias,<br>><br>> On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote:<br>> > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote:<br>> > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote:<br>> > > > Vc4CameraData::findMatchBuffers may return successfully without setting<br>> > > > the embedded buffer. Make sure to initialize the buffer and id to avoid<br>> > > > accessing garbage data.<br>> > ><br>> > > How so ? The function starts with<br>> > ><br>> > >         if (bayerQueue_.empty())<br>> > >                 return false;<br>> > ><br>> > >         /*<br>> > >          * Find the embedded data buffer with a matching timestamp to pass to<br>> > >          * the IPA. Any embedded buffers with a timestamp lower than the<br>> > >          * current bayer buffer will be removed and re-queued to the driver.<br>> > >          */<br>> > >         uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp;<br>> > >         embeddedBuffer = nullptr;<br>> > ><br>> > > so I don't see how it can leave embeddedBuffer unset if it returns<br>> > > successfully.<br>> > ><br>> > > > Without this change, libcamera v0.2.0 usually crashes with an assertion<br>> > > > error:<br>> > > ><br>> > > >  ipa_base.cpp:397 assertion "it != buffers_.end()" failed in prepareIsp()<br>> > ><br>> > > I've seen this indeed, and will test your patch. Maybe it's due to the<br>> > > second change, where you set params.buffers.embedded to 0 ?<br>> ><br>> > Correct: the fix is the second change;<br>><br>> Could you please update the commit message to reflect this ? I read it<br>> as indicating that embeddedBuffer could be null, while the issue is that<br>> params.buffers.embedded is not initialized.<br>><br>> > the first change is defensive in case<br>> > findMatchBuffers for some reason doesn't set embeddedBuffer in case none<br>> > is found.<br>><br>> I'm not sure I'd keep that change. I'll let Naush and David decide what<br>> they like best.<br><br>I agree, let's just keep the change to initialising  params.buffers.embedded.<br>With this, the change itself looks reasonable so<br><br>Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.com</a>><br><br>However... I could have sworn the mojom auto generated class file for ipa::RPi::PrepareParams would initialize all member variables to 0 (including buffers.embedded) on default construction.  I'm away right now so I cannot check this myself, but could anybody else check?  I'm somewhat surprised that this has not been reported more widely...<div><br></div><div>Thanks,</div><div>Naush</div><div><br>><br>><br>> > > > Signed-off-by: Elias Naur <<a href="mailto:mail@eliasnaur.com">mail@eliasnaur.com</a>><br>> > > > ---<br>> > > >  src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++-<br>> > > >  1 file changed, 2 insertions(+), 1 deletion(-)<br>> > > ><br>> > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp<br>> > > > index 26102ea7..d76389f3 100644<br>> > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp<br>> > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp<br>> > > > @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs)<br>> > > ><br>> > > >  void Vc4CameraData::tryRunPipeline()<br>> > > >  {<br>> > > > -     FrameBuffer *embeddedBuffer;<br>> > > > +     FrameBuffer *embeddedBuffer = nullptr;<br>> > > >       BayerFrame bayerFrame;<br>> > > ><br>> > > >       /* If any of our request or buffer queues are empty, we cannot proceed. */<br>> > > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline()<br>> > > >       params.requestControls = request->controls();<br>> > > >       params.ipaContext = request->sequence();<br>> > > >       params.delayContext = bayerFrame.delayContext;<br>> > > > +     params.buffers.embedded = 0;<br>> > > ><br>> > > >       if (embeddedBuffer) {<br>> > > >               unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);<br>><br>> --<br>> Regards,<br>><br>> Laurent Pinchart</div></div>