[pulseaudio-discuss] [PATCH 1/2] sink, source: Add a flag to inhibit suspend

[David Henningsson]

On 08/08/2013 09:14 AM, Arun Raghavan wrote:
> On Thu, 2013-08-08 at 09:57 +0300, Tanu Kaskinen wrote:
>> On Thu, 2013-08-08 at 12:09 +0530, Arun Raghavan wrote:
>>> On Thu, 2013-08-08 at 09:07 +0300, Tanu Kaskinen wrote:
>>>> On Thu, 2013-08-08 at 10:53 +0530, Arun Raghavan wrote:
>>>>> This adds the ability to inhibit the suspending of a sink/source. Policy
>>>>> modules may override this, but should avoid doing so if they can.
>>>>> ---
>>>>>  src/pulse/def.h        | 6 ++++++
>>>>>  src/pulsecore/sink.c   | 3 +++
>>>>>  src/pulsecore/source.c | 3 +++
>>>>>  3 files changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/src/pulse/def.h b/src/pulse/def.h
>>>>> index 58190cb..11f5cb2 100644
>>>>> --- a/src/pulse/def.h
>>>>> +++ b/src/pulse/def.h
>>>>> @@ -795,6 +795,9 @@ typedef enum pa_sink_flags {
>>>>>  
>>>>>      PA_SINK_DEFERRED_VOLUME = 0x2000000U,
>>>>>      /**< The HW volume changes are syncronized with SW volume. */
>>>>> +
>>>>> +    PA_SINK_ALWAYS_RUNNING = 0x4000000U,
>>>>> +    /**< The sink should not be automatically suspended. */
>>>>>  /** \endcond */
>>>>>  #endif
>>>>>  
>>>>> @@ -914,6 +917,9 @@ typedef enum pa_source_flags {
>>>>>  
>>>>>      PA_SOURCE_DEFERRED_VOLUME = 0x2000000U,
>>>>>      /**< The HW volume changes are syncronized with SW volume. */
>>>>> +
>>>>> +    PA_SOURCE_ALWAYS_RUNNING = 0x4000000U,
>>>>> +    /**< The source should not be automatically suspended. */
>>>>>  #endif
>>>>>  } pa_source_flags_t;
>>>>>  
>>>>> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
>>>>> index 00bc29a..9ebf4ce 100644
>>>>> --- a/src/pulsecore/sink.c
>>>>> +++ b/src/pulsecore/sink.c
>>>>> @@ -834,6 +834,9 @@ int pa_sink_suspend(pa_sink *s, bool suspend, pa_suspend_cause_t cause) {
>>>>>      pa_assert(PA_SINK_IS_LINKED(s->state));
>>>>>      pa_assert(cause != 0);
>>>>>  
>>>>> +    if ((s->flags & PA_SINK_ALWAYS_RUNNING) && (cause == PA_SUSPEND_IDLE))
>>>>> +        return 0;
>>>>
>>>> Shouldn't the return value be -1?
>>>>
>>>> This assumes that if ALWAYS_RUNNING is set, then the IDLE cause can't
>>>> appear together with other causes. I think that's a valid assumption,
>>>> though, so I don't have a problem with that. Avoiding that assumption
>>>> would require deciding what to do when one suspend cause is not allowed
>>>> and one is, and I don't want to form an opinion about that.
>>>
>>> My thinking was basically that this is not an error. A policy module
>>> said "go ahead and suspend this sink, I think it's not needed", while
>>> some other policy asserted "nope, don't ever suspend this sink". So it's
>>> not an error but a behaviour override.
>>
>> I disagree. ALWAYS_RUNNING is set by hw adaptation code, and the hw
>> adaptation shouldn't have any say in the policy. I think the
>> ALWAYS_RUNNING flag describes the hardware, it says "if you suspend this
>> sink needlessly, bad things will happen". The flag is not part of the
>> policy configuration. ALWAYS_RUNNING does sound like a policy flag,
>> though, so I think AVOID_UNNECESSARY_SUSPENDING would be a better name.
> 
> Too long and unwieldy imo.

Perhaps the flag could be PA_SINK_IGNORE_IDLE or
PA_SINK_IGNORE_SUSPEND_IDLE ? Because given your patch, it isn't always
running, it's just ignoring the idle suspend cause.

Another question btw. Is this PCM being used for DTMF, in-call music
playback or such? Or is it just open without any practical usage? In the
latter case, one could perhaps consider having the entire logic in
alsa-lib instead of PulseAudio.


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic