2017-02-23 13:49 GMT+01:00 <marcel-hollerb...@t-online.de>: > On Thu, Feb 23, 2017 at 01:38:31PM +0100, michael bouchaud wrote: > > 2017-02-23 13:23 GMT+01:00 <marcel-hollerb...@t-online.de>: > > > > > 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 > > > > > > > Yes so reread this value is the safer way because we set the slider to > the > > current volume. And if pulseaudio change the volume we got an > > event and the slider will move to the right place. > > > No the current volume might be old, so we have a jumping arround slider. > Which is bad. SO just dont set the value at all on drag stop, but just > re set the value after the barrier check in _slider_changed_cb. (As it > was before) >
The current volume is the current volume not old. Pulse audio notify us when changing the volume and this field is updated at this time as you said. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > -- > > 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