Please keep the list Cc'd, so that there's a record of the discussion.
On Sonntag, 19. Dezember 2010, Einar Rünkaru wrote:
> On Sun, Dec 19, 2010 at 10:41 PM, Johannes Sixt <j...@kdbg.org> 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?
> >> Second patch fixes audio-video synchronization while using ALSA sound
> >> output.
> >
> > This second patch is identical to Monty's that we discussed here:
>
> Yes almost the same. I rememebered it, but temporarily lost from sight.
>
> > http://thread.gmane.org/gmane.comp.video.cinelerra-cv.general/12032/focus
> >=12042
> >
> > (it's my first comment in that message). I.e., it does not improve things
> > for me: Video is initially very jerky, and catches up with audio after a
> > few seconds. (At that time, synchronization *is* better than it used to
> > be, but it's not an improvement when video starts off jerky.)
>
> Bug, what the second patch fixed, is that delay got from ALSA did not
> got stored for use in synchonization and for proper
> syncronization I had to set Preferences/Audio offset to 0.56s. After
> change my clip is perfectly syncronized with offset 0. It is
> definitely a bug if a program tries to use out of reach variable.
> (this->delay = delay; is a no-op before patch - both sides refer to
> same variable).
Good point.
But I don't know what I should do about this now. For me, the patch is clearly
a step backwards. Are there others who can test the effects of this patch?
It's basically this change that makes a difference for me:
diff --git a/cinelerra/audioalsa.C b/cinelerra/audioalsa.C
index f356917..7321292 100644
--- a/cinelerra/audioalsa.C
+++ b/cinelerra/audioalsa.C
@@ -508,7 +508,7 @@ int AudioALSA::write_buffer(char *buffer, int size)
if(done)
{
timer_lock->lock("AudioALSA::write_buffer");
- this->delay = delay;
+ snd_pcm_delay(get_output(), &delay);
timer->update();
samples_written += samples;
timer_lock->unlock();
diff --git a/cinelerra/audioalsa.h b/cinelerra/audioalsa.h
index ff28ae9..479efa8 100644
--- a/cinelerra/audioalsa.h
+++ b/cinelerra/audioalsa.h
@@ -62,7 +62,7 @@ private:
snd_pcm_t *dsp_in, *dsp_out, *dsp_duplex;
int64_t samples_written;
Timer *timer;
- int delay;
+ snd_pcm_sframes_t delay;
Mutex *timer_lock;
int interrupted;
};
_______________________________________________
Cinelerra mailing list
Cinelerra@skolelinux.no
https://init.linpro.no/mailman/skolelinux.no/listinfo/cinelerra
