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. > > > > > > > > > > > > > > > > > > > > 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
