<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Sep 14, 2023 at 3:47 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 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>
><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 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 allocate<br>
an internal buffer for it ?<br>
<br>
                FrameBuffer *frameBuffer = cameraStream->getBuffer();<br>
                buffer.internalBuffer = frameBuffer;<br>
<br></blockquote><div><br></div><div>Not really. The second stream can use the same internal buffer allocated by the first stream</div><div>to continue the processing. If we allocate a new internal buffer, this will fail anyway:</div><div><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px"><code>    descriptor<span class="gmail-hl gmail-opt">-></span>request_<span class="gmail-hl gmail-opt">-></span><span class="gmail-hl gmail-kwd" style="color:rgb(1,1,129)">addBuffer</span><span class="gmail-hl gmail-opt">(</span>sourceStream<span class="gmail-hl gmail-opt">-></span><span class="gmail-hl gmail-kwd" style="color:rgb(1,1,129)">stream</span><span class="gmail-hl gmail-opt">(),</span>
                                    frameBuffer<span class="gmail-hl gmail-opt">,</span> <span class="gmail-hl gmail-kwc" style="font-weight:bold">nullptr</span><span class="gmail-hl gmail-opt">);</span></code></pre></div><div>, as libcamera::Request already adds the internal buffer to the libcamera::Stream.</div><div><br></div><div>The only purpose of setting the internal buffer is to return the allocated buffer</div><div>to the CameraStream which created the buffer. See:</div><div><br></div><div><a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1246">https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1246</a><br></div><div><a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1370">https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1370</a><br></div><div><br></div><div>There are no other usages of `Camera3RequesetDescriptor::StreamBuffer::internalBuffer`.</div><div><br></div><div><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0)"><code style="">descriptor<span class="gmail-hl gmail-opt" style="">-></span>pendingStreamsToProcess_<span class="gmail-hl gmail-opt" style="">.</span><span class="gmail-hl gmail-kwd" style="color:rgb(1,1,129)">insert</span><span class="gmail-hl gmail-opt" style="">({</span> cameraStream<span class="gmail-hl gmail-opt" style="">, &</span>buffer <span class="gmail-hl gmail-opt" style="">});</span></code></pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0)"><span style="color:rgb(34,34,34);white-space:normal"><font face="arial, sans-serif">This will ensure the mapped stream to be processed in:</font></span></pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0)"><font face="arial, sans-serif"><a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1236">https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1236</a></font></pre></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">
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></blockquote><div><br></div><div>Sorry, I've checked with Han-lin, and we don't have such a test with CrOS or CTS.</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>
  j<br>
<br>
>       }<br>
><br>
>       /*<br>
> --<br>
> 2.42.0.283.g2d96d420d3-goog<br>
><br></blockquote><div><br></div><div>Thanks for the review!</div><div><br></div><div>Harvey </div></div></div>