Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#2557 closed defect (fixed)

SIGSEGV in Mythmusic

Reported by: bhuffman@… Owned by: stuartm
Priority: minor Milestone: 0.21
Component: mythmusic Version: 0.20
Severity: medium Keywords:
Cc: Ticket locked: no

Description

Mythmusic segfaults when changing tracks. This appears mostly randomly except that it doesn't occur in the middle of a song. It can happen on immediate startup or after playing for a long period of time. I triggered this by simply continually changing my play queue and skipping around songs. This *could* be a duplicate of 2485, but I don't know how to tell. I'm including a complete backtrace.

Attachments (5)

gdb.txt (193.6 KB) - added by bhuffman@… 13 years ago.
gdb backtrace of mythfrontend
mythobservable.patch (4.5 KB) - added by myth@… 13 years ago.
possible patch
playbackbox.diff (591 bytes) - added by paulh 13 years ago.
Patch to re-use the AudioOutput? on each track change
mythmusic_reuseaudio.diff (1.2 KB) - added by stuartm 13 years ago.
Playbackbox patch with call to audiooutputbase::Reset() (Updated)
mythobservable_segfault.diff (542 bytes) - added by stuartm 13 years ago.
Possible Fix

Download all attachments as: .zip

Change History (38)

Changed 13 years ago by bhuffman@…

Attachment: gdb.txt added

gdb backtrace of mythfrontend

comment:1 Changed 13 years ago by vbrunini@…

I'm fairly sure this is the same problem as 2485, the behavior sounds identical to what I'm experiencing. Maybe having 2 backtraces can help find the problem.

comment:2 Changed 13 years ago by myth@…

This seems to be dupe of ticket:2485. They both have a huge number of threads (with the same stack) in the backtrace. Looks like the AudioOutputBase? thread is leaked on every track, so the sigsegv is probably because of resource consumption (ticket:2485 mentions that toggling ulimit changes when the crash happens).

Changed 13 years ago by myth@…

Attachment: mythobservable.patch added

possible patch

comment:3 Changed 13 years ago by myth@…

The attached patch makes all methods in MythObservable? reentrant. I'm not sure it's the problem (couldn't reproduce a crash), but I could imagine two threads (ie. PlaybackBox? and MadDecoder?) operation on the list of listeners at the same time would be bad. It also removes some public methods that would be hard to guarantee thread safety.

comment:4 Changed 13 years ago by bhuffman@…

Thanks - I'll try this patch and see if it *seems* to fix my problem. It was never an easily reproducible crash anyway...

comment:5 Changed 13 years ago by bhuffman@…

I believe this has fixed it! Thanks! Another symptom that I saw before this patch was that after running for a while (without doing much navigating) it would crash. As part of my testing I also ran it for 24 hours straight (with the amp off, of course. ;-) ) and there were no problems.

comment:6 Changed 13 years ago by anonymous

I have also applied the patch and it has eliminated many of the segfaults I was having while editing playlists and restarting the playlist - thanks

comment:7 Changed 13 years ago by stuartm

Milestone: unknown0.21
Owner: changed from Isaac Richards to stuartm

comment:8 Changed 13 years ago by stuartm

Resolution: fixed
Status: newclosed

(In [12578]) Fixes #2557 (mythmusic segfault) by applying patch to make mythobservable thread safe. Adds locks and removes thread unsafe functions.

comment:9 Changed 13 years ago by stuartm

(In [12579]) Refs #2557

Backport [12578] to -fixes. Fixes a segfault in mythmusic.

comment:10 Changed 13 years ago by stuartm

(In [12598]) Refs #2557 - Revert [12578]. I didn't do enough testing of the patch, it seems these changes caused all sorts of havoc with autoexpiry, playback and other areas. I'm not exactly sure why, the majority of the changes simply reverted stuff which was added in [8175] - a patch originally by Eskil, as was [12578].

Until we figure out the problem it seems like a good idea to step back.

comment:11 Changed 13 years ago by anonymous

Resolution: fixed
Status: closedreopened

comment:12 Changed 13 years ago by myth@…

The thread unsafe methods weren't really added in [8175], the were just moved from MythContext to MythObservable?. I've never seen any ill effects of this patch, but then again, I pretty much only use MythVideo? and MythMusic.

Short of removing get/next/first listener, the only thing this patch really does is add a mutex to prevent modifying the listener list while someone is dispatching.

comment:13 Changed 13 years ago by bhuffman@…

I've been using this patch since the beginning of November (even with the latest SVN) and it's been working great.

comment:14 Changed 13 years ago by Stuart Auchterlonie

The changes to output.cpp no longer notify all the listeners of the object.

Stuart A.

comment:15 Changed 13 years ago by myth@…

Not so - output.cpp calls into MythObservable::dispatch which iterates across all the listeners. The only noticeable difference should be that the iteration happens with a lock held, so that the list doesn't mutate while it's being iterated on.

This might make a slight timing difference for anyone else trying to access the list ?

comment:16 in reply to:  13 Changed 13 years ago by anonymous

Replying to bhuffman@graze.net:

I've been using this patch since the beginning of November (even with the latest SVN) and it's been working great.

Have you been using other thing than mythmusic ? People were reporting the problems like

  • unable to delete recordings in some circumstances
  • strange hangs with remote frontend
  • I ended up having 34G recording yesterday, which should have been 1-2G
  • one program didn't record at all
  • parts of the mythweb were unusable (tv related)

comment:17 Changed 13 years ago by bhuffman@…

Have you been using other thing than mythmusic ? People were reporting the problems like

  • unable to delete recordings in some circumstances
  • strange hangs with remote frontend
  • I ended up having 34G recording yesterday, which should have been 1-2G
  • one program didn't record at all
  • parts of the mythweb were unusable (tv related)

I use mythtv for most things except mythweb. Specifically answering your questions:

  • I have deleted multiple recordings and haven't had a problem.
  • I've had hangs with mythfrontend, but they were related to ticket #2434 and after applying the patch in that ticket (think patch2), I don't get those hangs anymore.
  • I just looked at my recording directory and do not see any unusually large files.
  • Don't use mythweb.

comment:18 Changed 13 years ago by oa@…

I saw most of the above symptoms on my 0.20-fixes installation between Saturday (when I upgraded my installation to then-latest branch head) and Tuesday (when I upgraded again after having individually reverted nearly every other changeset but this one, thinking it didn't affect anything but MythMusic). All of the problems are now gone again.

Specifically, I saw:

  • recordings ballooning to tens of gigabytes, apparently as the backend did not stop recording at the end of the program.
  • remote frontends hanging
  • mythweb being unresponsive
  • other strangeness I can't really describe

I don't use MythMusic, though I have it installed. I don't regularly use MythVideo? either. I can attest to recordings being pretty much unusable though.

Changed 13 years ago by paulh

Attachment: playbackbox.diff added

Patch to re-use the AudioOutput? on each track change

comment:19 in reply to:  2 Changed 13 years ago by paulh

Replying to myth@eskil.org:

This seems to be dupe of ticket:2485. They both have a huge number of threads (with the same stack) in the backtrace. Looks like the AudioOutputBase? thread is leaked on every track, so the sigsegv is probably because of resource consumption (ticket:2485 mentions that toggling ulimit changes when the crash happens).

I've just attached a simple patch to re-use the AudioOutput? device on each track change. I've been running it for a couple of weeks now with no problems. If someone who can reproduce this problem give it a try to see if it improves things at all.

Although the BT shows lots of threads it may be a red herring apparently sometimes gdb can report the wrong information but it is strange that several people have said that changing ulimits has improved things for them so there may be some resource leakage somewhere.

The attached patch eliminates one possible location where some resources may be leaked. If it works great, if not at least its one thing eliminated.

comment:20 Changed 13 years ago by Honk

I applied your playbackbox.diff patch recently and mythmusic still segfaulting for me. Although it did seem to last a bit longer until it crashed (might be randomness though).

comment:21 Changed 13 years ago by stuartm

Although I'm not sure reusing AudioOutput? is the fix for this issue it's still worth doing. One obvious benefit is that audio isn't unmuted every time the track changes.

I've made one addition to Paul's patch to call AudioOutputBase::Reset() in PlaybackBoxMusic::stop. This means the music actually stops when you hit 'stop'.

Updated patch is attached. Assuming this doesn't cause any new problems for anyone I'll commit it this weekend.

As far as any leaks in mythmusic, I plan on valgrinding it when I get the time.

Changed 13 years ago by stuartm

Attachment: mythmusic_reuseaudio.diff added

Playbackbox patch with call to audiooutputbase::Reset() (Updated)

comment:22 Changed 13 years ago by stuartm

Updated the patch, found a small bug with the pause state when we aren't destroying the output class.

comment:23 Changed 13 years ago by anonymous

I'm still putting good money on my patch as being the right solution for this problem, iterating across an unlocked list that another thread is modifying never led to anything good.

I'd love to work out why it breaks other things, but since I don't use mythtv for tv, I don't have anything that is ill effected. If someone could point me to something that I can get crashing/hanging/misbehaving without having to get a tunercard and the works, I'll stab at it again.

comment:24 Changed 13 years ago by stuartm

(In [12872]) Refs #2557

Instead of deleting the AudioOuput? class every time we change track, re-use the same instance until we leave mythmusic.

This may or may not have an effect on the SIGSEGV a number of people have experienced when using mythmusic. I doubt it's the direct cause of the problem, but may reduce how often it occurs and will probably allow some better backtraces.

Other benefit are that mute won't get turned off on track changes and the transition between tracks seems a little faster.

Original patch by PaulH with some fixes to make it work.

comment:25 Changed 13 years ago by jlenk

I ran across ticket #3013 which is crash related to OGG files - I also have some OGG files which were ripped by myth too so I figured it was worth a shot so I deleted all my OGG files and reripped them as MP3 and have not had a crash since. That was almost 48 hours ago now with many stop/starts of mythmusic and continuous play.

I am also running the latest patch by stuartm and it is working well with no other side effects as far as I can tell so far.

comment:26 in reply to:  25 Changed 13 years ago by myth@…

Replying to jlenk:

I ran across ticket #3013 which is crash related to OGG files - I also have some OGG files which were ripped by myth too so I figured it was worth a shot so I deleted all my OGG files and reripped them as MP3 and have not had a crash since. That was almost 48 hours ago now with many stop/starts of mythmusic and continuous play.

Sounds reasonable, changing to only having 1 type of files lowers the chance of this bug triggering a lot, since there's less switching decoder which means less 'action' on the list of listeners.

When looking at ticket#3013, I found that it would crash almost everytime I had played an ogg/mp3 combo, left mythmusic and then went back to play. Applying the patch for mythobservable fixed it.

comment:27 Changed 13 years ago by stuartm

(In [13019]) Refs #2557

Rearrange stopDecoder and output->reset, this removes the possiblity that new samples would be sent to AudioOutput? in the few microseconds between the two calls resulting in a fragment of audio being heard at the start of the next track.

comment:28 Changed 13 years ago by stuartm

I've attached a patch which may help. I may also be barking up the wrong tree, but please give it a try.

Changed 13 years ago by stuartm

Possible Fix

comment:29 Changed 13 years ago by myth@…

I think it masks out the problem, at least I can't reproduce like I could before (ticket#3013), but that was also the only way I ever reproduced it.

I still don't think allowing two threads to read/write the same list is safe in the long run. Something else is going to get bitten by it and no-one will remember that we've already been through this.

comment:30 Changed 13 years ago by stuartm

You're right it's not the best solution, but it's a solution which avoids fixing all the other areas of mythtv which were affected when locking was used.

If the patch works for everyone who is having this problem, then it will suffice for now and if necessary I can open a new ticket as a reminder of a lack of locking in mythobservable.

comment:31 Changed 13 years ago by myth@…

Or we could just put in a doxygen comment saying that adding/removing listeners can only happen from the UI thread, that would also "solve" the problem.

comment:32 Changed 13 years ago by stuartm

Resolution: fixed
Status: reopenedclosed

(In [13306]) Closes #2557

Fixes a segfault in mythmusic when files of different types (decoders) are used in a playlist.

Unlike a previous fix, this doesn't touch mythobservable directly and should have no effect on other areas of mythtv.

comment:33 Changed 13 years ago by stuartm

(In [13307]) Refs #2557

Backports fix to -fixes.

Avoids segfault in mythmusic when files of different types (decoders) are used in a playlist.

Note: See TracTickets for help on using tickets.