[vlc-devel] Using logger during asynchronous close

[Alexandre Janniaux]

Hi,

On Mon, Mar 08, 2021 at 06:45:32PM +0200, Rémi Denis-Courmont wrote:
> Le lundi 8 mars 2021, 17:56:54 EET Alexandre Janniaux a écrit :
> > Hi,
> >
> > After merging my last mediacodec patches, I encountered segmentation
> > faults that unfortunately never occurred previously but were bound to
> > happen.
> >
> > Indeed, after those patches, the mediacodec decoder_t object can be
> > closed while the mediacodec core is still valid as long as the video
> > context is valid. If the mediacodec core is logging message, it will
> > go through the closed and potentially destroyed decoder_t object.
> >
> > I'm looking for solution for this problem but I'm not sure we already
> > encountered this issue so I'm not sure how to solve it.
> >
> > So far, I have those solutions:
> >
> >  - adding back refcount on vlc_object_t, which would probably be
> >    horrible.
>
> That wouldn't just be horrible. It wouldn't work. It only guaranteed that
> object variables were still usable, which is not what you would need.
>
> >  - adding refcount to logger object. We basically know that the current
> >    logger is always available but it might change in the future so
> >    having a way to ensure it's validity in a better way than having a
> >    way to ensure the validity of the underlying object.
>
> That wouldn't work either. You can't simply slap a reference count on
> something that does not have one. You'd merely move the problem from
> preserving the logger structure to preserving the underlying resources of the
> logger backend, and break the assumptions on the life cycle of those
> resources.

Indeed, though that wouldn't happen with the logger we currently use.

> >  - creating a dedicated object to log onto from libvlc, but then I lose
> >    the benefit of the logger inheritency infrastructure.
> >
> >  - likewise, using the logger from the libvlc instance, but same problem.
> >
> >  - using the logger from the object, but it could disappear too.
>
> You can't have it both ways. You can only log to a backend whose life cycle
> encompasses the source of the log messages.

Yes sure, hence the ideas gravitating around the creation of a new
thing (object, logger) to encompass the asynchronous part.

> Either you fix the design not to perform in(s)ane things, or you devise a weak
> reference mechanism. The later means that log messages will sometime get lost,
> which is far from good.

Just so this is clear, what is the «insane/inane» thing? If that's about
fixing mediacodec design, I can't really do that, though the flaws requiring
this are fixed for approximately 60% of android phones. Before API 26, if
you close the decoder, then the pictures sent by the decoder are not valid
anymore. There's no access to a lower level public not-dumb interface that
matches the concept picture_t so most of it is emulated through a higher
level interface which, as usual from Android, makes things incredibly more
complex.

The latter is what I currently have implemented in my branch, but that's quite
not satisfactory. However asynchronously closing the resources allocated from
the decoder should/has become quite common in the code base, which means that
those regions -- which might not have log there currently so are basically
using the latter solution -- are unable to do logging. The solution I gave
above all look bad because they are trying to solve a more general problem but
maybe we can find a better solution just for the case of the video pipeline?

Thank you for the precious insight,

Regards,
--
Alexandre Janniaux
Videolabs