From: Gustavo S. B. <bar...@pr...> - 2008-06-20 12:44:52
|
On Fri, Jun 20, 2008 at 5:55 AM, Cedric BAIL <ced...@fr...> wrote: > On Thu, Jun 19, 2008 at 7:28 PM, Gustavo Sverzut Barbieri > <bar...@pr...> wrote: >> On Thu, Jun 19, 2008 at 1:57 PM, Cedric BAIL <ced...@fr...> wrote: >>> On Thu, Jun 19, 2008 at 6:48 PM, Gustavo Sverzut Barbieri >>> <bar...@pr...> wrote: >>>> On Thu, Jun 19, 2008 at 1:34 PM, Cedric BAIL <ced...@fr...> wrote: >>>>> On Thu, Jun 19, 2008 at 3:13 PM, Gustavo Sverzut Barbieri >>>>> <bar...@pr...> wrote: >>>>>> On Thu, Jun 19, 2008 at 6:15 AM, Cedric BAIL <ced...@fr...> wrote: >>>>>>> On Wed, Jun 18, 2008 at 7:44 PM, Gustavo Sverzut Barbieri >>>>>>> <bar...@pr...> wrote: >>>>>>>> On Wed, Jun 18, 2008 at 1:53 PM, Cedric BAIL <ced...@fr...> 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. 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. -- Gustavo Sverzut Barbieri http://profusion.mobi embedded systems -------------------------------------- MSN: bar...@gm... Skype: gsbarbieri Mobile: +55 (19) 9225-2202 |