2017-02-23 13:38 GMT+01:00 michael bouchaud <michael.bouch...@gmail.com>:
> 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. >> > I remove it because it isn't in a good shape and all other place use VOLSET macro. So for consistency and reduce code duplication I use emix as other place do. > > > >> > > > >> > > > >> > > > > > >> > > > > > >> > > > > > > >> > > > > > > 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/c >> ore/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(mi >> xer_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-d >> evel >> > > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > -- >> > > > > > 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-d >> evel >> > > > > >> > > > > ------------------------------------------------------------ >> > > > > ------------------ >> > > > > 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 > -- 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
