[pulseaudio-discuss] [PATCH 09/15] sink, source: Assign to s->muted from only one place

[Tanu Kaskinen]

On Tue, 2014-04-15 at 21:07 +0600, Alexander E. Patrakov wrote:
> 15.04.2014 16:56, Tanu Kaskinen wrote:
> > +typedef int (*pa_sink_get_mute_cb_t)(pa_sink *s, bool *mute);
> > +
> >   struct pa_sink {
> >       pa_msgobject parent;
> >   
> > @@ -191,14 +193,14 @@ struct pa_sink {
> >        * set this callback. */
> >       pa_sink_cb_t write_volume; /* may be NULL */
> >   
> > -    /* Called when the mute setting is queried. A PA_SINK_MESSAGE_GET_MUTE
> > -     * message will also be sent. Called from IO thread if PA_SINK_DEFERRED_VOLUME
> > -     * flag is set otherwise from main loop context. If refresh_mute is false
> > -     * neither this function is called nor a message is sent.
> > +    /* Called when the mute setting is queried. Called from the IO thread if
> > +     * the PA_SINK_DEFERRED_VOLUME flag is set, otherwise called from the main
> > +     * thread. The implementation is expected to set the mute parameter and
> > +     * return 0 on success, or return -1 on failure.
> >        *
> >        * You must use the function pa_sink_set_get_mute_callback() to
> >        * set this callback. */
> > -    pa_sink_cb_t get_mute; /* may be NULL */
> > +    pa_sink_get_mute_cb_t get_mute; /* may be NULL */
> 
> It is not clear from the documentation how a NULL get_mute meets the 
> expectation above (i.e. get_mute == NULL has unclear semantics). Same 
> for sources.

Ok. The whole idea of the get_mute callback could be explained better.
It's not really used for what you'd expect: normally, when someone calls
pa_sink_get_mute(), the callback is not called, because
pa_sink_get_mute() can simply return sink->muted.

The callback is only used as a mechanism for implementing
backend-initiated mute changes. When the sink implementation notices
that mute might have been changed (e.g. alsa sink gets a notification
about a mixer change), it can call pa_sink_get_mute() with
force_refresh=true. That will cause the get_mute() callback to be
called, and that will then make the sink mute state change if there was
an actual change in the backend.

Another mechanism is pa_sink_mute_changed(). Some sink implementations
(alsa and solaris) use pa_sink_get_mute() for notifying about changed
mute and the rest use pa_sink_mute_changed(). The pa_sink_get_mute()
approach is strange, and I think the alsa and solaris sinks should be
converted to use the more straightforward pa_sink_mute_changed() like
everyone else. pa_sink_get_mute() and the associated callback would then
become unnecessary. I plan to do this refactoring at some later point.

For now, would the following documentation be good?

    /* If the sink mute can change "spontaneously" (i.e. initiated by the sink
     * implementation, not by someone else calling pa_sink_set_mute()), then
     * the sink implementation can notify about changed mute either by calling
     * pa_sink_mute_changed() or by calling pa_sink_get_mute() with
     * force_refresh=true. If the implementation chooses the latter approach,
     * it should implement the get_mute callback. Otherwise get_mute can be
     * NULL.
     *
     * This is called when pa_sink_get_mute() is called with
     * force_refresh=true. This is called from the IO thread if the
     * PA_SINK_DEFERRED_VOLUME flag is set, otherwise this is called from the
     * main thread. On success, the implementation is expected to return 0 and
     * set the mute parameter that is passed as a reference. On failure, the
     * implementation is expected to return -1.
     *
     * You must use the function pa_sink_set_get_mute_callback() to
     * set this callback. */
    pa_sink_get_mute_cb_t get_mute;

-- 
Tanu