On Fri, Jun 20, 2008 at 2:44 PM, Gustavo Sverzut Barbieri <[EMAIL PROTECTED]> wrote: > On Fri, Jun 20, 2008 at 5:55 AM, Cedric BAIL <[EMAIL PROTECTED]> wrote: >> On Thu, Jun 19, 2008 at 7:28 PM, Gustavo Sverzut Barbieri >> <[EMAIL PROTECTED]> wrote: >>> On Thu, Jun 19, 2008 at 1:57 PM, Cedric BAIL <[EMAIL PROTECTED]> wrote: >>>> On Thu, Jun 19, 2008 at 6:48 PM, Gustavo Sverzut Barbieri >>>> <[EMAIL PROTECTED]> wrote: >>>>> On Thu, Jun 19, 2008 at 1:34 PM, Cedric BAIL <[EMAIL PROTECTED]> wrote: >>>>>> On Thu, Jun 19, 2008 at 3:13 PM, Gustavo Sverzut Barbieri >>>>>> <[EMAIL PROTECTED]> wrote: >>>>>>> On Thu, Jun 19, 2008 at 6:15 AM, Cedric BAIL <[EMAIL PROTECTED]> wrote: >>>>>>>> On Wed, Jun 18, 2008 at 7:44 PM, Gustavo Sverzut Barbieri >>>>>>>> <[EMAIL PROTECTED]> wrote: >>>>>>>>> On Wed, Jun 18, 2008 at 1:53 PM, Cedric BAIL <[EMAIL PROTECTED]> >>>>>>>>> wrote: >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> Here is a patch that add support for background preloading of a data >>>>>>>>>> image. You can now, if you know what you do, ask evas to load the >>>>>>>>>> data >>>>>>>>>> of an image in memory before it is displayed (look at the sample code >>>>>>>>>> for expedite). >>>>>>>>>> >>>>>>>>>> The code is quite simple, we have now a work queue where we add all >>>>>>>>>> the Image_Entry we need to preload. They are processed one after the >>>>>>>>>> other in another thread. When it's done, it notify evas main thread >>>>>>>>>> with evas async API and work on the next Image_Entry. >>>>>>>>>> >>>>>>>>>> This means that we now require that engine surface creation and >>>>>>>>>> loader should be thread safe. It's currently the case for all the >>>>>>>>>> engine I know something about, so this should not break anything. >>>>>>>>>> Anyway this code is optionnal and could be completely unactivated in >>>>>>>>>> a >>>>>>>>>> per engine basis or globally. >>>>>>>>>> >>>>>>>>>> As always have fun reviewing this patch. >>>>>>>>> >>>>>>>>> so here it goes: >>>>>>>>> >>>>>>>>> configure.in: $build_async_preload should depend on async_events, no? >>>>>>>> >>>>>>>> You are right. >>>>>>>> >>>>>>>>> - EAPI Evas_Bool evas_async_events_put (void >>>>>>>>> *target, >>>>>>>>> Evas_Callback_Type type, void *event_info, void (*func)(void *target, >>>>>>>>> Evas_Callback_Type type, void *event_info)); >>>>>>>>> + EAPI Evas_Bool evas_async_events_put (const >>>>>>>>> void >>>>>>>>> *target, Evas_Callback_Type type, void *event_info, void (*func)(void >>>>>>>>> *target, Evas_Callback_Type type, void *event_info)); >>>>>>>> >>>>>>>>> you made target const, but not in the callback... this is strange. >>>>>>>> >>>>>>>> Well in most place in the EFL, when a function doesn't free/modify an >>>>>>>> object we mark it as const even if some outside fonction could modify >>>>>>>> it. I don't have a hard opinion on this subject, so if you prefer I >>>>>>>> will remove the const stuff. >>>>>>>> >>>>>>>>> As for the rest of the code. I told you at IRC, but you insist to not >>>>>>>>> listen to me, so the review is: do not use mutex or cond, use a pipe! >>>>>>>> >>>>>>>> Lol ! Sorry but the code was already written, my problem was just >>>>>>>> thread priority at that time :-) So I didn't want to rewrite it as I >>>>>>>> already have something working. >>>>>>>> >>>>>>>> And regarding pipe, I just see one problem using them, we will not be >>>>>>>> able to latter add the possibility to reprioritise the loading of >>>>>>>> image. This could be usefull in the case of an image viewer I believe. >>>>>>> >>>>>>> You can even implement this, instead of reading and processing it >>>>>>> directly, you read and append to some worker-only list, sorted by >>>>>>> priority (another member of work-info), then you process in that order >>>>>>> and you pipe back to main thread as usual... still no locks, each >>>>>>> thread is safe, etc... >>>>>>> >>>>>>> >>>>>>>> I also don't understand why you dislike so much mutex and cond stuff. >>>>>>> >>>>>>> because they're useless in this case. We should not have to have a >>>>>>> critical section where just one needs to operate. Each thread should >>>>>>> have its own list/queue, just the main thread should allocate/free the >>>>>>> work-info pointer. >>>>>> >>>>>> I disagree, you will always need a lock inside >>>>>> evas_cache_image_load_data. Or you will have a race condition with the >>>>>> loader itself. If you are unlucky, you could have just decided to >>>>>> start loading a image, but you don't have yet the time to change the >>>>>> state to PROCESS. So in the main evas thread, you cancel it and you >>>>>> start to load the data, in the worker thread, you just change to >>>>>> PROCESS and you start to load the data. This end-up, in the best case, >>>>>> with just some memory leak. So you need in the critical path to check >>>>>> this. It's why I did put mine and it's not locking if you don't have a >>>>>> worker thread in my case. >>>>>> >>>>>> I don't see how I can go around that without lock. >>>>> >>>>> You shouldn't have that case, because if it's loading on the thread, >>>>> makes no sense to load on the main thread. So if you want to handle >>>>> such thing, you can have atomic operations on such flag. >>>> >>>> Preloading is done in the background when requested. But loading stay >>>> in the main thread, we don't want to delay and slow down all call to >>>> image_load_data (remember it's in the critical path of all call to >>>> image_draw, and image_draw of all engine assume that the data are >>>> ready after this call). >>> >>> Which slow down would be this? You need to check if it's loading >>> anyway. this is an atomic operation on a flag. If it's being >>> processed, then you wait there, otherwise you just flag it as >>> canceled. >> >> I don't know, but if the worker thread is processing something, we >> will need to wait for it to be ready. If the image as not been asked >> to the worker thread, we will need to allocate and send a message. The >> worker thread should then read it and process it. This is the common >> case currently. So comparing this to checking if a the worker thread >> is doing something and only in that case remove the entry if possible >> or wait for it to be ready, I do believe that this should be a faster >> way of doing thing. And this patch doesn't impact expedite speed. >> >> Perhaps I didn't understand what you wanted to do in the case of >> loading data. You could perhaps explain this stuff in the pseudo code >> like you did for the rest, it was really easier to understand. > > hum, which part of loading data? > > what I said is that if you want to force data loading at some time, > for example, you want to get data/pixels, then you change the priority > and block (maybe on read(2)), waiting the thread to return loaded > data. Here you have 2 cases: > 1 - image was being loaded: you'll block the main thread and just > the worker/loader thread would run, so same as we have now, it should > run as fast, because one thread is not doing any work (sleep on read) > while the other is doing the image loading/decoder/... > 2 - other image was being loaded, after it finish it will be piped > to the main thread, then the required image will be processed, because > of higher priority, this is back to case 1. > > IMO, waiting one image to finish loading is totally acceptable, it > usually will not take much more and we avoid problems with partial > loading or restarting loading later.
Ok, I understand clearly where we have a point. In my opinion we should not delay pixel loading as they are in the critical path (called on each frame rendering). So I always planned a way to do this from the main thread so we never delay them, just slow the things down a bit (That's why I wanted to raise the priority of the main thread). I must add that for a JPEG of 640x480 it took me 2s to decompress on my set top box, so delaying the load of anything could be a bit problematic. > Also, just need to take care in 2 to dispatch the signal of the image > that was loading. This can be dispatched in idler, so after the > current image, which was blocked, is done. > > This solution is much cleaner and easier to me. I can't see any race > condition and any need for mutexes/cond. You just need to write a > small functions to handle communication from thread and other to > dispatch loaded signals. This way you can reuse these from the 2 > points where you need it, one in fd-handler and the other in the > blocking functions, like _data_get/forced load. > > > As for: > >> And this patch doesn't impact expedite speed. > > how would it? Expedite loads a handful of images and reuse them, this > is not a good test case. Ok, I see you did a test to see it was not a > problem with existing code, but it would be good to write a image > viewer that we could launch on a folder with huge pictures, even more > on slow devices... I'm sure Canola2 guys would be able to test that > and remove lots of hacks from their image viewer. Depends on the speed of your device :-) In my case, decompressing the image cost me 5% in first expedite test (difference between coldstart and with data loaded). And I win this with preload. With a score around 3, any variation is noticable and the number are more stable than on a my PC. On the PC, I can't say it has any impact on the speed. Any way, it will be better to test this patch with an image viewer that use the new API. -- Cedric BAIL ------------------------------------------------------------------------- Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
