<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thanks for your comments!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Aug 3, 2022 at 1:58 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@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">On Wed, Aug 03, 2022 at 08:52:01AM +0300, Laurent Pinchart via libcamera-devel wrote:<br>
> Hi Harvey,<br>
> <br>
> Thank you for the patch.<br>
> <br>
> On Tue, Aug 02, 2022 at 10:29:36AM +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>
> > As we hardly have use cases/applications that need both cameras at the<br>
> > same time, this patch adds a rule that only one camera can be started<br>
> > one time. This also allows the following patches that use both imgus to<br>
> > process frames from one single camera.<br>
> <br>
> While I think this is right for the IPU3, this is the wrong reason. The<br>
> fact that Chrome OS has little use for capturing from the two cameras at<br>
> the same time doesn't meant there are no use cases on other platforms.<br>
></blockquote><div><br></div><div>Before I update the commit message, let's go through the issues below first :)</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>
> The reason why I think this makes sense is that the two ImgU instances<br>
> are actually not independent, both need to be configured before any gets<br>
> started. That conflicts with how the libcamera API operates. I would<br>
> like to not suffer from this limitation though, but that would require<br>
> broader changes in the public API, I think it's fine to limit operation<br>
> to one camera at a time for now.<br>
></blockquote><div><br></div><div>So Han-lin and I have never tested soraka (with IPU3) on using both cameras simultaneously. Do you mean that it never works, even if we assign one ImgU to each camera in PipelineHandlerIPU3?</div><div>Is the culprit here [1]?</div><div><br></div><div>[1]: <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n550">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n550</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"> <br>
> On a side note, when I select a camera in a video call in the Chrome<br>
> browser, I get a small preview of each available camera. Isn't this a<br>
> use case that is of interest to Chrome OS ?<br>
></blockquote><div><br></div><div>Which web app did you use? I didn't find this feature on Google Meet though...</div><div>I'd like to try how the current libcamera on soraka support this feature :)</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>
> Kieran told me that this works under Windows on a Surface Go 2.<br>
> Technically speaking, I don't know how we could implement it though, as<br>
> far as I understand, only one instance of the ImgU can operate in video<br>
> mode and guarantee real time operation, while the other instance needs<br>
> to operate in still image capture mode and will work in "batch" mode,<br>
> using the free processing time of the hardware that isn't reserved for<br>
> video mode.<br>
></blockquote><div><br></div><div>You're talking about the current situation of IPU3 ImgUs, right?</div><div>I thought that libcamera's PipelineHandlerIPU3 only uses the two ImgUs in video mode? Is the constraint mentioned somewhere in the code or documentation (in libcamera or the shipped HAL)?</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>
> > Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> > ---<br>
> > include/libcamera/internal/pipeline_handler.h | 5 ++++<br>
> > src/libcamera/camera.cpp | 11 ++++++++<br>
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 18 +++++++++++++<br>
> > src/libcamera/pipeline_handler.cpp | 26 +++++++++++++++++++<br>
> > 4 files changed, 60 insertions(+)<br>
> <br>
> This patch should be split in two, with one patch that extends the<br>
> Camera and PipelineHandler API, and a second patch to implement it in<br>
> the IPU3 pipeline handler.<br>
> <br>
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h<br>
> > index c3e4c258..8277ceda 100644<br>
> > --- a/include/libcamera/internal/pipeline_handler.h<br>
> > +++ b/include/libcamera/internal/pipeline_handler.h<br>
> > @@ -55,6 +55,9 @@ public:<br>
> > virtual int exportFrameBuffers(Camera *camera, Stream *stream,<br>
> > std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;<br>
> > <br>
> > + virtual bool acquire(Camera *camera);<br>
> > + virtual void release(Camera *camera);<br>
> > +<br>
> > virtual int start(Camera *camera, const ControlList *controls) = 0;<br>
> > void stop(Camera *camera);<br>
> > bool hasPendingRequests(const Camera *camera) const;<br>
> > @@ -76,6 +79,8 @@ protected:<br>
> > <br>
> > CameraManager *manager_;<br>
> > <br>
> > + std::set<Camera*> acquiredCameras_;<br>
> <br>
> Space between Camera and *.<br>
> <br>
> > +<br>
> > private:<br>
> > void mediaDeviceDisconnected(MediaDevice *media);<br>
> > virtual void disconnect();<br>
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp<br>
> > index 713543fd..a31f8769 100644<br>
> > --- a/src/libcamera/camera.cpp<br>
> > +++ b/src/libcamera/camera.cpp<br>
> > @@ -826,6 +826,8 @@ int Camera::exportFrameBuffers(Stream *stream,<br>
> > * \return 0 on success or a negative error code otherwise<br>
> > * \retval -ENODEV The camera has been disconnected from the system<br>
> > * \retval -EBUSY The camera is not free and can't be acquired by the caller<br>
> > + * \retval -EUSERS The maximum number of cameras that can be opened concurrently<br>
> > + * were opened already<br>
> <br>
> That's not how acquire() should work. The purpose of acquire() is to<br>
> reserve a camera for usage by the application, preventing other<br>
> applications that use libcamera from doing the same. It's a simple<br>
> locking mechanism really. With this change, it won't be possible for an<br>
> application to acquire the front and back cameras. This will cause<br>
> issues, for instance, when switching between the front and back cameras<br>
> in an application, if the application wants to acquire the other camera<br>
> before releasing the current one, to make sure that, if the acquire<br>
> fails, the current camera can still be used. It also breaks the use case<br>
> of acquiring multiple cameras to switch between them while preventing<br>
> other applications from using any.<br>
> <br>
> I'd like to see a design proposal for how the public API should evolve<br>
> to make mutual exclusion possible, while keeping use cases such as the<br>
> above supported (unless, of course, you show that those use cases<br>
> actually don't make sense, in which case we could reconsider them).<br>
<br>
Another point I forgot to mention, don't we need an API to let<br>
applications query mutual exclusion constraints between cameras ?<br>
<br></blockquote><div><br></div><div>I see. Sorry that Han-lin and I didn't fully understand how libcamera::Camera::acquire() works.</div><div><br></div><div>As currently the Android adapter calls libcamera::Camera::start() only when the first frame request comes, I don't think it's a proper code path that we tell the user/application about the mutual exclusion constraints (i.e. fail the start operation).</div><div>Therefore, I agree that a new API to let applications query mutual exclusion constraints makes more sense. In the Android API, we have |conflicting_devices| [2] and |resource_cost| [3]. Do you think it's appropriate to simply add the same API in libcamera?</div><div><br></div><div>[2]: <a href="https://source.android.com/devices/camera/versioning#24">https://source.android.com/devices/camera/versioning#24</a></div><div>[3]: <a href="https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/mojo/camera_common.mojom#30">https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/mojo/camera_common.mojom#30</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">
> > */<br>
> > int Camera::acquire()<br>
> > {<br>
> > @@ -845,6 +847,14 @@ int Camera::acquire()<br>
> > return -EBUSY;<br>
> > }<br>
> > <br>
> > + if (!d->pipe_->acquire(this)) {<br>
> > + LOG(Camera, Info)<br>
> > + << "The maximum number of cameras that can be opened"<br>
> > + "concurrently were opened already";<br>
> > + d->pipe_->unlock();<br>
> > + return -EUSERS;<br>
> > + }<br>
> > +<br>
> > d->setState(Private::CameraAcquired);<br>
> > <br>
> > return 0;<br>
> > @@ -873,6 +883,7 @@ int Camera::release()<br>
> > if (ret < 0)<br>
> > return ret == -EACCES ? -EBUSY : ret;<br>
> > <br>
> > + d->pipe_->release(this);<br>
> > d->pipe_->unlock();<br>
> > <br>
> > d->setState(Private::CameraAvailable);<br>
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> > index fd989e61..091a40e1 100644<br>
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> > @@ -141,6 +141,8 @@ public:<br>
> > int exportFrameBuffers(Camera *camera, Stream *stream,<br>
> > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;<br>
> > <br>
> > + bool acquire(Camera *camera) override;<br>
> > +<br>
> > int start(Camera *camera, const ControlList *controls) override;<br>
> > void stopDevice(Camera *camera) override;<br>
> > <br>
> > @@ -763,8 +765,24 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)<br>
> > return 0;<br>
> > }<br>
> > <br>
> > +bool PipelineHandlerIPU3::acquire(Camera *camera) {<br>
> <br>
> The curly brace should be on the next line.<br>
> <br>
> > + /*<br>
> > + * Enforce that only a single camera can be used at a time to use both<br>
> > + * ImgUs on the camera, so that StillCapture stream can adopt another<br>
> > + * set of configuration.<br>
> > + */<br>
> > + if (!acquiredCameras_.empty())<br>
> <br>
> Given that the set can only contain a single camera, you could replace<br>
> it with just a pointer.<br>
> <br>
> > + return false;<br>
> > +<br>
> > + return PipelineHandler::acquire(camera);<br>
> > +<br>
> > +}<br>
> > +<br>
> > int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)<br>
> > {<br>
> > + if (acquiredCameras_.empty() || *acquiredCameras_.begin() != camera)<br>
> > + return -EUSERS;<br>
> > +<br>
> > IPU3CameraData *data = cameraData(camera);<br>
> > CIO2Device *cio2 = &data->cio2_;<br>
> > ImgUDevice *imgu = data->imgu_;<br>
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp<br>
> > index 7ebd76ad..fff9ee59 100644<br>
> > --- a/src/libcamera/pipeline_handler.cpp<br>
> > +++ b/src/libcamera/pipeline_handler.cpp<br>
> > @@ -270,6 +270,32 @@ void PipelineHandler::unlock()<br>
> > * otherwise<br>
> > */<br>
> > <br>
> > +/**<br>
> > + * \fn PipelineHandler::acquire()<br>
> > + * \brief Check if \a camera can be acquired and supported with the current<br>
> > + * pipeline handler usage. If \a camera has already been acquired (by the same<br>
> > + * or another process), return false.<br>
> > + * \param[in] camera The camera<br>
> > + */<br>
> > +bool PipelineHandler::acquire(Camera *camera)<br>
> > +{<br>
> > + if (acquiredCameras_.find(camera) != acquiredCameras_.end())<br>
> > + return false;<br>
> > +<br>
> > + acquiredCameras_.emplace(camera);<br>
> > + return true;<br>
> > +}<br>
> > +<br>
> > +/**<br>
> > + * \fn PipelineHandler::release()<br>
> > + * \brief Release exclusive access to \a camera<br>
> > + * \param[in] camera The camera<br>
> > + */<br>
> > +void PipelineHandler::release(Camera *camera)<br>
> > +{<br>
> > + acquiredCameras_.erase(camera);<br>
> > +}<br>
> > +<br>
> > /**<br>
> > * \fn PipelineHandler::start()<br>
> > * \brief Start capturing from a group of streams<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br></blockquote><div><br></div><div>Thanks again! </div></div></div>