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

16225 diff mbox series

Event loop was moved to be a part of CameraManager, so it is no longer
a user responsibility to control the event dispatching.
---
 .../guides/application-developer.rst          | 20 ++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Hi Daniel,

   I wonder if we shouldn't just drop the whole

Start an event loop
~~~~~~~~~~~~~~~~~~~

paragraph

On Wed, Jun 15, 2022 at 06:26:01PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> Event loop was moved to be a part of CameraManager, so it is no longer
> a user responsibility to control the event dispatching.
> ---
>  .../guides/application-developer.rst          | 20 ++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
> index 00bafb10..97c032a5 100644
> --- a/Documentation/guides/application-developer.rst
> +++ b/Documentation/guides/application-developer.rst
> @@ -27,10 +27,12 @@ defined names and types without the need of prefixing them.
>     #include <iomanip>
>     #include <iostream>
>     #include <memory>
> +   #include <thread>
>
>     #include <libcamera/libcamera.h>
>
>     using namespace libcamera;
> +   using namespace std::chrono_literals;
>
>     int main()
>     {
> @@ -506,7 +508,7 @@ and queue all the previously created requests.
>     for (std::unique_ptr<Request> &request : requests)
>         camera->queueRequest(request.get());
>
> -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.
> +
> +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
>  ---------------------------------
> --
> 2.34.1
>

Hi Jacopo,

On Thu, Jun 16, 2022 at 1:06 PM Jacopo Mondi <jacopo@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?

Best regards
Daniel


> On Wed, Jun 15, 2022 at 06:26:01PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > Event loop was moved to be a part of CameraManager, so it is no longer
> > a user responsibility to control the event dispatching.
> > ---
> >  .../guides/application-developer.rst          | 20 ++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
> > index 00bafb10..97c032a5 100644
> > --- a/Documentation/guides/application-developer.rst
> > +++ b/Documentation/guides/application-developer.rst
> > @@ -27,10 +27,12 @@ defined names and types without the need of prefixing them.
> >     #include <iomanip>
> >     #include <iostream>
> >     #include <memory>
> > +   #include <thread>
> >
> >     #include <libcamera/libcamera.h>
> >
> >     using namespace libcamera;
> > +   using namespace std::chrono_literals;
> >
> >     int main()
> >     {
> > @@ -506,7 +508,7 @@ and queue all the previously created requests.
> >     for (std::unique_ptr<Request> &request : requests)
> >         camera->queueRequest(request.get());
> >
> > -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.
> > +
> > +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
> >  ---------------------------------
> > --
> > 2.34.1
> >

Hi Daniel,

On Fri, Jun 17, 2022 at 09:24:50AM +0200, Daniel Semkowicz wrote:
> Hi Jacopo,
>
> On Thu, Jun 16, 2022 at 1:06 PM Jacopo Mondi <jacopo@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
~~~~~~~~~~~~~~~~~~~

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.

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.

.. 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
> > >  ---------------------------------
> > > --
> > > 2.34.1
> > >

The description sounds clear. I would just add minor changes.

On Fri, Jun 17, 2022 at 10:01 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Daniel,
>
> On Fri, Jun 17, 2022 at 09:24:50AM +0200, Daniel Semkowicz wrote:
> > Hi Jacopo,
> >
> > On Thu, Jun 16, 2022 at 1:06 PM Jacopo Mondi <jacopo@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
> ~~~~~~~~~~~~~~~~~~~
>
> 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"

>
> 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?

Best regards
Daniel

>
> .. 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
> > > >  ---------------------------------
> > > > --
> > > > 2.34.1
> > > >

Hi Daniel,

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@jmondi.org> wrote:
> >
> > Hi Daniel,
> >
> > On Fri, Jun 17, 2022 at 09:24:50AM +0200, Daniel Semkowicz wrote:
> > > Hi Jacopo,
> > >
> > > On Thu, Jun 16, 2022 at 1:06 PM Jacopo Mondi <jacopo@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
> > ~~~~~~~~~~~~~~~~~~~
> >
> > 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"
>
> >
> > 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!

Thanks
   j

>
> Best regards
> Daniel
>
> >
> > .. 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
> > > > >  ---------------------------------
> > > > > --
> > > > > 2.34.1
> > > > >

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@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@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.

> > > 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
> > > > > >  ---------------------------------

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@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@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

Hi Laurent,

On Fri, Jun 17, 2022 at 2:23 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> 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@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@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.
> >

Then do We want to leave section name as "Event loop" or also change
it to something more general?

Best regards

Daniel


>
> 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

Hi Daniel,

On Fri, Jun 17, 2022 at 03:53:41PM +0200, Daniel Semkowicz wrote:
> On Fri, Jun 17, 2022 at 2:23 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > On Fri, Jun 17, 2022 at 02:39:59PM +0300, Laurent Pinchart wrote:
> > > 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@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@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.
> > >
> 
> Then do We want to leave section name as "Event loop" or also change
> it to something more general?

Good point. It was initially about the libcamera event loop. We could
keep the name as-is, just shifting the meaning to refer to the
application event loop, but we could also rename it to "Event handling"
or something similar. Up to you.

> > 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
> > > > > > > > >  ---------------------------------