<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 19, 2023 at 3:52 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com">jacopo.mondi@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Harvey<br>
<br>
On Mon, Sep 18, 2023 at 12:18:34PM +0800, Cheng-Hao Yang via libcamera-devel wrote:<br>
> Hi Jacopo,<br>
><br>
> On Thu, Sep 14, 2023 at 3:47 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> wrote:<br>
><br>
> > Hi Harvey<br>
> ><br>
> > On Wed, Sep 13, 2023 at 03:20:50PM +0000, Harvey Yang via libcamera-devel<br>
> > wrote:<br>
> > > From: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> > ><br>
> > > In CameraDevice::processCaptureRequest, we might need to add an internal<br>
> > > buffer for Mapped streams. This patch fixes a case that more than one<br>
> > > Mapped streams depend on a stream that is not requested in one capture<br>
> > > request.<br>
> ><br>
> > Ah! you're right! I wonder how it went unoticed... maybe we never had<br>
> > to create two Mapped streams from a single buffer ? CTS has been run<br>
> > multiple times but we never hit this<br>
> ><br>
> > ><br>
> > > Change-Id: I37a1bcc9c4c2db666a90d74c39883ff18ed11bd5<br>
> ><br>
> > This shouldn't be here. We can remove it if you don't have to re-send<br>
> ><br>
> > > Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> > > ---<br>
> > >  src/android/camera_device.cpp | 2 +-<br>
> > >  1 file changed, 1 insertion(+), 1 deletion(-)<br>
> > ><br>
> > > diff --git a/src/android/camera_device.cpp<br>
> > b/src/android/camera_device.cpp<br>
> > > index 1f7ce440..25cedd44 100644<br>
> > > --- a/src/android/camera_device.cpp<br>
> > > +++ b/src/android/camera_device.cpp<br>
> > > @@ -1077,7 +1077,7 @@ int<br>
> > CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques<br>
> > >               descriptor->request_->addBuffer(sourceStream->stream(),<br>
> > >                                               frameBuffer, nullptr);<br>
> > ><br>
> > > -             requestedStreams.erase(sourceStream);<br>
> > > +             requestedStreams.insert(sourceStream);<br>
> ><br>
> > So, assuming two Mapped streams that map on the same cameraStream.<br>
> ><br>
> > The first processed one won't find a sourceStream in requestedStream<br>
> ><br>
> >         if (requestedStreams.find(sourceStream) != requestedStreams.end())<br>
> >                 continue;<br>
> ><br>
> > so we don't continue and we add create an internal buffer for it and<br>
> > add the framebuffer for the sourceStream to the requet<br>
> ><br>
> >         FrameBuffer *frameBuffer = cameraStream->getBuffer();<br>
> >         buffer.internalBuffer = frameBuffer;<br>
> ><br>
> >         descriptor->request_->addBuffer(sourceStream->stream(),<br>
> >                                         frameBuffer, nullptr);<br>
> ><br>
> > And this clearly was a nop because of the above if () statement<br>
> ><br>
> >         requestedStreams.erase(sourceStream);<br>
> ><br>
> > However, since the second one is a mapped stream too, don't we need to<br>
> > allocate<br>
> > an internal buffer for it ?<br>
> ><br>
> >                 FrameBuffer *frameBuffer = cameraStream->getBuffer();<br>
> >                 buffer.internalBuffer = frameBuffer;<br>
> ><br>
> ><br>
> Not really. The second stream can use the same internal buffer allocated by<br>
> the first stream<br>
> to continue the processing. If we allocate a new internal buffer, this will<br>
> fail anyway:<br>
><br>
>     descriptor->request_->addBuffer(sourceStream->stream(),<br>
>                                   frameBuffer, nullptr);<br>
><br>
> , as libcamera::Request already adds the internal buffer to the<br>
> libcamera::Stream.<br>
<br>
Inded, you're very right! I clearly was confused as I thought an<br>
"internal buffer" had to be allocated, but as we're here handling<br>
mapped streams the destination buffer is provided by the framework.<br>
<br>
Sorry for the noise.<br>
<br>
><br>
> The only purpose of setting the internal buffer is to return the allocated<br>
> buffer<br>
> to the CameraStream which created the buffer. See:<br>
><br>
> <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1246" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1246</a><br>
> <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1370" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1370</a><br>
><br>
> There are no other usages of<br>
> `Camera3RequesetDescriptor::StreamBuffer::internalBuffer`.<br>
<br>
ack<br>
<br>
><br>
> descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer });<br>
><br>
> This will ensure the mapped stream to be processed in:<br>
><br>
> <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1236" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1236</a><br>
><br>
<br>
I re-read the code, but I can't figure out if there are issue in<br>
processing 2 mapped streams created from the same internal buffer, as<br>
I fear that code path has never really been tested ?<br>
<br></blockquote><div><br></div><div>Yeah I understand. Logically, libcamera::Request doesn't care how the</div><div>buffer is created, while true that we don't test it at all yet.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
><br>
> > With your patch applied I presume the second mapped stream will hit<br>
> ><br>
> >         if (requestedStreams.find(sourceStream) != requestedStreams.end())<br>
> >                 continue;<br>
> ><br>
> > and continue, so no buffer will be allocated for it ?<br>
> ><br>
> > Have you got a test case for this to try ?<br>
> ><br>
> ><br>
> Sorry, I've checked with Han-lin, and we don't have such a test with CrOS<br>
> or CTS.<br>
><br>
<br>
Exactly, would be nice to test, but in the meantime, your patch seems<br>
to be fixing a bug indeed.<br>
<br></blockquote><div><br></div><div>Hope it's worth merging now :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks!<br>
<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
<br>
><br>
> > Thanks<br>
> >   j<br>
> ><br>
> > >       }<br>
> > ><br>
> > >       /*<br>
> > > --<br>
> > > 2.42.0.283.g2d96d420d3-goog<br>
> > ><br>
> ><br>
><br>
> Thanks for the review!<br>
><br>
> Harvey<br></blockquote><div><br></div><div>BR,</div><div>Harvey </div></div></div>