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