<div dir="ltr"><div dir="ltr">Hi Umang,<div><br></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 27, 2022 at 4:41 PM Umang Jain <<a href="mailto:umang.jain@ideasonboard.com" target="_blank">umang.jain@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 6/29/22 16:00, Harvey Yang via libcamera-devel wrote:<br>
> From: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
><br>
> This patch adds the StillCapture stream and imgu1 param buffers.<br>
<br>
<br>
It might be worth to add that we will ignore statistics for still <br>
capture, as they aren't needed for 3A here, right?<br>
<br></blockquote><div><br></div><div>Exactly, thanks!</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">
> The following patches will enable imgu1 to handle StillCapture stream<br>
> specifically, when the imgu0 is needed to handle video/preview streams.<br>
><br>
> Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> src/libcamera/pipeline/ipu3/frames.cpp | 23 +++++++++++++++++++++--<br>
> src/libcamera/pipeline/ipu3/frames.h | 8 +++++++-<br>
> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 +++++++++++++++++++-<br>
> 3 files changed, 47 insertions(+), 4 deletions(-)<br>
><br>
> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp<br>
> index a4c3477c..f9b705a5 100644<br>
> --- a/src/libcamera/pipeline/ipu3/frames.cpp<br>
> +++ b/src/libcamera/pipeline/ipu3/frames.cpp<br>
> @@ -23,7 +23,8 @@ IPU3Frames::IPU3Frames()<br>
> }<br>
> <br>
> void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuffers,<br>
> - const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers)<br>
> + const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers,<br>
> + const std::vector<std::unique_ptr<FrameBuffer>> &captureParamBuffers)<br>
> {<br>
> for (const std::unique_ptr<FrameBuffer> &buffer : paramBuffers)<br>
> availableParamBuffers_.push(buffer.get());<br>
> @@ -31,6 +32,9 @@ void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuff<br>
> for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers)<br>
> availableStatBuffers_.push(buffer.get());<br>
> <br>
> + for (const std::unique_ptr<FrameBuffer> &buffer : captureParamBuffers)<br>
> + availableCaptureParamBuffers_.push(buffer.get());<br>
> +<br>
> frameInfo_.clear();<br>
> }<br>
> <br>
> @@ -38,6 +42,7 @@ void IPU3Frames::clear()<br>
> {<br>
> availableParamBuffers_ = {};<br>
<br>
<br>
alternatively should we rename this to, availableVfParamBuffers_ to mark <br>
the distinction?<br>
<br></blockquote><div><br></div><div>I'm not sure if "availableVfParamBuffers_" describes it better though... It'll be used as ImgU0's param buffers, and ImgU0 is used for video streams. It might not always be used for preview, especially when a JPEG stream is not requested. IIUC "viewfinder" is mostly describing the "preview" stream, right? (Although Han-lin told me that the viewfinder node in ImgU might be more powerful, in terms of field of view, than the output node, which confuses me a lot)</div><div><br></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">
> availableStatBuffers_ = {};<br>
> + availableCaptureParamBuffers_ = {};<br>
> }<br>
> <br>
> IPU3Frames::Info *IPU3Frames::create(Request *request)<br>
> @@ -54,14 +59,22 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)<br>
> return nullptr;<br>
> }<br>
> <br>
> + if (availableCaptureParamBuffers_.empty()) {<br>
> + LOG(IPU3, Debug) << "Capture parameters buffer underrun";<br>
> + return nullptr;<br>
> + }<br>
> +<br>
> FrameBuffer *paramBuffer = availableParamBuffers_.front();<br>
<br>
<br>
Ties into the earlier comment, maybe vfParamBuffer ? ... and so on below..<br>
<br></blockquote><div><br></div><div>ditto</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">
> FrameBuffer *statBuffer = availableStatBuffers_.front();<br>
> + FrameBuffer *captureParamBuffer = availableCaptureParamBuffers_.front();<br>
> <br>
> paramBuffer->_d()->setRequest(request);<br>
> statBuffer->_d()->setRequest(request);<br>
> + captureParamBuffer->_d()->setRequest(request);<br>
> <br>
> availableParamBuffers_.pop();<br>
> availableStatBuffers_.pop();<br>
> + availableCaptureParamBuffers_.pop();<br>
> <br>
> /* \todo Remove the dynamic allocation of Info */<br>
> std::unique_ptr<Info> info = std::make_unique<Info>();<br>
> @@ -71,7 +84,9 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)<br>
> info->rawBuffer = nullptr;<br>
> info->paramBuffer = paramBuffer;<br>
> info->statBuffer = statBuffer;<br>
> + info->captureParamBuffer = captureParamBuffer;<br>
> info->paramDequeued = false;<br>
> + info->captureParamDequeued = false;<br>
> info->metadataProcessed = false;<br>
> <br>
> frameInfo_[id] = std::move(info);<br>
> @@ -84,6 +99,7 @@ void IPU3Frames::remove(IPU3Frames::Info *info)<br>
> /* Return params and stat buffer for reuse. */<br>
> availableParamBuffers_.push(info->paramBuffer);<br>
> availableStatBuffers_.push(info->statBuffer);<br>
> + availableCaptureParamBuffers_.push(info->captureParamBuffer);<br>
> <br>
> /* Delete the extended frame information. */<br>
> frameInfo_.erase(info->id);<br>
> @@ -102,6 +118,9 @@ bool IPU3Frames::tryComplete(IPU3Frames::Info *info)<br>
> if (!info->paramDequeued)<br>
> return false;<br>
> <br>
> + if (!info->captureParamDequeued)<br>
> + return false;<br>
> +<br>
> remove(info);<br>
> <br>
> bufferAvailable.emit();<br>
> @@ -131,7 +150,7 @@ IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)<br>
> return info;<br>
> <br>
> if (info->rawBuffer == buffer || info->paramBuffer == buffer ||<br>
> - info->statBuffer == buffer)<br>
> + info->statBuffer == buffer || info->captureParamBuffer == buffer)<br>
> return info;<br>
> }<br>
> <br>
> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h<br>
> index 6e3cb915..8fcb8a14 100644<br>
> --- a/src/libcamera/pipeline/ipu3/frames.h<br>
> +++ b/src/libcamera/pipeline/ipu3/frames.h<br>
> @@ -36,16 +36,20 @@ public:<br>
> FrameBuffer *paramBuffer;<br>
> FrameBuffer *statBuffer;<br>
> <br>
> + FrameBuffer *captureParamBuffer;<br>
> +<br>
> ControlList effectiveSensorControls;<br>
> <br>
> bool paramDequeued;<br>
> + bool captureParamDequeued;<br>
> bool metadataProcessed;<br>
> };<br>
> <br>
> IPU3Frames();<br>
> <br>
> void init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuffers,<br>
> - const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers);<br>
> + const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers,<br>
> + const std::vector<std::unique_ptr<FrameBuffer>> &captureParamBuffers);<br>
> void clear();<br>
> <br>
> Info *create(Request *request);<br>
> @@ -61,6 +65,8 @@ private:<br>
> std::queue<FrameBuffer *> availableParamBuffers_;<br>
> std::queue<FrameBuffer *> availableStatBuffers_;<br>
> <br>
> + std::queue<FrameBuffer *> availableCaptureParamBuffers_;<br>
> +<br>
> std::map<unsigned int, std::unique_ptr<Info>> frameInfo_;<br>
> };<br>
> <br>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> index e219f704..a201c5ca 100644<br>
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> @@ -70,6 +70,7 @@ public:<br>
> Stream outStream_;<br>
> Stream vfStream_;<br>
> Stream rawStream_;<br>
> + Stream outCaptureStream_;<br>
<br>
<br>
If there is a outCaptureStream_ now, should we be dropping outStream_ <br>
and associated buffer allocations for outStream_ ?<br>
<br>
My guess is yes, let's see what happens in subsequent patches.<br>
<br>
For this one,<br>
<br></blockquote><div><br></div><div>So |outStream_| is still in use with |outCaptureStream_| available, as it's still the main output stream of ImgU0 (ipu3.cpp: line 380).</div><div>Unless you're suggesting that when a JPEG stream is requested, there's at most one video stream requested (which is likely used as the Preview), and it should be supported by ImgU0's viewfinder node.</div><div>However, The Android LIMIT-level guaranteed configuration already has a use-case that supports two YUV/video streams and a JPEG stream [1], not to mention the FULL-level.</div><div><br></div><div>[1]: <a href="https://developer.android.com/reference/android/hardware/camera2/CameraDevice#regular-capture" target="_blank">https://developer.android.com/reference/android/hardware/camera2/CameraDevice#regular-capture</a></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">
Reviewed-by: Umang Jain <<a href="mailto:umang.jain@ideasonboard.com" target="_blank">umang.jain@ideasonboard.com</a>><br>
<br>
> <br>
> Rectangle cropRegion_;<br>
> bool supportsFlips_;<br>
> @@ -696,6 +697,8 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,<br>
> return imgu0_.viewfinder_->exportBuffers(count, buffers);<br>
> else if (stream == &data->rawStream_)<br>
> return data->cio2_.exportBuffers(count, buffers);<br>
> + else if (stream == &data->outCaptureStream_)<br>
> + return imgu1_.output_->exportBuffers(count, buffers);<br>
> <br>
> return -EINVAL;<br>
> }<br>
> @@ -718,11 +721,17 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)<br>
> data->outStream_.configuration().bufferCount,<br>
> data->vfStream_.configuration().bufferCount,<br>
> data->rawStream_.configuration().bufferCount,<br>
> + data->outCaptureStream_.configuration().bufferCount,<br>
> });<br>
> <br>
> ret = imgu0_.allocateBuffers(bufferCount);<br>
> if (ret < 0)<br>
> return ret;<br>
> + ret = imgu1_.allocateBuffers(bufferCount);<br>
> + if (ret < 0) {<br>
> + imgu0_.freeBuffers();<br>
> + return ret;<br>
> + }<br>
> <br>
> /* Map buffers to the IPA. */<br>
> unsigned int ipaBufferId = 1;<br>
> @@ -737,9 +746,14 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)<br>
> ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());<br>
> }<br>
> <br>
> + for (const std::unique_ptr<FrameBuffer> &buffer : imgu1_.paramBuffers_) {<br>
> + buffer->setCookie(ipaBufferId++);<br>
> + ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());<br>
> + }<br>
> +<br>
> data->ipa_->mapBuffers(ipaBuffers_);<br>
> <br>
> - data->frameInfos_.init(imgu0_.paramBuffers_, imgu0_.statBuffers_);<br>
> + data->frameInfos_.init(imgu0_.paramBuffers_, imgu0_.statBuffers_, imgu1_.paramBuffers_);<br>
> data->frameInfos_.bufferAvailable.connect(<br>
> data, &IPU3CameraData::queuePendingRequests);<br>
> <br>
> @@ -760,6 +774,7 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)<br>
> ipaBuffers_.clear();<br>
> <br>
> imgu0_.freeBuffers();<br>
> + imgu1_.freeBuffers();<br>
> <br>
> return 0;<br>
> }<br>
> @@ -1143,6 +1158,7 @@ int PipelineHandlerIPU3::registerCameras()<br>
> &data->outStream_,<br>
> &data->vfStream_,<br>
> &data->rawStream_,<br>
> + &data->outCaptureStream_,<br>
> };<br>
> CIO2Device *cio2 = &data->cio2_;<br>
> <br>
> @@ -1318,6 +1334,8 @@ void IPU3CameraData::paramsBufferReady(unsigned int id)<br>
> imgu0_->param_->queueBuffer(info->paramBuffer);<br>
> imgu0_->stat_->queueBuffer(info->statBuffer);<br>
> imgu0_->input_->queueBuffer(info->rawBuffer);<br>
> +<br>
> + info->captureParamDequeued = true;<br>
> }<br>
> <br>
> void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)<br>
</blockquote></div></div>