On Mon, Dec 20, 2010 at 11:39 PM, Johannes Sixt <j...@kdbg.org> wrote:
> Please keep the list Cc'd, so that there's a record of the discussion.
Sorry, my fault.
> 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?
>
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.
>> >> 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;
> };
>
Delay here is a number of samples, not yet played, residing currently
in ALSA subsystem. The amount of samples depends on hadware, drivers,
etc. This number must be taken into account in synchronization
calculations. I think that some of the complaints about bad
audio/video synchronization are caused by this bug.
I looked at jerkyness problem. When user hits play, cinelerra displays
th first frame and starts audio, gets timing info based on audio from
synchronization clock. The problem is in synchronization clock which
runs on wall clock until audio starts. Starting of audio may take lot
of time - opening device, decoding first packets of audio. In my test
case it took 4-5 frames. After audio is started and first buffers of
audio samples are written to device, syncronization clock is reset to
absolute audio position (eg near 0). Video is stopped until audio
reaches to current video position. Cursor on timeline jumps back and
moves sychronously with audio.
Current fix fixes synchronizaton after the first jumping is done, but
exposes better the jerkyness. At the moment I have no fix for this
problem. Somehow syncronization clock must be on hold until audio
starts, but case then there is no audio must be handled too. Also
there are cases related to preferences/audio offset and late audio.
Einar
_______________________________________________
Cinelerra mailing list
Cinelerra@skolelinux.no
https://init.linpro.no/mailman/skolelinux.no/listinfo/cinelerra
