Bug 59183 - arts_suspend() returns 0 if arts doesn't have DSP device open
Summary: arts_suspend() returns 0 if arts doesn't have DSP device open
Status: RESOLVED FIXED
Alias: None
Product: arts
Classification: Miscellaneous
Component: general (show other bugs)
Version: 1.1.1
Platform: Mandrake RPMs Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: George Staikos
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2003-05-31 19:25 UTC by David Walser
Modified: 2004-02-22 02:26 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
TiMidity++-2.12.0-pre1-detect.patch.luigi (990 bytes, application/octet-stream)
2003-06-27 12:03 UTC, David Walser
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Walser 2003-05-31 19:25:02 UTC
Version:           1.1.1 (using KDE KDE 3.1.1)
Installed from:    Mandrake RPMs
Compiler:          gcc version 3.2.2 (Mandrake Linux 9.2 3.2.2-5mdk) 
OS:          Linux

arts_suspend() will have artsd let go of the DSP device if it has it open.  If it's sucessful, the postcondition is that artsd doesn't have the DSP device open, and 1 is returned.

There is one condition where this postcondition is broken, that's when you call arts_suspend() and artsd didn't have the DSP device open in the first place.  As far as trying to get artsd to let go of the device, it shouldn't matter whether it had it open in the first place or not, if arts can manage to make sure it's not holding the device (whether it needs to do any work to make that happen or not), 1 should be returned.

0 should only be returned when, as it said in the header file, "there were active non-suspendable modules," meaning artsd has the device open and is not going to give it up.

arts_suspend() should not return 0 when arts doesn't have the DSP device open.
Comment 1 George Staikos 2003-06-27 09:32:39 UTC
This looks very easy to fix, but can you tell me what you think it should 
return in this case, and provide a testcase to verify the results?  I don't 
want to commit a fix without this information. 
 
Thanks 
 
Comment 2 David Walser 2003-06-27 12:03:01 UTC
Subject: Re:  arts_suspend() returns 0 if arts doesn't have DSP device open         

Hi, I apologize for e-mailing you directly, but KDE
Bugzilla is broken at the moment.  When I try to
submit an update it says "product was not defined;
this may indicate a bug in your browser."  It says
that whether I'm in Konqueror or Mozilla.  I'll paste
what I was trying to write here (and attach the
mentioned patch to this e-mail).

Thank you. 
 
In the header file, it says "asks aRtsd to free the
DSP device and return 1 if it was successful," so as
long as artsd isn't holding the device open after the
function is run, 1 should be returned. 
 
Here's a testcase: 
killall artsd 
artsd -s 45 
artsdsp play $(dirname $(dirname `which
startkde`))/share/sounds/KDE_Startup.wav 
 
Then wait one minute, and run a piece of C code that
runs arts_suspend().  It should return 1. 
 
If you want some code, if you're running RedHat or
Mandrake, get the newest TiMidity++ source RPM package
from RedHat Rawhide, or Mandrake Cooker, and replace
the patch that's name ends in detect.patch with the
one I attach here. 
 
If you want me to make you a tarball, let me know. 
 
When you compile and run timidity with this patch, it
will use arts output if arts_suspend returns 0, and
OSS output if it returns 1.  The easiest way I can
tell which output it's using is if it's using arts you
see the infamous arts error message: 
mcop warning: user defined signal handler found for
SIG_PIPE, overriding 
 
you could also just try playing something else with
arts simultaneously. 
 
Anyway, the expected behavior with this patched
timidity is this: 
arts is currently playing something, timidity uses
arts output 
arts is not currently playing something (whether 45
seconds have passed and it's already given up the
device or not), timidity uses OSS output 

--- George Staikos <staikos@kde.org> wrote:
> ------- You are receiving this mail because: -------
> You reported the bug, or are watching the reporter.
>      
> http://bugs.kde.org/show_bug.cgi?id=59183     
> staikos@kde.org changed:
> 
>            What    |Removed                    
> |Added
>
----------------------------------------------------------------------------
>              Status|UNCONFIRMED                 |NEW
>       everconfirmed|0                           |1
> 
> 
> 
> ------- Additional Comments From staikos@kde.org 
> 2003-06-27 09:32 -------
> This looks very easy to fix, but can you tell me
> what you think it should 
> return in this case, and provide a testcase to
> verify the results?  I don't 
> want to commit a fix without this information. 
>  
> Thanks

__________________________________
Do you Yahoo!?
SBC Yahoo! DSL - Now only $29.95 per month!
http://sbc.yahoo.com

Created an attachment (id=1893)
TiMidity++-2.12.0-pre1-detect.patch.luigi
Comment 3 George Staikos 2003-07-02 18:00:07 UTC
Correct, it returns -2 right now.  I'll repair asap. 
Comment 4 George Staikos 2003-07-02 18:08:20 UTC
sorry, correction: it returns 0, not -2 :) 
Comment 5 David Walser 2003-07-02 18:26:31 UTC
I was about to say :o) 
 
I think I found the problem.  I'm looking now at arts/soundserver/soundserver_impl.cc 
in the function SoundServer_impl::suspend() and it looks like this currently: 
bool SoundServer_impl::suspend() { 
        if(Dispatcher::the()->flowSystem()->suspendable() && 
           !Dispatcher::the()->flowSystem()->suspended()) 
                { 
                        Dispatcher::the()->flowSystem()->suspend(); 
                        arts_info("sound server suspended by client"); 
                        return true; 
                } 
        return false; 
} 
 
I believe it should look like the following: 
bool SoundServer_impl::suspend() { 
	if(Dispatcher::the()->flowSystem()->suspended() 
		return true; 
	if(Dispatcher::the()->flowSystem()->suspendable() { 
		Dispatcher::the()->flowSystem()->suspend(); 
		arts_info("sound server suspended by client"); 
		return true; 
	} 
return false; 
} 
Comment 6 George Staikos 2003-07-02 18:29:28 UTC
Subject: Re:  arts_suspend() returns 0 if arts doesn't have DSP device open

On Wednesday 02 July 2003 12:26, you wrote:
> ------- I was about to say :o)
>
> I think I found the problem.  I'm looking now at
> arts/soundserver/soundserver_impl.cc in the function
> SoundServer_impl::suspend() and it looks like this currently: bool
> SoundServer_impl::suspend() {
>         if(Dispatcher::the()->flowSystem()->suspendable() &&
>            !Dispatcher::the()->flowSystem()->suspended())
>                 {
>                         Dispatcher::the()->flowSystem()->suspend();
>                         arts_info("sound server suspended by client");
>                         return true;
>                 }
>         return false;
> }
>
> I believe it should look like the following:
> bool SoundServer_impl::suspend() {
> 	if(Dispatcher::the()->flowSystem()->suspended()
> 		return true;
> 	if(Dispatcher::the()->flowSystem()->suspendable() {
> 		Dispatcher::the()->flowSystem()->suspend();
> 		arts_info("sound server suspended by client");
> 		return true;
> 	}
> return false;
> }

  Hah I was *just* about to send the exact same thing back to you!  Ok I will 
commit this fix as I believe it to be correct too.  Thanks!

Comment 7 George Staikos 2003-07-02 18:33:35 UTC
Subject: ARTS_1_1_BRANCH: arts/soundserver

CVS commit by staikos: 

Fix 59183 with the excellent help of the reporter.
This could use a forward port for those who have arts-head checked out.  thanks.

CCMAIL: 59183-done@bugs.kde.org


  M +5 -2      soundserver_impl.cc   1.7.4.2


--- arts/soundserver/soundserver_impl.cc  #1.7.4.1:1.7.4.2
@@ -63,6 +63,9 @@ long SoundServer_impl::secondsUntilSuspe
 
 bool SoundServer_impl::suspend() {
-        if(Dispatcher::the()->flowSystem()->suspendable() &&
-           !Dispatcher::the()->flowSystem()->suspended())
+        if (Dispatcher::the()->flowSystem()->suspended())
+                {
+                        return true;
+                }
+        if(Dispatcher::the()->flowSystem()->suspendable())
                 {
                         Dispatcher::the()->flowSystem()->suspend();


Comment 8 Stefan Brüns 2004-02-22 02:26:53 UTC
This is a duplicate of BR 44566