From: Gustavo S. B. <bar...@pr...> - 2008-06-20 14:29:42
|
On Fri, Jun 20, 2008 at 10:52 AM, Cedric BAIL <ced...@fr...> wrote: > On Fri, Jun 20, 2008 at 2:44 PM, Gustavo Sverzut Barbieri > <bar...@pr...> wrote: >> 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. > > 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 guess we're talking about different things. My understanding is that we'll use this async loading for images we consider heavy, like big pictures or things we don't have the need to show immediately, like thumbnails in file manager, Canola, ephoto... So with these use cases in mind, when we should need to force some image that was previously requested to be async loaded to be available immediately? 1 - Maybe we don't even need to handle such case, users should always wait for "loaded" event callback and doing anything before that will return NULL. 2 - Maybe we need to force it during image_data_get(). In this case we do that check and block. Regular render path is almost the same, we just need to check if the image data is not available and skip it, or we can even do as browsers and allocate the buffer upstart, each line/block we decode from thread we mark object as dirty and canvas will redraw it. (Note: I think this is useless, but someone might like partial renderings). > 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. I know, n8x0 are also very slow and we have lots of problems, we had to load each thumbnail in its own idler so we don't block the main view too much... if we had this flag, it would be much easier :-) >> 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. Well, I need to check your changes to expedite. If you requested the image to be loaded before, then yes, makes sense, but if you're requesting test to start and then async load images, you're cheating, because in the first test frames you do not have the image data, so no operation :-) -- Gustavo Sverzut Barbieri http://profusion.mobi embedded systems -------------------------------------- MSN: bar...@gm... Skype: gsbarbieri Mobile: +55 (19) 9225-2202 |