<div dir="ltr"><div dir="ltr">Hi Umang,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 18 Nov 2020 at 13:38, Umang Jain <<a href="mailto:email@uajain.com">email@uajain.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">
<div>
Hi Naush,<br>
<br>
<div>On 11/18/20 6:56 PM, Naushir Patuck
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">Hi Umang,
<div><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, 18 Nov 2020 at
13:16, Umang Jain <<a href="mailto:email@uajain.com" target="_blank">email@uajain.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 Naush,<br>
<br>
On 11/12/20 2:29 PM, Naushir Patuck wrote:<br>
> Applications now have the ability to pass in controls
that need to be<br>
> applied on startup, rather than doing it through
Request where there might<br>
> be some frames of delay in getting the controls
applied.<br>
><br>
> This commit adds the ability to pass in a set of
libcamera controls into<br>
> the pipeline handlers through the
pipeline_handler::start() method. These<br>
> controls are provided by the application through the
camera::start()<br>
> public API.<br>
Is there a possiblity that the controls are passed in
through <br>
Camera::configure() that will subsequently call <br>
PipeHandler::configure()? Looking at the way Requests use
ControlList, <br>
it seems there is also a 'validator' that can(/should?) be
used with <br>
this API. *IF* we take that into account, then it feels like
<br>
Camera::configure() is a good place for passing in
ControlsList, rather <br>
than Camera::start()<br>
<br>
This is just a quick comment / line of thought. I haven't
actually dug <br>
deep into the Request/ControlsList code at this very moment.<br>
</blockquote>
<div><br>
</div>
<div>Indeed this was an option that we considered. However,
there are some usage issues with passing in controls on
configure(). Below is an extract from an earlier conversion
with Kieran on this topic:</div>
<div><br>
</div>
<div><span style="color:rgb(80,0,80)">
<div>On Mon, 19 Oct 2020 at 15:01, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>>
wrote: </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Then when a pipeline
is called with configure(), it needs to apply any<br>
controls in the list at that point. And it looks like
the<br>
CameraConfiguration is already passed through to:<br>
<br>
int RPiCameraData::configureIPA(const
CameraConfiguration *config)<br>
<br>
So, then there is the information required at that phase
too?<br>
<br>
I'm wondering if I've missed something obvious that
would have prevented<br>
you from already using this approach?<br>
</blockquote>
<div><br>
</div>
</span>
<div>Indeed, this was one of the approaches we discussed
some time back. However, as you mentioned,
CameraConfiguration gets set in the camera->configure()
phase, and this may not necessarily be what we want. An
application may want to set startup parameters without
doing a camera->configure() in a run. With this
change, we would do a sequence like <span>start</span>(),
stop() <span>start</span>(new config parameters). If we
were to do <span>start</span>(), stop() configure(new
config parameters), <span>start</span>(), it may incur
additional overheads in calling configure() only to setup
some new startup params, where the configure() might be
going to the kernel and calling the sensors to set
itself up again, even though it is not needed. Hope that
makes sense?</div>
</div>
</div>
</div>
</blockquote>
Ah okay, so this was already considered and discussed. I didn't see
any version tag(`vX`) on the series hence I thought this is brand
new. My apologies.<br>
<br>
Okay, I do see the point, in not going through configure(), as you
have discussed and implemented here. However, I am still under the
impression that, we might need to stick in a 'validate' before
calling Camera::start() with new control, i.e. <br>
<br>
sequence like <span>start</span>(), stop(), validate(new control
params), <span>start</span>(new control params)<br>
<br>
Maybe it's not needed but it surely feels like, it should be
present. And if the new controls fails the validation, we should
prevent starting of the Camera again. This is what I think should
happen (From application developer point of view).<br>
Again, I am not expert here and neither I have looked deep into the
code as of now.<br>
<br></div></blockquote><div><br></div><div>I am not entirely familiar with the control validator. Let me look through it's usage and get back to you. So presumably this validator would belong to the Camera class in your description above?</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"><div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>Regards,</div>
<div>Naush</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">
<br>
Thanks!<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> Documentation/guides/pipeline-handler.rst |
4 ++--<br>
> include/libcamera/camera.h |
2 +-<br>
> include/libcamera/internal/pipeline_handler.h |
2 +-<br>
> src/libcamera/camera.cpp |
11 ++++++-----<br>
> src/libcamera/pipeline/ipu3/ipu3.cpp |
4 ++--<br>
> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |
4 ++--<br>
> src/libcamera/pipeline/rkisp1/rkisp1.cpp |
4 ++--<br>
> src/libcamera/pipeline/simple/simple.cpp |
4 ++--<br>
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp |
4 ++--<br>
> src/libcamera/pipeline/vimc/vimc.cpp |
4 ++--<br>
> src/libcamera/pipeline_handler.cpp |
1 +<br>
> 11 files changed, 23 insertions(+), 21 deletions(-)<br>
><br>
> diff --git a/Documentation/guides/pipeline-handler.rst
b/Documentation/guides/pipeline-handler.rst<br>
> index 57aee455..63275a12 100644<br>
> --- a/Documentation/guides/pipeline-handler.rst<br>
> +++ b/Documentation/guides/pipeline-handler.rst<br>
> @@ -209,7 +209,7 @@ methods for the overridden class
members.<br>
> int exportFrameBuffers(Camera *camera,
Stream *stream,<br>
>
std::vector<std::unique_ptr<FrameBuffer>>
*buffers) override;<br>
> <br>
> - int start(Camera *camera) override;<br>
> + int start(Camera *camera, ControlList
*controls) override;<br>
> void stop(Camera *camera) override;<br>
> <br>
> int queueRequestDevice(Camera *camera,
Request *request) override;<br>
> @@ -239,7 +239,7 @@ methods for the overridden class
members.<br>
> return -1;<br>
> }<br>
> <br>
> - int PipelineHandlerVivid::start(Camera *camera)<br>
> + int PipelineHandlerVivid::start(Camera *camera,
ControlList *controls)<br>
> {<br>
> return -1;<br>
> }<br>
> diff --git a/include/libcamera/camera.h
b/include/libcamera/camera.h<br>
> index 5c5f1a05..f94f8599 100644<br>
> --- a/include/libcamera/camera.h<br>
> +++ b/include/libcamera/camera.h<br>
> @@ -103,7 +103,7 @@ public:<br>
> std::unique_ptr<Request>
createRequest(uint64_t cookie = 0);<br>
> int queueRequest(Request *request);<br>
> <br>
> - int start();<br>
> + int start(ControlList *controls = nullptr);<br>
> int stop();<br>
> <br>
> private:<br>
> diff --git
a/include/libcamera/internal/pipeline_handler.h
b/include/libcamera/internal/pipeline_handler.h<br>
> index c12c8904..bd3c4a81 100644<br>
> --- a/include/libcamera/internal/pipeline_handler.h<br>
> +++ b/include/libcamera/internal/pipeline_handler.h<br>
> @@ -78,7 +78,7 @@ public:<br>
> virtual int exportFrameBuffers(Camera *camera,
Stream *stream,<br>
>
std::vector<std::unique_ptr<FrameBuffer>>
*buffers) = 0;<br>
> <br>
> - virtual int start(Camera *camera) = 0;<br>
> + virtual int start(Camera *camera, ControlList
*controls) = 0;<br>
> virtual void stop(Camera *camera) = 0;<br>
> <br>
> int queueRequest(Camera *camera, Request
*request);<br>
> diff --git a/src/libcamera/camera.cpp
b/src/libcamera/camera.cpp<br>
> index dffbd6bd..de9c6c86 100644<br>
> --- a/src/libcamera/camera.cpp<br>
> +++ b/src/libcamera/camera.cpp<br>
> @@ -943,9 +943,10 @@ int Camera::queueRequest(Request
*request)<br>
> /**<br>
> * \brief Start capture from camera<br>
> *<br>
> - * Start the camera capture session. Once the camera
is started the application<br>
> - * can queue requests to the camera to process and
return to the application<br>
> - * until the capture session is terminated with \a
stop().<br>
> + * Start the camera capture session, optionally
providing a list of controls to<br>
> + * action before starting. Once the camera is started
the application can queue<br>
> + * requests to the camera to process and return to the
application until the<br>
> + * capture session is terminated with \a stop().<br>
> *<br>
> * \context This function may only be called when the
camera is in the<br>
> * Configured state as defined in \ref
camera_operation, and shall be<br>
> @@ -956,7 +957,7 @@ int Camera::queueRequest(Request
*request)<br>
> * \retval -ENODEV The camera has been disconnected
from the system<br>
> * \retval -EACCES The camera is not in a state where
it can be started<br>
> */<br>
> -int Camera::start()<br>
> +int Camera::start(ControlList *controls)<br>
> {<br>
> Private *const d = LIBCAMERA_D_PTR();<br>
> <br>
> @@ -967,7 +968,7 @@ int Camera::start()<br>
> LOG(Camera, Debug) << "Starting capture";<br>
> <br>
> ret =
d->pipe_->invokeMethod(&PipelineHandler::start,<br>
> -
ConnectionTypeBlocking, this);<br>
> +
ConnectionTypeBlocking, this, controls);<br>
> if (ret)<br>
> return ret;<br>
> <br>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> index 4cedb32b..8a1918d5 100644<br>
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> @@ -105,7 +105,7 @@ public:<br>
> int exportFrameBuffers(Camera *camera, Stream
*stream,<br>
>
std::vector<std::unique_ptr<FrameBuffer>>
*buffers) override;<br>
> <br>
> - int start(Camera *camera) override;<br>
> + int start(Camera *camera, ControlList *controls)
override;<br>
> void stop(Camera *camera) override;<br>
> <br>
> int queueRequestDevice(Camera *camera, Request
*request) override;<br>
> @@ -596,7 +596,7 @@ int
PipelineHandlerIPU3::freeBuffers(Camera *camera)<br>
> return 0;<br>
> }<br>
> <br>
> -int PipelineHandlerIPU3::start(Camera *camera)<br>
> +int PipelineHandlerIPU3::start(Camera *camera,
[[maybe_unused]] ControlList *controls)<br>
> {<br>
> IPU3CameraData *data = cameraData(camera);<br>
> CIO2Device *cio2 = &data->cio2_;<br>
> diff --git
a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 7ad66f21..ddb30e49 100644<br>
> ---
a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++
b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -237,7 +237,7 @@ public:<br>
> int exportFrameBuffers(Camera *camera, Stream
*stream,<br>
>
std::vector<std::unique_ptr<FrameBuffer>>
*buffers) override;<br>
> <br>
> - int start(Camera *camera) override;<br>
> + int start(Camera *camera, ControlList *controls)
override;<br>
> void stop(Camera *camera) override;<br>
> <br>
> int queueRequestDevice(Camera *camera, Request
*request) override;<br>
> @@ -726,7 +726,7 @@ int
PipelineHandlerRPi::exportFrameBuffers([[maybe_unused]]
Camera *camera, Stre<br>
> return ret;<br>
> }<br>
> <br>
> -int PipelineHandlerRPi::start(Camera *camera)<br>
> +int PipelineHandlerRPi::start(Camera *camera,
[[maybe_unused]] ControlList *controls)<br>
> {<br>
> RPiCameraData *data = cameraData(camera);<br>
> int ret;<br>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> index 1b1922a9..2e8d2930 100644<br>
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> @@ -187,7 +187,7 @@ public:<br>
> int exportFrameBuffers(Camera *camera, Stream
*stream,<br>
>
std::vector<std::unique_ptr<FrameBuffer>>
*buffers) override;<br>
> <br>
> - int start(Camera *camera) override;<br>
> + int start(Camera *camera, ControlList *controls)
override;<br>
> void stop(Camera *camera) override;<br>
> <br>
> int queueRequestDevice(Camera *camera, Request
*request) override;<br>
> @@ -822,7 +822,7 @@ int
PipelineHandlerRkISP1::freeBuffers(Camera *camera)<br>
> return 0;<br>
> }<br>
> <br>
> -int PipelineHandlerRkISP1::start(Camera *camera)<br>
> +int PipelineHandlerRkISP1::start(Camera *camera,
[[maybe_unused]] ControlList *controls)<br>
> {<br>
> RkISP1CameraData *data = cameraData(camera);<br>
> int ret;<br>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp
b/src/libcamera/pipeline/simple/simple.cpp<br>
> index 0d3078f7..b047aeb9 100644<br>
> --- a/src/libcamera/pipeline/simple/simple.cpp<br>
> +++ b/src/libcamera/pipeline/simple/simple.cpp<br>
> @@ -126,7 +126,7 @@ public:<br>
> int exportFrameBuffers(Camera *camera, Stream
*stream,<br>
>
std::vector<std::unique_ptr<FrameBuffer>>
*buffers) override;<br>
> <br>
> - int start(Camera *camera) override;<br>
> + int start(Camera *camera, ControlList *controls)
override;<br>
> void stop(Camera *camera) override;<br>
> <br>
> bool match(DeviceEnumerator *enumerator)
override;<br>
> @@ -646,7 +646,7 @@ int
SimplePipelineHandler::exportFrameBuffers(Camera *camera,
Stream *stream,<br>
> return
data->video_->exportBuffers(count, buffers);<br>
> }<br>
> <br>
> -int SimplePipelineHandler::start(Camera *camera)<br>
> +int SimplePipelineHandler::start(Camera *camera,
[[maybe_unused]] ControlList *controls)<br>
> {<br>
> SimpleCameraData *data = cameraData(camera);<br>
> V4L2VideoDevice *video = data->video_;<br>
> diff --git
a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
> index 0f3241cc..87b0f03d 100644<br>
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
> @@ -76,7 +76,7 @@ public:<br>
> int exportFrameBuffers(Camera *camera, Stream
*stream,<br>
>
std::vector<std::unique_ptr<FrameBuffer>>
*buffers) override;<br>
> <br>
> - int start(Camera *camera) override;<br>
> + int start(Camera *camera, ControlList *controls)
override;<br>
> void stop(Camera *camera) override;<br>
> <br>
> int queueRequestDevice(Camera *camera, Request
*request) override;<br>
> @@ -236,7 +236,7 @@ int
PipelineHandlerUVC::exportFrameBuffers(Camera *camera,
Stream *stream,<br>
> return data->video_->exportBuffers(count,
buffers);<br>
> }<br>
> <br>
> -int PipelineHandlerUVC::start(Camera *camera)<br>
> +int PipelineHandlerUVC::start(Camera *camera,
[[maybe_unused]] ControlList *controls)<br>
> {<br>
> UVCCameraData *data = cameraData(camera);<br>
> unsigned int count =
data->stream_.configuration().bufferCount;<br>
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp
b/src/libcamera/pipeline/vimc/vimc.cpp<br>
> index 914b6b54..d81b8598 100644<br>
> --- a/src/libcamera/pipeline/vimc/vimc.cpp<br>
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp<br>
> @@ -92,7 +92,7 @@ public:<br>
> int exportFrameBuffers(Camera *camera, Stream
*stream,<br>
>
std::vector<std::unique_ptr<FrameBuffer>>
*buffers) override;<br>
> <br>
> - int start(Camera *camera) override;<br>
> + int start(Camera *camera, ControlList *controls)
override;<br>
> void stop(Camera *camera) override;<br>
> <br>
> int queueRequestDevice(Camera *camera, Request
*request) override;<br>
> @@ -313,7 +313,7 @@ int
PipelineHandlerVimc::exportFrameBuffers(Camera *camera,
Stream *stream,<br>
> return data->video_->exportBuffers(count,
buffers);<br>
> }<br>
> <br>
> -int PipelineHandlerVimc::start(Camera *camera)<br>
> +int PipelineHandlerVimc::start(Camera *camera,
[[maybe_unused]] ControlList *controls)<br>
> {<br>
> VimcCameraData *data = cameraData(camera);<br>
> unsigned int count =
data->stream_.configuration().bufferCount;<br>
> diff --git a/src/libcamera/pipeline_handler.cpp
b/src/libcamera/pipeline_handler.cpp<br>
> index 894200ee..bafcf21b 100644<br>
> --- a/src/libcamera/pipeline_handler.cpp<br>
> +++ b/src/libcamera/pipeline_handler.cpp<br>
> @@ -351,6 +351,7 @@ const ControlList
&PipelineHandler::properties(const Camera *camera) const<br>
> * \fn PipelineHandler::start()<br>
> * \brief Start capturing from a group of streams<br>
> * \param[in] camera The camera to start<br>
> + * \param[in] controls Controls for the IPA to action
before starting the camera<br>
> *<br>
> * Start the group of streams that have been
configured for capture by<br>
> * \a configure(). The intended caller of this method
is the Camera class which<br>
<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote></div></div>