[FFmpeg-devel] [PATCH] avformat/avisynth: remove atexit() handler

[FFmpeg-devel] [PATCH] avformat/avisynth: remove atexit() handler

[Stephen Hutchinson]

The atexit() handler in the avisynth demuxer was added because
there was a conflict in AvxSynth that arose due to their use
of C++ global objects, particularly in relation to having
added a logging function relying on log4cpp.

This conflict was responsible for causing a segfault on exit.
It did not affect Windows with the (at the time) upstream
AviSynth 2.5 and 2.6, nor does it affect AviSynth+.

Unfortunately, none of this was actually shielded by ifdefs
indicating the fact it was only needed for AvxSynth, so four
years ago when AviSynth+ replaced AvxSynth as the handler
for AviSynth scripts on Unix-like OSes, the fact that the
atexit handler was no longer necessary was overlooked.

Signed-off-by: Stephen Hutchinson <qyot27@gmail.com>
---
 libavformat/avisynth.c | 45 ------------------------------------------
 1 file changed, 45 deletions(-)

diff --git a/libavformat/avisynth.c b/libavformat/avisynth.c
index 625bdf7e3a..5d1ecc0bf6 100644
--- a/libavformat/avisynth.c
+++ b/libavformat/avisynth.c
@@ -115,9 +115,6 @@ typedef struct AviSynthContext {
     int error;
 
     uint32_t flags;
-
-    /* Linked list pointers. */
-    struct AviSynthContext *next;
 } AviSynthContext;
 
 static const int avs_planes_packed[1] = { 0 };
@@ -133,15 +130,7 @@ static const int avs_planes_rgba[4]   = { AVS_PLANAR_G, AVS_PLANAR_B,
 
 static AVMutex avisynth_mutex = AV_MUTEX_INITIALIZER;
 
-/* A conflict between C++ global objects, atexit, and dynamic loading requires
- * us to register our own atexit handler to prevent double freeing. */
 static AviSynthLibrary avs_library;
-static int avs_atexit_called        = 0;
-
-/* Linked list of AviSynthContexts. An atexit handler destroys this list. */
-static AviSynthContext *avs_ctx_list = NULL;
-
-static av_cold void avisynth_atexit_handler(void);
 
 static av_cold int avisynth_load_library(void)
 {
@@ -185,7 +174,6 @@ static av_cold int avisynth_load_library(void)
     LOAD_AVS_FUNC(avs_get_env_property, 1);
 #undef LOAD_AVS_FUNC
 
-    atexit(avisynth_atexit_handler);
     return 0;
 
 fail:
@@ -214,30 +202,11 @@ static av_cold int avisynth_context_create(AVFormatContext *s)
         }
     }
 
-    if (!avs_ctx_list) {
-        avs_ctx_list = avs;
-    } else {
-        avs->next    = avs_ctx_list;
-        avs_ctx_list = avs;
-    }
-
     return 0;
 }
 
 static av_cold void avisynth_context_destroy(AviSynthContext *avs)
 {
-    if (avs_atexit_called)
-        return;
-
-    if (avs == avs_ctx_list) {
-        avs_ctx_list = avs->next;
-    } else {
-        AviSynthContext *prev = avs_ctx_list;
-        while (prev->next != avs)
-            prev = prev->next;
-        prev->next = avs->next;
-    }
-
     if (avs->clip) {
         avs_library.avs_release_clip(avs->clip);
         avs->clip = NULL;
@@ -248,20 +217,6 @@ static av_cold void avisynth_context_destroy(AviSynthContext *avs)
     }
 }
 
-static av_cold void avisynth_atexit_handler(void)
-{
-    AviSynthContext *avs = avs_ctx_list;
-
-    while (avs) {
-        AviSynthContext *next = avs->next;
-        avisynth_context_destroy(avs);
-        avs = next;
-    }
-    dlclose(avs_library.library);
-
-    avs_atexit_called = 1;
-}
-
 /* Create AVStream from audio and video data. */
 static int avisynth_create_stream_video(AVFormatContext *s, AVStream *st)
 {
-- 
2.43.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/avisynth: remove atexit() handler

[Andreas Rheinhardt]

Stephen Hutchinson:
> The atexit() handler in the avisynth demuxer was added because
> there was a conflict in AvxSynth that arose due to their use
> of C++ global objects, particularly in relation to having
> added a logging function relying on log4cpp.
> 
> This conflict was responsible for causing a segfault on exit.
> It did not affect Windows with the (at the time) upstream
> AviSynth 2.5 and 2.6, nor does it affect AviSynth+.
> 
> Unfortunately, none of this was actually shielded by ifdefs
> indicating the fact it was only needed for AvxSynth, so four
> years ago when AviSynth+ replaced AvxSynth as the handler
> for AviSynth scripts on Unix-like OSes, the fact that the
> atexit handler was no longer necessary was overlooked.
> 
> Signed-off-by: Stephen Hutchinson <qyot27@gmail.com>
> ---
>  libavformat/avisynth.c | 45 ------------------------------------------
>  1 file changed, 45 deletions(-)
> 
> diff --git a/libavformat/avisynth.c b/libavformat/avisynth.c
> index 625bdf7e3a..5d1ecc0bf6 100644
> --- a/libavformat/avisynth.c
> +++ b/libavformat/avisynth.c
> @@ -115,9 +115,6 @@ typedef struct AviSynthContext {
>      int error;
>  
>      uint32_t flags;
> -
> -    /* Linked list pointers. */
> -    struct AviSynthContext *next;
>  } AviSynthContext;
>  
>  static const int avs_planes_packed[1] = { 0 };
> @@ -133,15 +130,7 @@ static const int avs_planes_rgba[4]   = { AVS_PLANAR_G, AVS_PLANAR_B,
>  
>  static AVMutex avisynth_mutex = AV_MUTEX_INITIALIZER;
>  
> -/* A conflict between C++ global objects, atexit, and dynamic loading requires
> - * us to register our own atexit handler to prevent double freeing. */
>  static AviSynthLibrary avs_library;
> -static int avs_atexit_called        = 0;
> -
> -/* Linked list of AviSynthContexts. An atexit handler destroys this list. */
> -static AviSynthContext *avs_ctx_list = NULL;
> -
> -static av_cold void avisynth_atexit_handler(void);
>  
>  static av_cold int avisynth_load_library(void)
>  {
> @@ -185,7 +174,6 @@ static av_cold int avisynth_load_library(void)
>      LOAD_AVS_FUNC(avs_get_env_property, 1);
>  #undef LOAD_AVS_FUNC
>  
> -    atexit(avisynth_atexit_handler);
>      return 0;
>  
>  fail:
> @@ -214,30 +202,11 @@ static av_cold int avisynth_context_create(AVFormatContext *s)
>          }
>      }
>  
> -    if (!avs_ctx_list) {
> -        avs_ctx_list = avs;
> -    } else {
> -        avs->next    = avs_ctx_list;
> -        avs_ctx_list = avs;
> -    }
> -
>      return 0;
>  }
>  
>  static av_cold void avisynth_context_destroy(AviSynthContext *avs)
>  {
> -    if (avs_atexit_called)
> -        return;
> -
> -    if (avs == avs_ctx_list) {
> -        avs_ctx_list = avs->next;
> -    } else {
> -        AviSynthContext *prev = avs_ctx_list;
> -        while (prev->next != avs)
> -            prev = prev->next;
> -        prev->next = avs->next;
> -    }
> -
>      if (avs->clip) {
>          avs_library.avs_release_clip(avs->clip);
>          avs->clip = NULL;
> @@ -248,20 +217,6 @@ static av_cold void avisynth_context_destroy(AviSynthContext *avs)
>      }
>  }
>  
> -static av_cold void avisynth_atexit_handler(void)
> -{
> -    AviSynthContext *avs = avs_ctx_list;
> -
> -    while (avs) {
> -        AviSynthContext *next = avs->next;
> -        avisynth_context_destroy(avs);
> -        avs = next;
> -    }
> -    dlclose(avs_library.library);
> -
> -    avs_atexit_called = 1;
> -}
> -
>  /* Create AVStream from audio and video data. */
>  static int avisynth_create_stream_video(AVFormatContext *s, AVStream *st)
>  {

avisynth_context_destroy() is currently always called while holding the
lock (i.e. avisynth_mutex). Is this even necessary? It is clear that
avisynth_load_library() (and the check for whether it should be called)
need the lock, but does anything else (like
avs_create_script_environment) really need it?

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/avisynth: remove atexit() handler

[Stephen Hutchinson]

On 7/7/24 9:50 AM, Andreas Rheinhardt wrote:
> 
> avisynth_context_destroy() is currently always called while holding the
> lock (i.e. avisynth_mutex). Is this even necessary? It is clear that
> avisynth_load_library() (and the check for whether it should be called)
> need the lock, but does anything else (like
> avs_create_script_environment) really need it?
> 

Threading control is a topic that definitely goes over my head, so
my honest answer would be that I don't know.  But what I would say is
that if I correctly understand the order of execution inside the
demuxer, library, and script environment, AviSynth loading its own
plugins occurs after avs_create_script_environment is called in the
client program.

If avs_load_library and its check need the lock in place,
is it supposed to be assumed that then AviSynth handles
loading/unloading its plugins purely on its own without the
overarching lock, or is that cascaded loading something that
needs the lock in place throughout the process (because
avisynth_context_destroy is what ultimately calls
avs_delete_script_environment and unloads whatever plugins were
loaded by AviSynth)?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2] avformat/avisynth: remove mutex lock from read_close

[Stephen Hutchinson]

Signed-off-by: Stephen Hutchinson <qyot27@gmail.com>
---
After asking about this and testing with AviSynth+ 3.7.3 and
AviSynth 2.6, it would appear that this isn't necessary.
If it was to handle an edge case and said case reappears,
it can be re-evaluated then.

 libavformat/avisynth.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/libavformat/avisynth.c b/libavformat/avisynth.c
index 5d1ecc0bf6..e586fd8ce3 100644
--- a/libavformat/avisynth.c
+++ b/libavformat/avisynth.c
@@ -1085,11 +1085,7 @@ static int avisynth_read_packet(AVFormatContext *s, AVPacket *pkt)
 
 static av_cold int avisynth_read_close(AVFormatContext *s)
 {
-    if (ff_mutex_lock(&avisynth_mutex))
-        return AVERROR_UNKNOWN;
-
     avisynth_context_destroy(s->priv_data);
-    ff_mutex_unlock(&avisynth_mutex);
     return 0;
 }
 
-- 
2.43.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/avisynth: remove atexit() handler

[Stephen Hutchinson]

On 7/7/24 1:46 PM, Stephen Hutchinson wrote:
> On 7/7/24 9:50 AM, Andreas Rheinhardt wrote:
>>
>> avisynth_context_destroy() is currently always called while holding the
>> lock (i.e. avisynth_mutex). Is this even necessary? It is clear that
>> avisynth_load_library() (and the check for whether it should be called)
>> need the lock, but does anything else (like
>> avs_create_script_environment) really need it?
>>
> 
> Threading control is a topic that definitely goes over my head, so
> my honest answer would be that I don't know.  But what I would say is
> that if I correctly understand the order of execution inside the
> demuxer, library, and script environment, AviSynth loading its own
> plugins occurs after avs_create_script_environment is called in the
> client program.
> 
> If avs_load_library and its check need the lock in place,
> is it supposed to be assumed that then AviSynth handles
> loading/unloading its plugins purely on its own without the
> overarching lock, or is that cascaded loading something that
> needs the lock in place throughout the process (because
> avisynth_context_destroy is what ultimately calls
> avs_delete_script_environment and unloads whatever plugins were
> loaded by AviSynth)?

I've sent a second patch to remove the lock requirement in
avisynth_read_close.

The atexit patch was separate anyway, and if there's no
objections raised, I'll push that one in a day or two,
or wait a little longer and push both at the same time
if the lock change is fine.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".