[Spice-devel] [spice-gtk v2] gstaudio: check before setting mute/volume properties

[Victor Toso]

Hi,

On Mon, Jan 21, 2019 at 05:31:53AM -0500, Frediano Ziglio wrote:
> > 
> > Hi,
> > 
> > On Fri, Jan 18, 2019 at 11:53:28AM -0500, Frediano Ziglio wrote:
> > > > 
> > > > From: Victor Toso <me at victortoso.com>
> > > > 
> > > > Otherwise we can get warnings such as:
> > > > 
> > > >     GLib-GObject-WARNING **: 16:10:00.857:
> > > >     g_object_set_is_valid_property: object class 'GstAutoAudioSink' has
> > > >     no
> > > >     property named 'volume'
> > > > 
> > > > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > > 
> > > The patch remove the warning... but who control the volume?
> > 
> > We do! This is a message that host sent to us.
> > 
> 
> Yes, the host send a volume change, we (the client) get it,

We don't.

> there's no "volume" properties... so, how do we set the volume?
> 
> > > Is it just constant at the maximum?
> > 
> > Did not understand the question!
> > 
> 
> In the case there's no "volume" properties, I suppose the
> volume is fixed to the maximum.
> 
> > > Maybe we should detect and add a "volume" element to the
> > > pipeline?
> > 
> > That's doable but different. Better is to request how is actually
> > in control to set the right volume/mute for that application. If
> > we use volume element, we are doing it inside the pipeline.
> > 
> 
> If there's no way to set the volume for the application I would
> control the volume of the pipeline, at the end the result will
> be the same from the user prospective.

If there is no volume/mute, we just don't allow changing client's
volume from host's messages. Simple enough and a good compromise
for me.

> > > Does GstAutoAudioSink forward the volume settings to the chosen
> > > element (if the property is available) ?
> > 
> > AFAIK, yes, the sink/src elements. This was happening with
> > wasapi{sink, src} elements, same should happen with alsa based on
> > Snir's patch.
> > 
> > Btw, volume/mute for wasapi is ongoing
> > 
> >     https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/715
> > 
> > So, we should check if volume/mute property exists as they are
> > not really mandatory.
> > 
> 
> This I understood but from the user prospective the user will set
> the volume and nothing will happen on the client.
> 
> > Cheers,
> > 
> > > > ---
> > > >  src/spice-gstaudio.c | 32 +++++++++++++++++++++++---------
> > > >  1 file changed, 23 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > > > index 98c1f4f..51ff028 100644
> > > > --- a/src/spice-gstaudio.c
> > > > +++ b/src/spice-gstaudio.c
> > > > @@ -368,10 +368,13 @@ static void playback_volume_changed(GObject
> > > > *object,
> > > > GParamSpec *pspec, gpointer
> > > >      if (!e)
> > > >          e = g_object_ref(p->playback.sink);
> > > >  
> > > > -    if (GST_IS_STREAM_VOLUME(e))
> > > > +    if (GST_IS_STREAM_VOLUME(e)) {
> > > >          gst_stream_volume_set_volume(GST_STREAM_VOLUME(e),
> > > >          GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> > > > -    else
> > > > +    } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> > > > "volume") != NULL) {
> > > >          g_object_set(e, "volume", vol, NULL);
> > > > +    } else {
> > > > +        g_warning("playback: ignoring volume change on %s",
> > > > gst_element_get_name(e));
> > > > +    }
> > > >  
> > > >      g_object_unref(e);
> > > >  }
> > > > @@ -394,8 +397,13 @@ static void playback_mute_changed(GObject *object,
> > > > GParamSpec *pspec, gpointer d
> > > >      if (!e)
> > > >          e = g_object_ref(p->playback.sink);
> > > >  
> > > > -    if (GST_IS_STREAM_VOLUME(e))
> > > > +    if (GST_IS_STREAM_VOLUME(e)) {
> > > >          gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> > > > +    } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> > > > "mute")
> > > > != NULL) {
> > > > +        g_object_set(e, "mute", mute, NULL);
> > > > +    } else {
> > > > +        g_warning("playback: ignoring mute change on %s",
> > > > gst_element_get_name(e));
> > > > +    }
> > > >  
> > > >      g_object_unref(e);
> > > >  }
> > > > @@ -427,10 +435,13 @@ static void record_volume_changed(GObject *object,
> > > > GParamSpec *pspec, gpointer d
> > > >      if (!e)
> > > >          e = g_object_ref(p->record.src);
> > > >  
> > > > -    if (GST_IS_STREAM_VOLUME(e))
> > > > +    if (GST_IS_STREAM_VOLUME(e)) {
> > > >          gst_stream_volume_set_volume(GST_STREAM_VOLUME(e),
> > > >          GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> > > > -    else
> > > > -        g_warning("gst lacks volume capabilities on src");
> > > > +    } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> > > > "volume") != NULL) {
> > > > +        g_object_set(e, "volume", vol, NULL);
> > > > +    } else {
> > > > +        g_warning("record: ignoring volume change on %s",
> > > > gst_element_get_name(e));
> > > > +    }
> > > >  
> > > >      g_object_unref(e);
> > > >  }
> > > > @@ -453,10 +464,13 @@ static void record_mute_changed(GObject *object,
> > > > GParamSpec *pspec, gpointer dat
> > > >      if (!e)
> > > >          e = g_object_ref(p->record.src);
> > > >  
> > > > -    if (GST_IS_STREAM_VOLUME (e))
> > > > +    if (GST_IS_STREAM_VOLUME (e)) {
> > > >          gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> > > > -    else
> > > > -        g_warning("gst lacks mute capabilities on src: %d", mute);
> > > > +    } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> > > > "mute")
> > > > != NULL) {
> > > > +        g_object_set(e, "mute", mute, NULL);
> > > > +    } else {
> > > > +        g_warning("record: ignoring mute change on %s",
> > > > gst_element_get_name(e));
> > > > +    }
> > > >  
> > > >      g_object_unref(e);
> > > >  }
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190121/5320292c/attachment-0001.sig>