Tue 22 Feb 2011 09:54:25 PM UTC, comment #3:
For the record, the issue was that while we do properly lock the container of A/V frames, we have no mechanism to ensure that the single frame fetched from the container is kept alive while used by a thread.
In particular, this specific case has a thread peeking first
frame and another pulling it off (obtaining ownership).
By the time the first thread attempts to dereference the peeked
frame, the other thread deleted it already.
Bastiaan patch would keep the queue locked for the whole (short) time of both peeking and querying the frame for timestamp.
Being a short time it'd be fine by me. Only concern is that it
drops locking from another function, which would require going trough all callers to be sure about what the consequences could be.
A safer approach might be to leave the existing lockings in place and just add longer locking in nextFrameTimestamp avoiding the call to peekNextFrame (or extend the peekNextFrame interface to allow specifying whether or not one wants locking, or a new function or something along those lines).
Also, for symmetricity, the same behaviour should be made for Audio and Video.
|
Fri 18 Feb 2011 09:24:54 PM UTC, original submission:
STR: valgrind gui/.libs/lt-gtk-gnash -M gst http://download.omroep.nl/vpro/luisterpaal/new-luisterpaal/player.swf?list=/feeds/luisterpaalPlaylist?luisterpaalnr=44452648 -U http://3voor12.vpro.nl/speler/luisterpaal/44452648#luisterpaal.44452648
Press the 'next item' button.
==11849== Thread 1:
==11849== Invalid read of size 4
==11849== at 0x4862574: gnash::media::MediaParser::nextAudioFrameTimestamp(unsigned long long&) const (MediaParser.cpp:226)
==11849== by 0x436D247: gnash::Sound_as::getPosition() const (Sound_as.cpp:752)
==11849== by 0x436BE1F: gnash::Sound_as::update() (Sound_as.cpp:341)
==11849== by 0x4277DC1: gnash::(anonymous namespace)::ExecuteCallback::operator()(gnash::as_object const*) const (movie_root.cpp:108)
==11849== by 0x428113D: gnash::(anonymous namespace)::ExecuteCallback std::for_each<__gnu_cxx::__normal_iterator<gnash::as_object*, std::vector<gnash::as_object, std::allocator<gnash::as_object> > >, gnash::(anonymous namespace)::ExecuteCallback>(__gnu_cxx::__normal_iterator<gnash::as_object, std::vector<gnash::as_object, std::allocator<gnash::as_object> > >, __gnu_cxx::__normal_iterator<gnash::as_object, std::vector<gnash::as_object, std::allocator<gnash::as_object*> > >, gnash::(anonymous namespace)::ExecuteCallback) (stl_algo.h:4275)
==11849== by 0x427C6E1: gnash::movie_root::executeAdvanceCallbacks() (movie_root.cpp:1474)
==11849== by 0x427A94E: gnash::movie_root::advance() (movie_root.cpp:855)
==11849== by 0x80BB1E1: gnash::Gui::advanceMovie(bool) (gui.cpp:970)
==11849== by 0x80E7184: gnash::Gui::advance_movie(gnash::Gui*) (gui.h:285)
==11849== by 0x3D98FC: g_timeout_dispatch (gmain.c:3585)
==11849== by 0x3D8191: g_main_context_dispatch (gmain.c:2149)
==11849== by 0x3D8977: g_main_context_iterate.clone.5 (gmain.c:2780)
==11849== Address 0x78982d4 is 12 bytes inside a block of size 20 free'd
==11849== at 0x4005821: operator delete(void*) (vg_replace_malloc.c:346)
==11849== by 0x437029F: std::auto_ptr<gnash::media::EncodedAudioFrame>::~auto_ptr() (in /home/bastiaan/gnash/obj-master/libcore/.libs/libgnashcore-0.8.9dev.so)
==11849== by 0x436D44E: gnash::Sound_as::getAudio(short*, unsigned int, bool&) (Sound_as.cpp:773)
==11849== by 0x436D60B: gnash::Sound_as::getAudioWrapper(void, short, unsigned int, bool&) (Sound_as.cpp:846)
==11849== by 0x46AF0BE: gnash::sound::AuxStream::fetchSamples(short*, unsigned int) (AuxStream.h:47)
==11849== by 0x46ADD8D: gnash::sound::sound_handler::fetchSamples(short*, unsigned int) (sound_handler.cpp:583)
==11849== by 0x46C64B4: gnash::sound::SDL_sound_handler::fetchSamples(short*, unsigned int) (sound_handler_sdl.cpp:231)
==11849== by 0x46C65CF: gnash::sound::SDL_sound_handler::sdl_audio_callback(void, unsigned char, int) (sound_handler_sdl.cpp:270)
==11849== by 0x2817FC4: SDL_RunAudio (SDL_audio.c:198)
==11849== by 0x2820B3E: SDL_RunThread (SDL_thread.c:204)
==11849== by 0x286C2BD: RunThread (SDL_systhread.c:47)
==11849== by 0x313E98: start_thread (pthread_create.c:301)
==11849==
==11849== Invalid read of size 4
==11849== at 0x4862577: gnash::media::MediaParser::nextAudioFrameTimestamp(unsigned long long&) const (MediaParser.cpp:226)
==11849== by 0x436D247: gnash::Sound_as::getPosition() const (Sound_as.cpp:752)
==11849== by 0x436BE1F: gnash::Sound_as::update() (Sound_as.cpp:341)
==11849== by 0x4277DC1: gnash::(anonymous namespace)::ExecuteCallback::operator()(gnash::as_object const*) const (movie_root.cpp:108)
==11849== by 0x428113D: gnash::(anonymous namespace)::ExecuteCallback std::for_each<__gnu_cxx::__normal_iterator<gnash::as_object*, std::vector<gnash::as_object, std::allocator<gnash::as_object> > >, gnash::(anonymous namespace)::ExecuteCallback>(__gnu_cxx::__normal_iterator<gnash::as_object, std::vector<gnash::as_object, std::allocator<gnash::as_object> > >, __gnu_cxx::__normal_iterator<gnash::as_object, std::vector<gnash::as_object, std::allocator<gnash::as_object*> > >, gnash::(anonymous namespace)::ExecuteCallback) (stl_algo.h:4275)
==11849== by 0x427C6E1: gnash::movie_root::executeAdvanceCallbacks() (movie_root.cpp:1474)
==11849== by 0x427A94E: gnash::movie_root::advance() (movie_root.cpp:855)
==11849== by 0x80BB1E1: gnash::Gui::advanceMovie(bool) (gui.cpp:970)
==11849== by 0x80E7184: gnash::Gui::advance_movie(gnash::Gui*) (gui.h:285)
==11849== by 0x3D98FC: g_timeout_dispatch (gmain.c:3585)
==11849== by 0x3D8191: g_main_context_dispatch (gmain.c:2149)
==11849== by 0x3D8977: g_main_context_iterate.clone.5 (gmain.c:2780)
==11849== Address 0x78982d0 is 8 bytes inside a block of size 20 free'd
==11849== at 0x4005821: operator delete(void*) (vg_replace_malloc.c:346)
==11849== by 0x437029F: std::auto_ptr<gnash::media::EncodedAudioFrame>::~auto_ptr() (in /home/bastiaan/gnash/obj-master/libcore/.libs/libgnashcore-0.8.9dev.so)
==11849== by 0x436D44E: gnash::Sound_as::getAudio(short*, unsigned int, bool&) (Sound_as.cpp:773)
==11849== by 0x436D60B: gnash::Sound_as::getAudioWrapper(void, short, unsigned int, bool&) (Sound_as.cpp:846)
==11849== by 0x46AF0BE: gnash::sound::AuxStream::fetchSamples(short*, unsigned int) (AuxStream.h:47)
==11849== by 0x46ADD8D: gnash::sound::sound_handler::fetchSamples(short*, unsigned int) (sound_handler.cpp:583)
==11849== by 0x46C64B4: gnash::sound::SDL_sound_handler::fetchSamples(short*, unsigned int) (sound_handler_sdl.cpp:231)
==11849== by 0x46C65CF: gnash::sound::SDL_sound_handler::sdl_audio_callback(void, unsigned char, int) (sound_handler_sdl.cpp:270)
==11849== by 0x2817FC4: SDL_RunAudio (SDL_audio.c:198)
==11849== by 0x2820B3E: SDL_RunThread (SDL_thread.c:204)
==11849== by 0x286C2BD: RunThread (SDL_systhread.c:47)
==11849== by 0x313E98: start_thread (pthread_create.c:301)
==11849==
|