Skip to content
Snippets Groups Projects

Draft: Move preparser code to a module

Open Draft: Move preparser code to a module
chouquette/vlc:add_preparser_module into master
9 unresolved threads
Open Hugo Beauzée-Luyssen requested to merge chouquette/vlc:add_preparser_module into master
9 unresolved threads

This is a preliminary WIP for #25119, which moves preparsing in a dedicated module, in order to be able to implement a fork/exec based preparser later on.

Merge request reports

Members who can merge are allowed to add commits.

Merge request pipeline #131173 passed

Stage:

    Merge request pipeline passed for 32fd57c9

    Approval is optional
    Merge blocked: 4 checks failed
    Merge request must not be draft.
    Unresolved discussions must be resolved.
    Merge conflicts must be resolved.
    Merge request must be rebased, because a fast-forward merge is not possible.

    Merge details

    • The source branch is 17245 commits behind the target branch.
    • 4 commits will be added to master.
    • Source branch will be deleted.

    • Mentions issue #25119

    Activity

    Filter activity
    • Approvals
    • Assignees & reviewers
    • Comments (from bots)
    • Comments (from users)
    • Commits & branches
    • Edits
    • Labels
    • Lock status
    • Mentions
    • Merge request status
    • Tracking
    src/preparser/preparser.h → include/vlc_preparser.h
    29 input_item_t* item;
    30 input_item_meta_request_option_t options;
    31 const input_preparser_callbacks_t* cbs;
    32 void* data;
    33 int timeout;
    34 void* id;
    35 };
    36
    37 struct vlc_preparser
    38 {
    39 struct vlc_object_t obj;
    40
    41 module_t *module;
    42 void* sys;
    43
    44 int (*pf_preparse)(struct vlc_preparser*, struct vlc_preparser_task*);
    • Romain Vimont
      Romain Vimont @rom1v ·
      Developer

      I'm wondering: how should the caller create a vlc_preparser_task instance? Just a malloc() and fill the struct?

    • Hugo Beauzée-Luyssen
      Hugo Beauzée-Luyssen @chouquette ·
      Author Developer

      It's created by vlc_preparser_Preparse but now that you mention it, I agree that it's a weird API...

      We could turn the struct into an opaque one with a few getters but that seems a bit overkill.

      The core code needs to be able to fiddle with whatever is stored (notably to be able to know if art fetching is required) and the preparser modules definitely need those information to know what to do

    • Please register or sign in to reply
    • Alexandre Janniaux
      Alexandre Janniaux @alexandre-janniaux ·
      Developer

      Hi,

      How is the selection between the different preparser module be done? Is there an existing strategy for this ? Would it make sense to have the capability for the preparser on the preparser task instead of the preparser itself ?

    • Hugo Beauzée-Luyssen
      Hugo Beauzée-Luyssen @chouquette ·
      Author Developer

      Yes it would, I realized this was needed when I tried to use the fork based preparser, which started libvlc, which started the fork based preparser, which [...]

      Locally I have a variable to select a specific preparser and a shortcut to identify the various modules

    • Please register or sign in to reply
  • Thomas Guillem
    Thomas Guillem @tguillem started a thread on commit 807985e7
    • include/vlc_preparser.h 0 → 100644
    29 input_item_t* item;
    30 input_item_meta_request_option_t options;
    31 const input_preparser_callbacks_t* cbs;
    32 void* data;
    33 int timeout;
    34 void* id;
    35 };
    36
    37 struct vlc_preparser
    38 {
    39 struct vlc_object_t obj;
    40
    41 module_t *module;
    42 void* sys;
    43
    44 int (*pf_preparse)(struct vlc_preparser*, struct vlc_preparser_task*);
  • Thomas Guillem
    Thomas Guillem @tguillem started a thread on commit 807985e7
    • include/vlc_preparser.h 0 → 100644
    55 * The input item is retained until the preparsing is done or until the
    56 * preparser object is deleted.
    57 *
    58 * @param timeout maximum time allowed to preparse the item. If -1, the default
    59 * "preparse-timeout" option will be used as a timeout. If 0, it will wait
    60 * indefinitely. If > 0, the timeout will be used (in milliseconds).
    61 * @param id unique id provided by the caller. This is can be used to cancel
    62 * the request with input_preparser_Cancel()
    63 * @returns VLC_SUCCESS if the item was scheduled for preparsing, an error code
    64 * otherwise
    65 * If this returns an error, the on_preparse_ended will *not* be invoked
    66 */
    67 int vlc_preparser_Preparse(struct vlc_preparser*, input_item_t*,
    68 input_item_meta_request_option_t,
    69 const input_preparser_callbacks_t*,
    70 void*, int, void*);
  • Thomas Guillem
    Thomas Guillem @tguillem started a thread on commit 807985e7
    • include/vlc_preparser.h 0 → 100644
    47 };
    48
    49 struct vlc_preparser* vlc_preparser_New(vlc_object_t*);
    50 void vlc_preparser_Delete(struct vlc_preparser*);
    51
    52 /**
    53 * This function enqueues the provided item to be preparsed.
    54 *
    55 * The input item is retained until the preparsing is done or until the
    56 * preparser object is deleted.
    57 *
    58 * @param timeout maximum time allowed to preparse the item. If -1, the default
    59 * "preparse-timeout" option will be used as a timeout. If 0, it will wait
    60 * indefinitely. If > 0, the timeout will be used (in milliseconds).
    61 * @param id unique id provided by the caller. This is can be used to cancel
    62 * the request with input_preparser_Cancel()
  • Thomas Guillem
    Thomas Guillem @tguillem started a thread on commit 807985e7
    • include/vlc_preparser.h 0 → 100644
    35 };
    36
    37 struct vlc_preparser
    38 {
    39 struct vlc_object_t obj;
    40
    41 module_t *module;
    42 void* sys;
    43
    44 int (*pf_preparse)(struct vlc_preparser*, struct vlc_preparser_task*);
    45
    46 void (*pf_cancel)(struct vlc_preparser*, void*);
    47 };
    48
    49 struct vlc_preparser* vlc_preparser_New(vlc_object_t*);
    50 void vlc_preparser_Delete(struct vlc_preparser*);
  • Thomas Guillem
    Thomas Guillem @tguillem started a thread on commit 807985e7
    • src/preparser/preparser.c
    573 priv->fetcher = input_fetcher_New( VLC_OBJECT(p) );
    574 if( unlikely( !priv->fetcher ) )
    575 msg_Warn( p, "unable to create art fetcher" );
    576
    577 priv->default_timeout =
    578 VLC_TICK_FROM_MS(var_InheritInteger(parent, "preparse-timeout"));
    579 if (priv->default_timeout < 0)
    580 priv->default_timeout = 0;
    581
    582 return p;
    583 }
    584
    585 void vlc_preparser_Delete(struct vlc_preparser* p)
    586 {
    587 struct vlc_preparser_private* priv = preparser_priv(p);
    588 module_unneed(p, p->module);
    • Thomas Guillem
      Thomas Guillem @tguillem ·
      Maintainer

      For new modules, we generally don't store the p->module anymore (and don't call module_undeed) but call a close callback directly. Cf. vlc_display_operations as an example.

    • Please register or sign in to reply
  • Thomas Guillem
    Thomas Guillem @tguillem started a thread on commit 32fd57c9
    • src/preparser/preparser.c
    23 23 #endif
    • Steve Lhomme
      Steve Lhomme @robUx4 ·
      Developer

      I have nothing against moving the preparser to a module, but I don't think it's mandatory to do it to call the code via an exec/fork call.

      Also the use of modules is that you can implement different ones. But does it make sense to have different implementations of the preparser ?

    • Alexandre Janniaux
      Alexandre Janniaux @alexandre-janniaux ·
      Developer

      You cannot open process arbitrarily/the same way on every platforms, ie. some platforms won't support fork/exec, so the question is necessary.

    • Rémi Denis-Courmont
      Rémi Denis-Courmont @Courmisch ·
      Maintainer

      We already have a wrapper for fork/exec or spawn, and it's anyway a compile-time, not run-time, determination. Besides doing the "decision" via modules probing creates recursion problems (that Hugo already mentioned). I agree with Steve that modules seems pointless if not harmful here.

      Edited by Rémi Denis-Courmont
    • Alexandre Janniaux
      Alexandre Janniaux @alexandre-janniaux ·
      Developer

      I'm not sure that this is so obvious. If you run libvlc and fork/exec is available, but the executable are not, we probably want to handle this too. Likewise, it probably doesn't make sense to fork/exec in a sandbox context. That's not saying that we should go for the module entrypoint, but the question is not already sorted out.

    • Rémi Denis-Courmont
      Rémi Denis-Courmont @Courmisch ·
      Maintainer

      If fork() fails, you probably have more important problems to deal with than running the preparser. If exec() fails, then that's hardly different from the preparser process crashing, so the last thing you want is to try in-process and risk crashing the entire VLC instance.

      Edited by Rémi Denis-Courmont
    • Romain Vimont
      Romain Vimont @rom1v ·
      Developer

      But does it make sense to have different implementations of the preparser ?

      It might be interesting to have several implementations, for example:

      1. a local "in process" preparser;
      2. a preparser which uses a separate process and deserializes the results (which will be considered experimental at first); btw, this one could be implemented in another language where deserialization is more convenient than in C.
      Edited by Romain Vimont
    • Rémi Denis-Courmont
      Rémi Denis-Courmont @Courmisch ·
      Maintainer

      I'd rather have serialisation in known-safe generated C code (we already have that for JSON), than in any hand-written unsafe language.

      Note that we cannot have non-C code in core.

      Edited by Rémi Denis-Courmont
    • Please register or sign in to reply
  • Steve Lhomme changed milestone to %3.0.x maintenance

    changed milestone to %3.0.x maintenance

  • Steve Lhomme changed milestone to %4.0

    changed milestone to %4.0

    • Please register or sign in to reply

    VideoLAN code repository instance