[vlc-devel] [PATCH 4/6] fetcher: use vlc_executor_t

[Alexandre Janniaux]

Hi,

On Tue, Sep 08, 2020 at 01:18:09PM +0200, Thomas Guillem wrote:
>
>
> On Tue, Sep 8, 2020, at 12:36, Alexandre Janniaux wrote:
> > Hi,
> >
> > I like the end result very much, though the race condition
> > on Cancel is a bit inconvenient. I don't have any better way
> > for now anyway.
> >
> > Just a few comments below:
> >
> > On Mon, Sep 07, 2020 at 05:40:09PM +0200, Romain Vimont wrote:
> > > Replace the background workers by executors.
> > > ---
> > >  src/preparser/fetcher.c | 379 ++++++++++++++++++++++------------------
> > >  1 file changed, 207 insertions(+), 172 deletions(-)
> > >
> > > diff --git a/src/preparser/fetcher.c b/src/preparser/fetcher.c
> > > index 5bb4aa1cef..d237ded782 100644
> > > --- a/src/preparser/fetcher.c
> > > +++ b/src/preparser/fetcher.c
> > > @@ -31,43 +31,118 @@
> > >  #include <vlc_threads.h>
> > >  #include <vlc_memstream.h>
> > >  #include <vlc_meta_fetcher.h>
> > > +#include <vlc_executor.h>
> > >
> > >  #include "art.h"
> > >  #include "libvlc.h"
> > >  #include "fetcher.h"
> > >  #include "input/input_interface.h"
> > > -#include "misc/background_worker.h"
> > >  #include "misc/interrupt.h"
> > >
> > >  struct input_fetcher_t {
> > > -    struct background_worker* local;
> > > -    struct background_worker* network;
> > > -    struct background_worker* downloader;
> > > +    vlc_executor_t *executor_local;
> > > +    vlc_executor_t *executor_network;
> > > +    vlc_executor_t *executor_downloader;
> >
> > I'm ok with it, but we probably want to have a single executor
> > for all of them in the very end, don't we?
>
> No, you don't want to block local parsing because all threads from the network executor are waiting for I/O.

Indeed that's a bit annoying.

For the future:

Since the parsing/fetching is not done by the task itself, it
could probably just not wait in the executor?

The way I see it is that there are two competing asynchronous
runtime in play: input_thread and vlc_executor.

If we remove the wait in the vlc_executor and get notification
from the input_thread, we almost don't need the executor. We
probably want to keep the executor since it's more generic
and could be much easier to use in the end though.

If we remove the asynchronicity in the input thread and replace
the input thread by task submission, that's the case where we
have the issue you mentionned.

But in the end, if we succeed to have the async framework
everywhere, we could just have a poll/epoll/IOCP (a readiness
framework is easy to write with a completion one anyway) task
somewhere and notify the access when he can read, be it a network
or local access. It might even work across sandbox boundaries.

This can probably be deferred to a deadline far in the future
though so my remark is useless for this patchset.

Regards,
--
Alexandre Janniaux
Videolabs