On Thu, Feb 23, 2017 at 01:18:35PM +0100, michael bouchaud wrote:
> 2017-02-23 13:13 GMT+01:00 <marcel-hollerb...@t-online.de>:
>
> > On Thu, Feb 23, 2017 at 01:00:10PM +0100, michael bouchaud wrote:
> > > 2017-02-23 12:37 GMT+01:00 <marcel-hollerb...@t-online.de>:
> > >
> > > > On Thu, Feb 23, 2017 at 12:14:05PM +0100, michael bouchaud wrote:
> > > > > 2017-02-23 12:01 GMT+01:00 <marcel-hollerb...@t-online.de>:
> > > > >
> > > > > > First of all to the technical side:
> > > > > >
> > > > > > The commit i pushed removes the code that fetches the volume from
> > pa
> > > > > > when the drag of a slider is done. Reason for that was that we
> > might
> > > > > > set a old volume from pulseaudio which is not the last one the user
> > > > > > dragged to. This can happen bacause the volume is sometimes not up
> > to
> > > > > > date when we are ending the drag, if pa gives us a new volume that
> > is
> > > > > > not exact to the one we had when the drag stopped (for example some
> > > > > > clamping of the volume happened inside pulseaudio) then we are
> > getting
> > > > > > a callback where we are setting the volume again in the slider. So
> > > > > > that removing of code does not really remove a feature. We are
> > getting
> > > > > > notified later about a new volume.
> > > > > >
> > > > >
> > > > > No we aren't notified, because the volume doesn't change. If the
> > volume
> > > > > is setted to 100 as example and you set it to 105. emix won't change
> > the
> > > > > volume. The volume is still setted to 100 not 105 ...
> > > >
> > > > Well depends on what you are doing. If you are dragging then it should
> > > > work, if you do it with clicking/keybindings then it will clamp at 100%
> > > > (BARRIER
> > > > feature),. Can you elaborate a bit on your usecase?
> > > >
> > >
> > > you have 100 setted and you drag to 105, as you say BARRIER feature
> > > won't change the volume. But without this commit the slider stay at 105.
> >
> > It did before commit dbdf411b488fd4d3f37a26d8cb142b25aba784d6. Reread
> > the patch, you are removing there a elm_slider_value_set, this ensured
> > that the value stayed at the value returned by the barrier feature.
> >
> > >
> > >
> > > > >
> > > > >
> > > > > >
> > > > > > On Thu, Feb 23, 2017 at 11:30:18AM +0100, Michaël Bouchaud wrote:
> > > > > > > Sorry for the miss of comments about this revert.
> > > > > > >
> > > > > > > I explained to you, this commit remove something needed to get
> > proper
> > > > > > value
> > > > > > > setted to the volume slider.
> > > > > >
> > > > > > What is that something ? As explained above, if something else
> > changes
> > > > > > the volume again, we are getting a callback that results in a
> > update of
> > > > > > the slider.
> > > > > >
> > > > > > > When you have done this revert you have posted this video
> > > > > > > https://omicron.homeip.net/filedump/mixer_gadget_bug.ogv. We
> > can see
> > > > > > that
> > > > > > > emixer works great but not the enlightenment mixer module. Both
> > share
> > > > > > about
> > > > > > > 90
> > > > > > > percents of the code to talk to pulseaudio. So no need to talk
> > with
> > > > > > > pulseaudio
> > > > > > > guys, we got something weird into our enlightenment volume
> > mixer. I
> > > > > > analyze
> > > > > > > code of two and see the enlightenment module make some alloc to
> > set
> > > > the
> > > > > > > volume
> > > > > > > and emixer doesn't. I fix that in the previous commit
> > > > > > > (dbdf411b488fd4d3f37a26d8cb142b25aba784d6). I've asked
> > morlenxus to
> > > > > > check my
> > > > > > > branch, but he never came back to say if this work. I don't see
> > why
> > > > this
> > > > > > > wouldn't working because emixer and enlightenment module are the
> > same
> > > > > > code
> > > > > > > now.
> > > > > >
> > > > > > While analyzing the code of emixer.c and the module you probebly
> > should
> > > > > > have noticed that emixer doesnt do exactly THAT. it does not set
> > the
> > > > > > volume again on drag end. thats NOT done. The only place where
> > this is
> > > > > > done is on a input volume slider, not the sink or sink input.
> > > > > > And the bug in the video is clearly happening for sink / sink
> > input.
> > > > > >
> > > > >
> > > > > ??? Sorry but the code is in the revert ...
> > > > >
> > > >
> > > > Your commit (fba185798cf75eaeaba4a95d2be25fb2fea6ef1a) brings back the
> > > > resetting of the volume on drag stop. Show me where this is done in
> > > > emixer.c for a slider of the sink volume.
> > > >
> > >
> > > You remove it ... so :)
> >
> > yeah. in e_mod_main.c but since you are comparing emixer with the module
> > you should also have the code you want in somewhere in emixer.c show me
> > where it is.
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > The weirdness part in our enlightenment codebase here is that we
> > > > > > sometimes get into the state where we are getting updates from
> > > > > > pulseaudio very slowly (Can be caused by havy work load, etc.
> > etc.).
> > > > > >
> > > > >
> > > > > Absolutely not, we are just rereading the value we have setted to
> > > > > pulseaudio.
> > > > > Pulseaudio isn't solicited at this time.
> > > > >
> > > >
> > > > Its not like this, read the pulse code of emix. if we are setting a
> > > > volume to pulseaudio it just translates it to a pulseaudio volume
> > struct
> > > > and sets it for that sink. It never attaches the same volume struct to
> > > > the sink. The volume in the sink is only updated when pa sends the
> > > > callback of "hey volume has changed" (pulse.c:215)
> > > >
> > >
> > > yes but the code here don't call any emix or pulse stuff. Just reread the
> > > value.
> > >
> > > /* retrieve data */
> > > EINA_SAFETY_ON_NULL_RETURN(mixer_context->sink_default);
> > > Emix_Sink *s = (Emix_Sink *)mixer_context->sink_default;
> > > /* Read it */
> > > int val = s->volume.volumes[0];
> > > /* Set it */
> > > elm_slider_value_set(obj, val);
> > >
> > > Please if I'm wrong, spot me where we call pulse stuff.
> >
> > No it does not. s->volume.volumes[0] is still the old last value known
> > from pulseaudio. Not the value we have setted. And this is bad and can
> > result in the bug you can see in the video.
> >
>
> Not the value setted to pulseaudio ? And what do VOLSET macro ?
Thats what it does:
#define VOLSET(vol, srcvol, target, func) \
do { \
Emix_Volume _v; \
int _pvol = srcvol.volumes[0]; \
if ((_pvol > 80) && (_pvol <= 100) && \
(vol > 100) && (vol < 120)) vol = 100; \
_v.channel_count = srcvol.channel_count; \
_v.volumes = calloc(srcvol.channel_count, sizeof(int)); \
if (_v.volumes) { \
unsigned int _i; \
for (_i = 0; _i < _v.channel_count; _i++) _v.volumes[_i] = vol; \
func(target, _v); \
free(_v.volumes); \
} \
} while (0)
It just gives the volume to emix with the func you pass in.
But the volume is never ever copied to target->volume.
target->volume is just changed when pulseaudio sends a new volume
>
> >
> > >
> > > >
> > > > But now, lets assume you are right. That means that we are setting
> > EXACTLY
> > > > the same value we have just gotten from our slider, back to our slider.
> > > > Why is that needed?
> > > >
> > >
> > > To be sure we display the correct value setted to pulse (BARRIER feature
> > > could not change the volume or set it to a different value).
> > >
> > > If you want to see why we need this. Open emixer and move the
> > > enlightenment module slider in different value. You will see we don't get
> > > the same value on both in some case.
> > >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > > And more important, not getting this commit into our code just
> > hidde
> > > > us
> > > > > > the
> > > > > > > bug
> > > > > > > and give us no chance to spot it.
> > > > > >
> > > > > > Thats a part i dont understand :)
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > 2017-02-23 9:09 GMT+01:00 Marcel Hollerbach <
> > > > > > marcel-hollerb...@t-online.de>:
> > > > > > >
> > > > > > > > bu5hm4n pushed a commit to branch master.
> > > > > > > >
> > > > > > > > http://git.enlightenment.org/core/enlightenment.git/commit/
> > ?id=
> > > > > > > > 9745890a37036091d5dec320fecda7ed4c6bdb6c
> > > > > > > >
> > > > > > > > commit 9745890a37036091d5dec320fecda7ed4c6bdb6c
> > > > > > > > Author: Marcel Hollerbach <marcel-hollerb...@t-online.de>
> > > > > > > > Date: Thu Feb 23 09:08:24 2017 +0100
> > > > > > > >
> > > > > > > > Revert "Revert "mixer: do not set back the value from emix
> > > > once the
> > > > > > > > drag is finished""
> > > > > > > >
> > > > > > > > This reverts commit fba185798cf75eaeaba4a95d2be25f
> > b2fea6ef1a.
> > > > > > > >
> > > > > > > > There is not even a description why you reverted it. This
> > is a
> > > > > > bugfix
> > > > > > > > that fixed a bug. So talk to me what the issue is, but
> > please
> > > > stop
> > > > > > > > reverting commits silently.
> > > > > > > > ---
> > > > > > > > src/modules/mixer/e_mod_main.c | 11 -----------
> > > > > > > > src/modules/mixer/emixer.c | 13 -------------
> > > > > > > > 2 files changed, 24 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/src/modules/mixer/e_mod_main.c
> > > > b/src/modules/mixer/e_mod_
> > > > > > > > main.c
> > > > > > > > index ac805cc..2c86915 100644
> > > > > > > > --- a/src/modules/mixer/e_mod_main.c
> > > > > > > > +++ b/src/modules/mixer/e_mod_main.c
> > > > > > > > @@ -481,16 +481,6 @@ _slider_changed_cb(void *data EINA_UNUSED,
> > > > > > > > Evas_Object *obj,
> > > > > > > > }
> > > > > > > >
> > > > > > > > static void
> > > > > > > > -_slider_drag_stop_cb(void *data EINA_UNUSED, Evas_Object *obj,
> > > > > > > > - void *event EINA_UNUSED)
> > > > > > > > -{
> > > > > > > > - EINA_SAFETY_ON_NULL_RETURN(mixer_context->sink_default);
> > > > > > > > - Emix_Sink *s = (Emix_Sink *)mixer_context->sink_default;
> > > > > > > > - int val = s->volume.volumes[0];
> > > > > > > > - elm_slider_value_set(obj, val);
> > > > > > > > -}
> > > > > > > > -
> > > > > > > > -static void
> > > > > > > > _sink_selected_cb(void *data, Evas_Object *obj EINA_UNUSED,
> > void
> > > > > > > > *event_info EINA_UNUSED)
> > > > > > > > {
> > > > > > > > Emix_Sink *s = data;
> > > > > > > > @@ -554,7 +544,6 @@ _popup_new(Instance *inst)
> > > > > > > > evas_object_show(slider);
> > > > > > > > elm_slider_min_max_set(slider, 0.0, emix_max_volume_get());
> > > > > > > > evas_object_smart_callback_add(slider, "changed",
> > > > > > _slider_changed_cb,
> > > > > > > > NULL);
> > > > > > > > - evas_object_smart_callback_add(slider, "slider,drag,stop",
> > > > > > > > _slider_drag_stop_cb, NULL);
> > > > > > > > elm_slider_value_set(slider, volume);
> > > > > > > > elm_box_pack_end(bx, slider);
> > > > > > > > evas_object_show(slider);
> > > > > > > > diff --git a/src/modules/mixer/emixer.c
> > > > b/src/modules/mixer/emixer.c
> > > > > > > > index 5cde881..1bcd96c 100644
> > > > > > > > --- a/src/modules/mixer/emixer.c
> > > > > > > > +++ b/src/modules/mixer/emixer.c
> > > > > > > > @@ -49,17 +49,6 @@ _cb_sink_volume_change(void *data,
> > > > > > > > }
> > > > > > > >
> > > > > > > > static void
> > > > > > > > -_cb_sink_volume_drag_stop(void *data,
> > > > > > > > - Evas_Object *obj,
> > > > > > > > - void *event EINA_UNUSED)
> > > > > > > > -{
> > > > > > > > - Evas_Object *bxv = data;
> > > > > > > > - Emix_Sink *sink = evas_object_data_get(bxv, "sink");
> > > > > > > > - int vol = sink->volume.volumes[0];
> > > > > > > > - elm_slider_value_set(obj, vol);
> > > > > > > > -}
> > > > > > > > -
> > > > > > > > -static void
> > > > > > > > _cb_sink_mute_change(void *data,
> > > > > > > > Evas_Object *obj,
> > > > > > > > void *event_info EINA_UNUSED)
> > > > > > > > @@ -134,8 +123,6 @@ _emix_sink_add(Emix_Sink *sink)
> > > > > > > > elm_box_pack_end(bx, sl);
> > > > > > > > evas_object_show(sl);
> > > > > > > > evas_object_smart_callback_add(sl, "changed",
> > > > > > _cb_sink_volume_change,
> > > > > > > > bxv);
> > > > > > > > - evas_object_smart_callback_add(sl, "slider,drag,stop",
> > > > > > > > - _cb_sink_volume_drag_stop,
> > bxv);
> > > > > > > >
> > > > > > > > ck = elm_check_add(win);
> > > > > > > > evas_object_data_set(bxv, "mute", ck);
> > > > > > > >
> > > > > > > > --
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Michaël Bouchaud (yoz) <y...@efl.so>
> > > > > > > ------------------------------------------------------------
> > > > > > ------------------
> > > > > > > Check out the vibrant tech community on one of the world's most
> > > > > > > engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> > > > > > > _______________________________________________
> > > > > > > enlightenment-devel mailing list
> > > > > > > enlightenment-devel@lists.sourceforge.net
> > > > > > > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
> > > > > >
> > > > > > ------------------------------------------------------------
> > > > > > ------------------
> > > > > > Check out the vibrant tech community on one of the world's most
> > > > > > engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> > > > > > _______________________________________________
> > > > > > enlightenment-devel mailing list
> > > > > > enlightenment-devel@lists.sourceforge.net
> > > > > > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Michaël Bouchaud
> > > > > ------------------------------------------------------------
> > > > ------------------
> > > > > Check out the vibrant tech community on one of the world's most
> > > > > engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> > > > > _______________________________________________
> > > > > enlightenment-devel mailing list
> > > > > enlightenment-devel@lists.sourceforge.net
> > > > > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
> > > >
> > > > ------------------------------------------------------------
> > > > ------------------
> > > > Check out the vibrant tech community on one of the world's most
> > > > engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> > > > _______________________________________________
> > > > enlightenment-devel mailing list
> > > > enlightenment-devel@lists.sourceforge.net
> > > > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
> > > >
> > >
> > >
> > >
> > > --
> > > Michaël Bouchaud
> > > ------------------------------------------------------------
> > ------------------
> > > Check out the vibrant tech community on one of the world's most
> > > engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> > > _______________________________________________
> > > enlightenment-devel mailing list
> > > enlightenment-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
> >
> > ------------------------------------------------------------
> > ------------------
> > Check out the vibrant tech community on one of the world's most
> > engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> > _______________________________________________
> > enlightenment-devel mailing list
> > enlightenment-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
> >
>
>
>
> --
> Michaël Bouchaud
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> _______________________________________________
> enlightenment-devel mailing list
> enlightenment-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
