Message ID | 20240707032511.2714-1-qyot27@gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] avformat/avisynth: remove atexit() handler | expand |
Context | Check | Description |
---|---|---|
yinshiyou/make_loongarch64 | success | Make finished |
yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
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
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)?
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.
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) {
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(-)