<div dir="ltr">Thanks Jacopo! LGTM.<div>Also thanks Laurent for the suggestions.</div><div><br></div><div>BR,</div><div>Harvey</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Sep 21, 2023 at 6:50 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 Laurent<br>
<br>
On Thu, Sep 21, 2023 at 12:08:57PM +0300, Laurent Pinchart via libcamera-devel wrote:<br>
> Hello,<br>
><br>
> The subject line should start with "android: ".<br>
><br>
> On Tue, Sep 19, 2023 at 09:52:13AM +0200, Jacopo Mondi via libcamera-devel wrote:<br>
> > On Mon, Sep 18, 2023 at 12:18:34PM +0800, Cheng-Hao Yang via libcamera-devel wrote:<br>
> > > On Thu, Sep 14, 2023 at 3:47 PM Jacopo Mondi wrote:<br>
> > > > On Wed, Sep 13, 2023 at 03:20:50PM +0000, Harvey Yang via libcamera-devel 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>
> > > > > Change-Id: I37a1bcc9c4c2db666a90d74c39883ff18ed11bd5<br>
> > > ><br>
> > > > This shouldn't be here. We can remove it if you don't have to re-send<br>
><br>
> On the other hand, a Fixes: line would be nice. I think<br>
><br>
> Fixes: 7ea83eba0df6 ("android: camera_device: Postpone mapped streams handling")<br>
><br>
> is the right one.<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 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 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>
> It could be nice to capture a bit more context in the commit message, I<br>
> had to read through the implementation to understand the issue fixed by<br>
> this patch.<br>
><br>
> Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
><br>
<br>
With Harvey's ack I'll push with the following commit message<br>
<br>
-------------------------------------------------------------------------------<br>
android: camera_device: Fix requestedStream handling<br>
<br>
The Android CameraDevice class adds a sourceStream for each Mapped<br>
stream requested by the framework.<br>
<br>
When mapping multiple framework streams to the same sourceStream, the<br>
implementation of CameraDevice::processCaptureRequest wrongly erases the<br>
just added sourceStream from the list of streams to request to<br>
libcamera.<br>
<br>
Fix this by adding the stream instead of erasing it.<br>
-------------------------------------------------------------------------------<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>
> > > 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>
> > > 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>
> > 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>
> > > > 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>
> > > Sorry, I've checked with Han-lin, and we don't have such a test with CrOS<br>
> > > or CTS.<br>
> ><br>
> > Exactly, would be nice to test, but in the meantime, your patch seems<br>
> > to be fixing a bug indeed.<br>
> ><br>
> > Thanks!<br>
> ><br>
> > Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> ><br>
> > > > > }<br>
> > > > ><br>
> > > > > /*<br>
><br>
> --<br>
> Regards,<br>
><br>
> Laurent Pinchart<br>
</blockquote></div>