On Wed, 30 Oct 2013 11:29:57 -0200 Ulisses Furquim <uliss...@gmail.com> said:
> Hi Raster, > > On Tue, Oct 29, 2013 at 11:35 AM, Carsten Haitzler <ras...@rasterman.com> > wrote: > > On Tue, 29 Oct 2013 10:20:33 -0200 Ulisses Furquim <uliss...@gmail.com> > > said: > > > >> Raster, > >> > >> On Mon, Oct 28, 2013 at 10:03 PM, Carsten Haitzler <ras...@rasterman.com> > >> wrote: > >> > On Mon, 28 Oct 2013 17:13:08 -0200 Ulisses Furquim <uliss...@gmail.com> > >> > said: > >> > > >> >> Hi Raster, > >> >> > >> >> On Mon, Oct 28, 2013 at 12:47 AM, Carsten Haitzler > >> >> <ras...@rasterman.com> wrote: > >> >> > On Fri, 18 Oct 2013 15:19:00 -0300 Ulisses Furquim > >> >> > <uliss...@gmail.com> said: > >> >> > > >> >> >> Raster, > >> >> >> > >> >> >> On Wed, Oct 16, 2013 at 8:58 PM, Carsten Haitzler > >> >> >> <ras...@rasterman.com> wrote: > >> >> >> > On Wed, 16 Oct 2013 12:26:26 -0300 Ulisses Furquim > >> >> >> > <uliss...@gmail.com> said: > >> >> >> > > >> >> >> >> Raster, > >> >> >> >> > >> >> >> >> On Wed, Oct 16, 2013 at 12:01 PM, Carsten Haitzler > >> >> >> >> <ras...@rasterman.com> wrote: > >> >> >> >> > raster pushed a commit to branch master. > >> >> >> >> > > >> >> >> >> > http://git.enlightenment.org/core/efl.git/commit/?id=06c3c0cd0c0e2af7279470ab5b3fd3100e1499db > >> >> >> >> > > >> >> >> >> > commit 06c3c0cd0c0e2af7279470ab5b3fd3100e1499db > >> >> >> >> > Author: Carsten Haitzler (Rasterman) <ras...@rasterman.com> > >> >> >> >> > Date: Thu Oct 17 00:00:05 2013 +0900 > >> >> >> >> > > >> >> >> >> > async render -> alpha set. if not visible dont WAIT. do it > >> >> >> >> > now. > >> >> >> >> > --- > >> >> >> >> > src/modules/ecore_evas/engines/x/ecore_evas_x.c | 11 +++++++ > >> >> >> >> > +--- 1 file changed, 8 insertions(+), 3 deletions(-) > >> >> >> >> > > >> >> >> >> > diff --git a/src/modules/ecore_evas/engines/x/ecore_evas_x.c > >> >> >> >> > b/src/modules/ecore_evas/engines/x/ecore_evas_x.c index > >> >> >> >> > 627dd15..69e0709 100644 > >> >> >> >> > --- a/src/modules/ecore_evas/engines/x/ecore_evas_x.c > >> >> >> >> > +++ b/src/modules/ecore_evas/engines/x/ecore_evas_x.c > >> >> >> >> > @@ -2284,10 +2284,15 @@ _ecore_evas_x_alpha_set(Ecore_Evas *ee, > >> >> >> >> > int alpha) { > >> >> >> >> > if (ee->in_async_render) > >> >> >> >> > { > >> >> >> >> > - ee->delayed.alpha = alpha; > >> >> >> >> > - ee->delayed.alpha_changed = EINA_TRUE; > >> >> >> >> > - return; > >> >> >> >> > + if (ee->visible) > >> >> >> >> > + { > >> >> >> >> > + ee->delayed.alpha = alpha; > >> >> >> >> > + ee->delayed.alpha_changed = EINA_TRUE; > >> >> >> >> > + return; > >> >> >> >> > + } > >> >> >> >> > } > >> >> >> >> > + if (ee->in_async_render) > >> >> >> >> > + evas_sync(ee->evas); > >> >> >> >> > >> >> >> >> Why? We're syncing just to apply the alpha for those not visible? > >> >> >> >> Your commit message is wrong because we are WAITING on this sync > >> >> >> >> call before the _alpha_do(). Thus it's almost the same as letting > >> >> >> >> the alpha be set the delayed way. Unless I'm missing anything > >> >> >> >> we're not gaining anything with this patch. > >> >> >> > > >> >> >> > no sync -> segv. sync -> no segv. (inside evas async rendering > >> >> >> > where an xob is null). since changing to alpha destroys and > >> >> >> > recreates the window id... not surprising. > >> >> >> > >> >> >> Do you have a backtrace? I did some quick testing but no segv for me. > >> >> > > >> >> > i did have one... it was in the async "flush" code - the one where the > >> >> > mainloop inline does its putimage to the window... but the xob was > >> >> > NULL (i didn't even check more than that because i instantly had the > >> >> > twinge that "aaaah i bet the xob creation failed due to invalid > >> >> > window id or something and thus why its null.. when the code is > >> >> > ASSUMING window id doesnt change during rendering"... thus simple - > >> >> > ensure rendering is done before we change window id. :) > >> >> > >> >> Window ID shouldn't change during rendering, that's the point of > >> >> having delayed operations. :-) Like Gustavo said I'm interested to > >> >> know what really was the problem because I don't think we should be > >> >> solving everything by adding evas_sync() calls everywhere. > >> > > >> > the window id does change. that's the core issue here. it is destroyed > >> > and re-created as that is the only way we can switch the visual of the > >> > window :) (given ecore-x). thus why i forced the "wait for async to > >> > finish"... :) also remember any properties set on the window before are > >> > then lost and we rely on ecore-evas to restore the ones it knows about > >> > (and has remembered to restore). > >> > >> *During* rendering it shouldn't, have you read what I wrote?! :-) > > > > yes - i read it... but it DOES ... if you make the changes and remove the > > deferred change in alpha and do it there and then. i made that change before > > any commit made it anywhere... and segv land it was. thus need to sync and > > wait so rendering is done - then window can be destroyed+created safely. > > It may be that only me was confused but just now I realize what you're > trying to do. And yes, if you don't defer the alpha change and don't > wait the rendering to finish it's really prone to crash. > > >> >> >> > if we delay UNCONDITIONALLY (which is what it did before the > >> >> >> > change - regardless of visibility) we WAIT until the window has > >> >> >> > already rendered something (which likely is after a show that came > >> >> >> > AFTER the set to alpha), and then now that we rendered and > >> >> >> > showed... we destroy and re-create again... when we could have > >> >> >> > happily just done it without delay/wait and avoided artifacts on > >> >> >> > invisible windows. :) > >> >> >> > >> >> >> I understood what you and Mike meant, thanks. But the way it works is > >> >> >> that if we are not async rendering it'll be done with no delay and if > >> >> >> we are async rendering then it'll be delayed until the "old" frame is > >> >> >> completely rendered and no new frames will be sent to render because > >> >> >> we actively drop frames! Thus I don't think that setting alpha would > >> >> >> destroy any new content rendered and showed but only old ones. Does > >> >> >> that make sense? > >> >> > > >> >> > the problem is we are messing with a window and its properties... and > >> >> > the code expects to mess with it there and then.. but it gets > >> >> > deferred until later .. and this leads to inconsistency issues. as > >> >> > best i saw a primary one is that UNTIL we render yet again the window > >> >> > stays hidden. (invisible). and our render was over already... > >> >> > >> >> Hmm.. Raster, please, we need to have this information better on > >> >> commits messages or elsewhere. I'm trying to make sure we fix things > >> >> the proper way but without information is really difficult to help. > >> >> :-/ > >> > > >> > i didnt think putting in a sync for a rare operation (like switching to > >> > an argb target window) would warrant a full essay on it... it's rather > >> > simple and minor > >> > >> Yes, it is. However, I still think the way to solve should be > >> different and we may be hiding the real bug as it seems you don't even > >> know yet how the async rendering code works. :-/ And it's not only on > >> this case, commit messages need to improve a lot to be useful later > >> and even for others to understand the real problem being fixed. If you > >> are solving a crash putting the backtrace in the commit message and an > >> explanation how to reproduce is good practice. > > > > actually i'm not fixing the crash - there was no crash UNTIL i started to > > fix a side-effect of the delayed alpha change. i'm fixing the fact that we > > have invisible override-redirect tooltip windows in elm because if they use > > alpha *SOMETIMES* they don't appear at all (they simply are not rendered). > > different people can or cannot reproduce this SOMETIMES on SOME systems. > > this is a end bi-product of the thing i changed/fixed above. deferring > > alpha change creates an intermittent bug of non-visible windows. this is a > > bi-product of delaying the change in window id. *IF* i just REMOVE the "if > > (ee->in_async_renderer)" section entirely and force it to change THEN (even > > if not visible yet) we GET the segv at that point - that code never made it > > to git. i spotted that (and it's been discussed here as a bi-product). > > > > delaying the _alpha_do until the last render leads to nothing being visible > > at all until something forces a reconfigure of the window (re-render, show > > etc. etc.). > > Thanks for explaining. Like I said I only understood now that the segv > was caused by your change to not defer the alpha change and what > you're trying to solve is the side-effect of delaying that on tooltip > windows in elm. > > > if you want me to find the root cause then it's SOMEWHERE between compositor > > AND efl or a combo of both, i have to debug the interaction of these 2 going > > VIA a 3rd process (xserver). i am happy to do this, if you are happy to > > wait a few more weeks or so until we release efl 1.8. i am sure it is > > totally worth sinking a few weeks of gory printf debugging into the event > > streams of e and efl on both sides of the fence and trawling the logs JUST > > to try keep the window del/add in the async handler rather than just simply > > do a "hey wait up - finish what you are doing in the bg for rendering THEN > > i'll re-do the window". this is by far the simplest solution. sure - it > > means a pause/wait. it's a pause in a relatively seldom code path and only > > once at window setup time. it USED to work. i took the easiest path to > > fixing it (for now). now this would affect pretty much any > > override-redirect or even non-override-redirect window in the same way, but > > by LUCK in most cases it seems to work, but not in the ones i saw. i am not > > sure as to the exact thing that has/is happening, and to figure it out > > would take a considerable extra investment in time and effort, neither of > > which i have the luxury of happening in abundance. > > I didn't mention you should be root causing, debugging or doing > whatever, I was trying to understand what you're really fixing. If we > had that in the commit message I wouldn't be asking, getting confused > and then finally understanding. Mike was really more accurate in his > explanation but I was somewhat distracted by your mention of a crash. > > >> I was trying to understand the bug being fixed and yes, it's minor and > >> I can just leave it as that. However, we need to improve the way we do > >> development as an open source project. It's kind of embarrassing that > >> we don't have proper usage of version control, good commit messages, > >> reviews, release schedule and so on. :-/ > > > > we have a release schedule. efl 1.8 was due middle of the year. what we > > have is no manpower to make it happen. everyone is busy doing work for > > their jobs. me included. for reasons of confidentiality i can't say more > > than that. it's out of my hands. the manpower simply is not there, but it > > WAS scheduled to be there when the original plan to release mid-year was > > made. there is no guarantee as to when/if that manpower will be available, > > so efl gets whatever side-time can be found. > > Yeah, I know that. Let's see if we can have the release now. > > > if you want to have every commit go through review... please just look at > > the review BACKLOG on phab for just submissions from those without commit > > access. > > > > https://phab.enlightenment.org/differential/query/c.8PlsTrzNjQ/ > > > > still open from april... and i haven't even covered the bug tasks filed that > > also require attention. > > > > what we have a compromise and have those with commit access skip > > "review-before-commit" and allow for post-commit reviews to catch things (ie > > assume that the vast majority of what they do is fine and very little bad > > stuff is going in) and those without to go through extra review. if you are > > offering up the manpower to clear out our above review backlog, AND > > https://phab.enlightenment.org/maniphest/query/open/ AND then add to that > > the manpower to do timely reviews of every commit that is done by devs with > > commit access (that's about 24 commits per day, every day, 365 days a year) > > do tell... > > > > also when i say review. i mean not just some rubber-stamping, but ACTUAL > > review (if it's just rubber stamping which i notice is what other "forced > > review" setups have become around me). if it's just a bureaucratic rubber > > stamp, then it's nothing more than a feel-good exercise in time-wasting as > > all reviews are approved without any ACTUAL review. > > > > if you are offering that manpower (estimated 5 fulltime people indefinitely > > into the future doing NO WORK AT ALL other than review), then i'm all-ears. > > as such our current setup is "review before commit for people not trusted > > yet" and "review voluntarily after commit for those who are". stating we > > have no review setup is plainly false. what we have is one that you don't > > like because it's not strict enough to force EVERYONE through review first, > > and to do that we will need more people OR to sacrifice that amount of > > manpower that COULD be spent on working on efl instead. > > > > (my guess is there might end up an average of maybe 1h of work per commit to > > ACTUALLY really read it and check it for issues - some are trivial no > > brainers or 2-3 mins of work, others may take multiple hours - eg like > > jean-philippe's merge where each commit has to be seen within the context > > of previous commits in history - so if you can find an ADDITIONAL 5 > > full-time people to do NOTHING ELSE but review just for the existing commit > > rate, assuming they work 4h/week but with an overhead of 1h/day of other > > stuff so 35h/week available). > > I wasn't completely fair. We've been improving on some aspects but we > need to continue. As for the ACTUAL versus rubber-stamping review I > also agree. And yes, we need more manpower to implement the kind of > review you mentioned and we don't have that. If we don't have more > people in this community we won't ever have that so we need to improve > what we have. sure. what we have now is a compromise that works with the manpower we have. if and as time allows we'll adapt and do more. we already have done a lot more. stefan commented on that. :) we have started to formally group-review submitted patches via phab. it's a lot better than things were. i personally think that rather than focus on reviews and scm's at this stage we should work better on documentation and tools. :) that will help bring people in.. and thus make space for manpower... :) -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ Android is increasing in popularity, but the open development platform that developers love is also attractive to malware creators. Download this white paper to learn more about secure code signing practices that can help keep Android apps secure. http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
