[libcamera-devel] [PATCH 3/3] Documentation: Update the "Start an event loop" section

[Jacopo Mondi]

Hi Laurent

On Fri, Jun 17, 2022 at 02:39:59PM +0300, Laurent Pinchart wrote:
> Hello,
>
> On Fri, Jun 17, 2022 at 11:39:45AM +0200, Jacopo Mondi via libcamera-devel wrote:
> > On Fri, Jun 17, 2022 at 11:28:19AM +0200, Daniel Semkowicz wrote:
> > > The description sounds clear. I would just add minor changes.
> > >
> > > On Fri, Jun 17, 2022 at 10:01 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > > On Fri, Jun 17, 2022 at 09:24:50AM +0200, Daniel Semkowicz wrote:
> > > > > On Thu, Jun 16, 2022 at 1:06 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > > > >
> > > > > > Hi Daniel,
> > > > > >
> > > > > >    I wonder if we shouldn't just drop the whole
> > > > > >
> > > > > > Start an event loop
> > > > > > ~~~~~~~~~~~~~~~~~~~
> > > > > >
> > > > > > paragraph
> > > > >
> > > > > I was also thinking about that when I was editing the current text.
> > > > > It is always the decision on how many details about the internal
> > > > > architecture We want to present to the developer in this guide.
> > > > > As it is the introduction guide then maybe indeed it will be better to
> > > > > keep it simple and do not talk too much about the internals.
> > > > >
> > > > > There will still be this one "sleep_for(3000ms)" line needed and I think
> > > > > it would be good to leave some short explanation why it is here. Do you
> > > > > think that adding it at the end of "Request queueing" section is a good
> > > > > idea or should I create a separate section about it?
> > > >
> > > > You're right, throwing a sleep_for() without saying anything might be
> > > > not the best idea :)
> > > >
> > > > Let's resume your original proposal and let me try to integrate it
> > > > with a full rewrite of the paragraph.
> > > >
> > > > > > > -Start an event loop
> > > > > > > +Event loop
> > > > > > >  ~~~~~~~~~~~~~~~~~~~
> > > > > > >
> > > > > > >  The libcamera library needs an event loop to monitor and dispatch events
> > > > > > > @@ -524,17 +526,17 @@ a notifier it is monitoring, it emits the notifier's
> > > > > > >  notifiers' signals and emit application visible events, such as the
> > > > > > >  ``Camera::bufferReady`` and ``Camera::requestCompleted`` signals.
> > > > > > >
> > > > > > > -The code below retrieves a reference to the system-wide event dispatcher and for
> > > > > > > -the a fixed duration of 3 seconds, processes all the events detected in the
> > > > > > > -system.
> > > > > > > +Event loop is handled internally by ``CameraManager`` instance in a separate
> > > > > > > +thread. ``CameraManager::start()`` creates a new thread and starts the event
> > > > > > > +loop processing.
> > > >
> > > > Event loop
> > > > ~~~~~~~~~~~~~~~~~~~
>
> Nitpicking, the underline should be shortened.
>
> > > >
> > > > The libcamera library needs an event loop to monitor and dispatch
> > > > events generated by the video devices part of the capture pipeline.
> > > > libcamera runs its own event loop in a separate thread, created at
> > > > ``CameraManager::start()_`` time. This means applications will run in
> > > > their own thread and need to manage their own execution opportunely,
> > > > to respond to user generate events and to libcamera generated events
> > > > emitted through signals.
> > >
> > > typo: "user generate" -> "user generated"
>
> Or even "user-generated" I think.
>
> I'd write this a bit differently to avoid mentioning the internal event
> loop as such. How about the following ?
>
> libcamera creates an internal execution thread at '`CameraManager::start()_``
> time to decouple its own event processing from the application's main thread.
> Applications are thus free to manage their own execution opportunely, and only
> need to respond to events generated by libcamera emitted through signals.
>
> Real-world applications will likely either integrate with the event loop of the
> framework they use, or create their own event loop to respond to user events.
> For the simple application presented in this example, it is enough to prevent
> immediate termination by pausing for 3 seconds. During that time, the libcamera
> thread will generate request completion events that the application will handle
> in the ``requestComplete()`` slot connected to the ``Camera::requestCompleted``
> signal.
>

Good, works for me!

> > > > Real-world applications will likely either integrate in the event loop
> > > > of higher level frameworks or create their own one to respond to user
> > > > generated events, while for the simple application presented in this
> > > > example it is enough to prevent immediate termination by pausing for 3
> > > > seconds, during which the asynchronous request completion events
> > > > generated by libcamera will be handled by the ``requestComplete()``
> > > > slot connected to the ``Camera::requestCompleted`` signal.
> > >
> > > I would break down this part into less complex sentences to improve
> > > readability. Something like:
> > >
> > > Real-world applications will likely either integrate in the event loop
> > > of higher level frameworks or create their own one to respond to user
> > > generated events. For the simple application presented in this example,
> > > it is enough to prevent immediate termination by pausing for 3 seconds.
> > > During this time, the asynchronous request completion events generated
> > > by libcamera will be handled by the ``requestComplete()`` slot connected
> > > to the ``Camera::requestCompleted`` signal.
> > >
> > > Is it ok?
> >
> > Thanks, much better!
> >
> > > > .. code:: cpp
> > > >
> > > >    std::this_thread::sleep_for(3000ms);
> > > >
> > > > What do you think ?
> > > >
> > > > > > > +
> > > > > > > +As the ``CameraManager`` was already started in our example, we need to prevent
> > > > > > > +the immediate termination of the application. The code below pauses the main
> > > > > > > +thread for 3 seconds, so that the event loop thread can process the requests.
> > > > > > >
> > > > > > >  .. code:: cpp
> > > > > > >
> > > > > > > -   EventDispatcher *dispatcher = cm->eventDispatcher();
> > > > > > > -   Timer timer;
> > > > > > > -   timer.start(3000);
> > > > > > > -   while (timer.isRunning())
> > > > > > > -       dispatcher->processEvents();
> > > > > > > +   std::this_thread::sleep_for(3000ms);
> > > > > > >
> > > > > > >  Clean up and stop the application
> > > > > > >  ---------------------------------
>
> --
> Regards,
>
> Laurent Pinchart