[PATCH] Fix a sndio crash when exiting in muted state
Patch from Theo Buehler <tb@openbsd.org> CVSROOT: /cvs Module name: ports Changes by: tb@cvs.openbsd.org 2023/03/14 04:08:13 Modified files: x11/mplayer : Makefile x11/mplayer/patches: patch-libao2_ao_sndio_c Log message: mplayer: fix a crash when exiting in muted state Muting mplayer with 'm', then hitting '<enter>' segfaults mplayer since it tries to unmute itself after uninitializing the sndio backend. Clear the havevol flag on uninit() to block subsequent AOCONTROL_{GET,SET}_VOLUME operations. Fix suggested by and ok ratchov Index: libao2/ao_sndio.c =================================================================== --- libao2/ao_sndio.c (revision 38412) +++ libao2/ao_sndio.c (working copy) @@ -159,6 +159,7 @@ pfds = NULL; sio_close(hdl); hdl = NULL; + havevol = 0; return 0; } @@ -172,6 +173,7 @@ hdl = NULL; free(pfds); pfds = NULL; + havevol = 0; } /*
ping. On March 14, 2023 6:31:39 a.m. Brad Smith <brad-at-comstyle.com@ffmpeg.org> wrote:
Patch from Theo Buehler <tb@openbsd.org>
CVSROOT: /cvs Module name: ports Changes by: tb@cvs.openbsd.org 2023/03/14 04:08:13
Modified files: x11/mplayer : Makefile x11/mplayer/patches: patch-libao2_ao_sndio_c
Log message: mplayer: fix a crash when exiting in muted state
Muting mplayer with 'm', then hitting '<enter>' segfaults mplayer since it tries to unmute itself after uninitializing the sndio backend. Clear the havevol flag on uninit() to block subsequent AOCONTROL_{GET,SET}_VOLUME operations.
Fix suggested by and ok ratchov
Index: libao2/ao_sndio.c =================================================================== --- libao2/ao_sndio.c (revision 38412) +++ libao2/ao_sndio.c (working copy) @@ -159,6 +159,7 @@ pfds = NULL; sio_close(hdl); hdl = NULL; + havevol = 0; return 0; }
@@ -172,6 +173,7 @@ hdl = NULL; free(pfds); pfds = NULL; + havevol = 0; }
/* _______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
Sent with Aqua Mail for Android
Index: libao2/ao_sndio.c =================================================================== --- libao2/ao_sndio.c (revision 38412) +++ libao2/ao_sndio.c (working copy) @@ -159,6 +159,7 @@ pfds = NULL; sio_close(hdl); hdl = NULL; + havevol = 0; return 0; }
@@ -172,6 +173,7 @@ hdl = NULL; free(pfds); pfds = NULL; + havevol = 0; }
Well, it's probably a good idea to do that, but a aoctl happening after uninit (if I understand right) is the far worse issue an probably needs to be fixed by itself.
Hi Brad, On 2023-03-22 22:58 +0100, Reimar Döffinger wrote:
Index: libao2/ao_sndio.c =================================================================== --- libao2/ao_sndio.c (revision 38412) +++ libao2/ao_sndio.c (working copy) @@ -159,6 +159,7 @@ pfds = NULL; sio_close(hdl); hdl = NULL; + havevol = 0; return 0; }
@@ -172,6 +173,7 @@ hdl = NULL; free(pfds); pfds = NULL; + havevol = 0; }
Well, it's probably a good idea to do that, but a aoctl happening after uninit (if I understand right) is the far worse issue an probably needs to be fixed by itself.
Could you test if the attached patch also fixes the issues you observed? Only lightly tested but it would help if you could confirm if it fixes the problem. Thanks, Alexander
On 2023-03-24 4:38 p.m., Alexander Strasser wrote:
Hi Brad,
On 2023-03-22 22:58 +0100, Reimar Döffinger wrote:
Index: libao2/ao_sndio.c =================================================================== --- libao2/ao_sndio.c (revision 38412) +++ libao2/ao_sndio.c (working copy) @@ -159,6 +159,7 @@ pfds = NULL; sio_close(hdl); hdl = NULL; + havevol = 0; return 0; }
@@ -172,6 +173,7 @@ hdl = NULL; free(pfds); pfds = NULL; + havevol = 0; } Well, it's probably a good idea to do that, but a aoctl happening after uninit (if I understand right) is the far worse issue an probably needs to be fixed by itself.
Could you test if the attached patch also fixes the issues you observed?
Only lightly tested but it would help if you could confirm if it fixes the problem.
Thanks, Alexander
According to feedback MPlayer no longer crashes with this applied and the other patch removed.
On 24 Mar 2023, at 21:38, Alexander Strasser <eclipse7@gmx.net> wrote:
Hi Brad,
On 2023-03-22 22:58 +0100, Reimar Döffinger wrote:
Index: libao2/ao_sndio.c =================================================================== --- libao2/ao_sndio.c (revision 38412) +++ libao2/ao_sndio.c (working copy) @@ -159,6 +159,7 @@ pfds = NULL; sio_close(hdl); hdl = NULL; + havevol = 0; return 0; }
@@ -172,6 +173,7 @@ hdl = NULL; free(pfds); pfds = NULL; + havevol = 0; }
Well, it's probably a good idea to do that, but a aoctl happening after uninit (if I understand right) is the far worse issue an probably needs to be fixed by itself.
Could you test if the attached patch also fixes the issues you observed?
Only lightly tested but it would help if you could confirm if it fixes the problem.
I don't know, this seems the far more obvious and straight-forward fix? IMO this looks just like an oversight really. --- mplayer.c (revision 38410) +++ mplayer.c (working copy) @@ -672,6 +672,7 @@ if (mpctx->audio_out) mpctx->audio_out->uninit(mpctx->eof ? 0 : 1); mpctx->audio_out = NULL; + mpctx->mixer.audio_out = NULL; } #ifdef CONFIG_GUI
On 25 Mar 2023, at 16:22, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
On 24 Mar 2023, at 21:38, Alexander Strasser <eclipse7@gmx.net> wrote:
Hi Brad,
On 2023-03-22 22:58 +0100, Reimar Döffinger wrote:
Index: libao2/ao_sndio.c =================================================================== --- libao2/ao_sndio.c (revision 38412) +++ libao2/ao_sndio.c (working copy) @@ -159,6 +159,7 @@ pfds = NULL; sio_close(hdl); hdl = NULL; + havevol = 0; return 0; }
@@ -172,6 +173,7 @@ hdl = NULL; free(pfds); pfds = NULL; + havevol = 0; }
Well, it's probably a good idea to do that, but a aoctl happening after uninit (if I understand right) is the far worse issue an probably needs to be fixed by itself.
Could you test if the attached patch also fixes the issues you observed?
Only lightly tested but it would help if you could confirm if it fixes the problem.
I don't know, this seems the far more obvious and straight-forward fix? IMO this looks just like an oversight really.
--- mplayer.c (revision 38410) +++ mplayer.c (working copy) @@ -672,6 +672,7 @@ if (mpctx->audio_out) mpctx->audio_out->uninit(mpctx->eof ? 0 : 1); mpctx->audio_out = NULL; + mpctx->mixer.audio_out = NULL; } #ifdef CONFIG_GUI
After some discussion, probably all our patches make sense, but a full solution likely needs a bit more. Essentially, I think the aos need to have a flag whether their mixer (volume etc) is a global, persistent one. If it is, we should not NULL the mixer audio out here, but instead require the ao SETVOLUME remains valid/allowed after uninit. To do it really properly, there would be also some additional logic to handle it fully correctly when different files are played with different -ao options. Each of the patches on their own fixes that specific crash, and mine is probably safest there, but without Alexander's causes a regression (audio remains muted after MPlayer exits). So if no objections, I think it would be best for someone (Alex?) to apply them all in one go. Best regards, Reimar
On 2023-03-26 15:58 +0200, Reimar Döffinger wrote:
On 25 Mar 2023, at 16:22, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
On 24 Mar 2023, at 21:38, Alexander Strasser <eclipse7@gmx.net> wrote:
On 2023-03-22 22:58 +0100, Reimar Döffinger wrote:
Index: libao2/ao_sndio.c =================================================================== --- libao2/ao_sndio.c (revision 38412) +++ libao2/ao_sndio.c (working copy) @@ -159,6 +159,7 @@ pfds = NULL; sio_close(hdl); hdl = NULL; + havevol = 0; return 0; }
@@ -172,6 +173,7 @@ hdl = NULL; free(pfds); pfds = NULL; + havevol = 0; }
Well, it's probably a good idea to do that, but a aoctl happening after uninit (if I understand right) is the far worse issue an probably needs to be fixed by itself.
Could you test if the attached patch also fixes the issues you observed?
Only lightly tested but it would help if you could confirm if it fixes the problem.
I don't know, this seems the far more obvious and straight-forward fix? IMO this looks just like an oversight really.
--- mplayer.c (revision 38410) +++ mplayer.c (working copy) @@ -672,6 +672,7 @@ if (mpctx->audio_out) mpctx->audio_out->uninit(mpctx->eof ? 0 : 1); mpctx->audio_out = NULL; + mpctx->mixer.audio_out = NULL; } #ifdef CONFIG_GUI
After some discussion, probably all our patches make sense, but a full solution likely needs a bit more. Essentially, I think the aos need to have a flag whether their mixer (volume etc) is a global, persistent one. If it is, we should not NULL the mixer audio out here, but instead require the ao SETVOLUME remains valid/allowed after uninit. To do it really properly, there would be also some additional logic to handle it fully correctly when different files are played with different -ao options.
I would like doing something like that after a release. Would need some design and probably more or less changes in all aos depending on the exact architecture chosen.
Each of the patches on their own fixes that specific crash, and mine is probably safest there, but without Alexander's causes a regression (audio remains muted after MPlayer exits). So if no objections, I think it would be best for someone (Alex?) to apply them all in one go.
I intent to apply all of the patches after a quick test session in one go. Will do it probably on Friday. Thank you all, Alexander
On 2023-03-28 21:25 +0200, Alexander Strasser wrote:
On 2023-03-26 15:58 +0200, Reimar Döffinger wrote:
On 25 Mar 2023, at 16:22, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
On 24 Mar 2023, at 21:38, Alexander Strasser <eclipse7@gmx.net> wrote:
[patches snipped]
After some discussion, probably all our patches make sense, but a full solution likely needs a bit more. Essentially, I think the aos need to have a flag whether their mixer (volume etc) is a global, persistent one. If it is, we should not NULL the mixer audio out here, but instead require the ao SETVOLUME remains valid/allowed after uninit. To do it really properly, there would be also some additional logic to handle it fully correctly when different files are played with different -ao options.
I would like doing something like that after a release.
Would need some design and probably more or less changes in all aos depending on the exact architecture chosen.
Each of the patches on their own fixes that specific crash, and mine is probably safest there, but without Alexander's causes a regression (audio remains muted after MPlayer exits). So if no objections, I think it would be best for someone (Alex?) to apply them all in one go.
I intent to apply all of the patches after a quick test session in one go.
Will do it probably on Friday.
All patches committed. Sorry for the week long delay. I did bit more testing and tried to make the commit messages more exhaustive. Best regards, Alexander
On 22 Mar 2023, at 22:58, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
Index: libao2/ao_sndio.c =================================================================== --- libao2/ao_sndio.c (revision 38412) +++ libao2/ao_sndio.c (working copy) @@ -159,6 +159,7 @@ pfds = NULL; sio_close(hdl); hdl = NULL; + havevol = 0; return 0; }
@@ -172,6 +173,7 @@ hdl = NULL; free(pfds); pfds = NULL; + havevol = 0; }
Well, it's probably a good idea to do that, but a aoctl happening after uninit (if I understand right) is the far worse issue an probably needs to be fixed by itself.
I can confirm this needs to be fixed elsewhere, as I get exactly the same crash e.g. on macos with ao_coreaudio. The backtrace there looks like this: * frame #0: 0x0000000100019868 mplayer`control(cmd=5, arg=0x000000016fdfe490) at ao_coreaudio.c:197:11 [opt] frame #1: 0x000000010000a7e8 mplayer`mixer_setvolume(mixer=0x0000000100f194c0, l=<unavailable>, r=<unavailable>) at mixer.c:70:23 [opt] frame #2: 0x000000010000aa5c mplayer`mixer_mute(mixer=<unavailable>) at mixer.c:136:3 [opt] [artificial] frame #3: 0x000000010000b880 mplayer`exit_player_with_rc(how=EXIT_EOF, rc=0) at mplayer.c:706:9 [opt] frame #4: 0x0000000100010f78 mplayer`main(argc=<unavailable>, argv=<unavailable>) at mplayer.c:0 [opt] frame #5: 0x000000018e57fe50 dyld`start + 2544 I suspect that when doing ao uninit the mixer needs to be updated?
participants (3)
-
Alexander Strasser -
Brad Smith -
Reimar Döffinger