On Wed, Dec 22, 2010 at 11:00 PM, Johannes Sixt <[email protected]> wrote: > On Dienstag, 21. Dezember 2010, Einar Rünkaru wrote: >> On Mon, Dec 20, 2010 at 11:39 PM, Johannes Sixt <[email protected]> wrote: >> > On Sonntag, 19. Dezember 2010, Einar Rünkaru wrote: >> >> On Sun, Dec 19, 2010 at 10:41 PM, Johannes Sixt <[email protected]> wrote: >> >> > On Sonntag, 19. Dezember 2010, Einar Rünkaru wrote: >> >> >> diff --git a/cinelerra/pluginclient.h b/cinelerra/pluginclient.h >> >> >> index f1a340b..2b20e66 100644 >> >> >> --- a/cinelerra/pluginclient.h >> >> >> +++ b/cinelerra/pluginclient.h >> >> >> @@ -112,6 +112,7 @@ void thread_class::run() \ >> >> >> int result = window->run_window(); \ >> >> >> /* This is needed when the GUI is closed from itself */ \ >> >> >> if(result) plugin->client_side_close(); \ >> >> >> + plugin->thread = 0; \ >> >> >> } >> >> > >> >> > But PLUGIN_DESTRUCTOR_MACRO says this: >> >> > >> >> > #define PLUGIN_DESTRUCTOR_MACRO \ >> >> > if(thread) \ >> >> > { \ >> >> > /* This is needed when the GUI is closed from elsewhere than itself */ >> >> > \ /* Since we now use autodelete, this is all that has to be done, >> >> > thread will take care of itself ... */ \ >> >> > /* Thread join will wait if this was not called from the thread itself >> >> > or go on if it was */ \ >> >> > thread->window->lock_window("PLUGIN_DESTRUCTOR_MACRO"); >> >> > \ thread->window->set_done(0); \ >> >> > thread->window->unlock_window(); \ >> >> > thread->join(); \ >> >> > ... >> >> > >> >> > How does this make sense when you have to set plugin->thread to NULL >> >> > manually? I mean, even when the window is "closed from elsewhere", >> >> > run_window() will return eventually, and only after that can the >> >> > desctructor run (so I hope, otherwise we are in bigger trouble). And >> >> > in this case, the destructor expects thread to be non-NULL. It would >> >> > not be the case anymore with your patch. >> >> > >> >> > I haven't analyzed what's going on, but am only reasoning from what I >> >> > see in the code fragment and comments. I must be missing something. >> >> > Please help. >> >> >> >> Scenario (names from macro): >> >> 1. Plugin creates thread_class and starts it. thread_class::run() >> >> writes pointer to itself >> >> into plugin. >> >> 2. thread_class:run() returns, immediately after that thread_class >> >> object will be destroyed and memory freed. >> >> 3. Freed memory will be used and written over by another obect. >> >> 4. Plugin destructor uses thread ptr to access already destroyed >> >> object, but this ptr points to nowhere (if step 3 happened). >> >> >> >> It's definitely a bug if you try to reference to already destroyed >> >> object. plugin->thread in thread_class and thread in >> >> PLUGIN_DESTRUCTOR_MACRO are the same and point to thread_class object >> >> whitch may be already destroyed when plugin destructor runs. It is >> >> hard to understand, but I was able to create quite reliable crash >> >> situation at that point. >> > >> > But why don't you delete the reference to thread from >> > PLUGIN_DESTRUCTOR_MACRO? Isn't it worthless because it is alway NULL? >> > >> > And if you do remove it, how is the "closed from elsewhere" situation >> > taken care of? >> > >> > Did you understand what the point of commit d5ff675ddb is that touched >> > PLUGIN_DESTRUCTOR_MACRO? >> >> Remember that thread_class created with synchronous=0 and >> autodelete=1, this means that thread exits >> immediately afrer returning from run and deletes thread_class object. >> There is no need for thread_join (but it does not harm). >> PLUGIN_DESTRUCTOR_MACRO must not use thread ptr after thread_class has >> returned (look guicast/thread.C) >> >> "closed from elsewere" means, that plugin destructor was called while >> thread_class is running. In this case plugin tells to thread_class to >> close window and return from run (and delete itself). >> >> There is no problem if thread_class is not created - thread ptr is >> null and destructor works as expected. Thread ptr is set in >> thread_class::run to indicate when thread_class is ready and it is >> logical to clear it while thread_class prepares to exit. >> >> In the Ui the thread_class is plugin's configuration window. And user >> can remove plugin - plugin destructor must destroy >> plugin's configuration window - or user can simply close configuration >> window - then plugin destructor must not use thread ptr - thread >> object is immediately deleted after close of the window (autodelete >> means this). >> >> I am thinking about removing the autodelete feature, because of hard >> to understand and debug side effects. The only place, where autodelete >> is used is PLUGIN_THREAD_OBJECT macro. But before all plugins must be >> checked - may be some of them makes special use of the feature. > > I observed this in the debugger. I was only able to trigger the case where the > destructor runs first while thread is still non-NULL. How do I trigger the > case where run() terminates before the destructor investigates if (thread != > NULL)? > I investigated this case a bit more. Things are more complex as I first thought. I din't find execution path where the line 'plugin->thread = 0' is important. Closing plugin's configuration destroys plugin and this takes if(thread).. path and this closes the configuration window.
Bad news is that, if there is context switch in set_done, it is possible that because of autodelete freature the thread destroys the thread object and next line refers unexsisting object - memory area that can be overwritten by another object. The only solution is to remove all references to thread below set_done. It is possible: set_done does not need locking, and asynchronous thread does not need join(). I get occasional crashes exactly at the moment of plugin destruction during playback and I don't understand why - plugins configuration window is not used, but discussed change fixed it. Today I got crash again, and removing join, based on analysis above, fixed it. I dont like the self-destruction idea.. >> >> >> Second patch fixes audio-video synchronization while using ALSA sound >> >> >> output. >> ... >> I looked at jerkyness problem... > > Do I understand correctly that you keep the issue on your radar? Yes. I am trying to get rid of framerate and samplerate in the core of cinelerra and use pts time instead of it. So I have to look at all related places. I reached to audio playback and synchronization. Einar _______________________________________________ Cinelerra mailing list [email protected] https://init.linpro.no/mailman/skolelinux.no/listinfo/cinelerra
