[vlc-devel] [PATCH 1/2] libvlc: deactivate the playlist before destroying the preparser

[Filip Roséen]

Hi Rémi,

I think my points will be easier explained once I have created the
tickets, and submitted a new implementation that follows the rules
associated with the tickets.

I also believe I need a few more cups of coffee in order to prevent my
fingers from writing things my mind does not fully agree with.

With that in mind, I will supply a more detailed explanation of my
reasoning at a later time. I apologize for the inconvenience.

---------------------------------------------------------------------

On 16/08/25 16:50, Rémi Denis-Courmont wrote:

> In other words, deactivating the playlist removes one source of preparser 
> events. Not the other ones (interfaces, LibVLC media players, VLM(?) 
> SDs(?)...). I still don´t understand the point.

Having things schedule preparsing entities after `libvlc_release` has
been called should be an error in the callee, not the underlying
implementation.

The difference with the playlist is that the playlist of course works
by requesting new entities to be worked with (in one way or another).

> Preparsing is serialized to avoid stressing system resources (CPU, memory, I/O 
> bandwith, network...). This is a feature, not a bug.

Yes, the preparsing itself should be serialized - but;

 - why should a request to preparse an entity block because some other
   entity is being preparsed?

Putting a letter in a mailbox should not be a blocking operation just
because the mailman is delivering letters to someone else.

> >   - there is really no reason for the preparser thread to be in
> >     *DETACH* state,
> 
> The point is to free the thread resources even though no other thread is 
> joining or likely to join any time soon. Though the extent of the "resources" 
> depends on the threading implementation.

Having it attached is an easier way to join on the thread when the
preparser is being deleted (without having to reinvent the wheel in
terms of joining).

  - http://git.videolan.org/?p=vlc.git;a=blob;f=src/playlist/preparser.c;h=8baf34519e4dfe2c15075caa51a34ac0bb929173;hb=HEAD#l173

I see your point, but I do not see if the added complexity is worth
it, especially not given that we will run into cases where we fire up
a thread, preparse an entity, let the thread die, and then fire up a
new one to preparse some other entity.

I have not run any benchmark on the matter, but I can hopefully
include such in future emails dealing with this issue.

> No. The preparser has no dependency on the playlist. It is a shared resource 
> by design. The playlist depends on the preparser, and the preparser should be 
> created before and destroyed after the playlist.
> 
> Where are the chicken and the egg?!

The preparser indirectly modifies the playlist, such as when
preparsing a directory (to add children), and when the playlist is in
"flat mode" (it will both delete and insert entities).

> The problem is the playlist leaving dangling callbacks on input items when it 
> terminates, which leads to use-after-free if any of the dangling callbacks 
> fires.
> 
> > [ ... ]
> 
> If the playlist deregistered its callbacks correctly (admittedly not exactly 
> easy), there would be no bug. The preparser would fire events after the 
> playlist is gone, and yet, there would be no use-after-free.
> 
> The problem is the playlist assuming it owns the input items, or more 
> specifically, when it does not. Or more specifically, input items can outlive 
> the playlist; the playlist wrongly assumes otherwise.

Agreed.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20160825/c76fe2f9/attachment.html>