GNOME Bugzilla – Bug 629322
Implement partial rendering of pages to decrease memory usage.
Last modified: 2018-05-22 14:00:49 UTC
This is a bug to track development of the refactoring needed in Evince to allow partial rendering of pages using a technique similar to tiled rendering. This is not tiled rendering in the strict sense, since the idea I have is to make tiles of size roughly the screen, so we don't use too much memory when viewing a Pdf with a big zoom (say 1600%).
Created attachment 169997 [details] [review] [libview] Remove unused bin_window variable.
Carlos, the above is a trivial patch that could go in (IMO) in master. I would also want to get opinion/advise on the approach. First goal: before implementing the partial rendering in a new ev-pixbuf-cache, I want to isolate EvView from knowing whether the pixbuf cache does partial rendering or not (or tile rendering or whatever), so that we can fix a new API in ev-pixbuf-cache after which all refactoring to implement partial/tiled rendering only happens in ev-pixbuf-cache. I really want to commit to this goal so we can make the refactoring easier to review and merge. What do you think? To achieve the above goal I see (at least) two options: 1. I move most of the logic of draw_one_page in ev-view.c to a ev_pixbuf_cache_render_page. 2. Change the semantics of ev_pixbuf_cache_get_surface to return exactly a surface with the exact portion of the page which is visible (adding a page_area to the arguments). I like 1 more since I don't really know how to implement 2 without copying appropriated portions from cache surfaces to a "buffer"-like target surface returned by the method. I also fear that this extra copy in each expose event will affect performance. So what do you think? Second goal: To have partial/tiled rendering, we will need several EvJobRenders per page, so rendering selections in the EvJobRender seems a little awkard to me. I would propose to create a new EvJobRenderSelection to deal with the async render of selections. This can also be reviewed/merged before the refactoring on ev-pixbuf-cache. Finally, if we achieve the first goal, we can have ev_pixbuf_cache and ev_tile_cache with the same api, so we can use an interface or a base gobject and add a dconf setting to which "render cache" to use. This would allows to have both implementations living for a while together and would also allows to track regression and performance of the new implementation.
Review of attachment 169997 [details] [review]: Push it please.
(In reply to comment #2) > Carlos, the above is a trivial patch that could go in (IMO) in master. I would > also want to get opinion/advise on the approach. > > First goal: before implementing the partial rendering in a new ev-pixbuf-cache, > I want to isolate EvView from knowing whether the pixbuf cache does partial > rendering or not (or tile rendering or whatever), so that we can fix a new API > in ev-pixbuf-cache after which all refactoring to implement partial/tiled > rendering only happens in ev-pixbuf-cache. > > I really want to commit to this goal so we can make the refactoring easier to > review and merge. What do you think? Well, my idea was actually the opposite, I think the cache should be that, a cache, and shouldn't render anything. The view is who knows what it needs to be rendered, and might use a cache to save what has been rendered. For tiled rendering we would have a tile manager in addition to the cache, where not using tiled rendering is just the special case tile size == page size. > > Second goal: To have partial/tiled rendering, we will need several EvJobRenders > per page, so rendering selections in the EvJobRender seems a little awkard to > me. I would propose to create a new EvJobRenderSelection to deal with the async > render of selections. This can also be reviewed/merged before the refactoring > on ev-pixbuf-cache. hmm, maybe we can still use a job render per page and render all the tiles of that page in the same job. Remeber that we only use a worker thread, so having too many jobs might be slower that having fewer jobs doing more work.
Created attachment 229041 [details] [review] Make the Pixbuf use HashTable internally Carlos, I just decided to restart from zero on this. Here is a patch that changes the pixbuf_cache to use a hashtable instead of the cache_job_info arrays. The main reason to change to use hashtable is flexibility. For the moment, I tried hard of not changing API neither functionality of the pixbuf_cache, but in the future, the idea would be to change the keys in the table so we can have keys like "page=1,scale=200%,rotation=0,tile=2,1" to implement tiling rendering in a way that is transparent to the PixbufCache. AFAIK, it works, and I haven't experienced any problems in my testing. Could you please review this when you get the chance.
(In reply to comment #5) > Created an attachment (id=229041) [details] [review] > Make the Pixbuf use HashTable internally > > Carlos, I just decided to restart from zero on this. Here is a patch that > changes the pixbuf_cache to use a hashtable instead of the cache_job_info > arrays. > > The main reason to change to use hashtable is flexibility. For the moment, I > tried hard of not changing API neither functionality of the pixbuf_cache, but > in the future, the idea would be to change the keys in the table so we can have > keys like "page=1,scale=200%,rotation=0,tile=2,1" I don't think we want to cache tiles for different scale or orientation. > to implement tiling rendering > in a way that is transparent to the PixbufCache. I doesn't need to be transparent for the pixbuf cache, at least not if it means the code will be more complex. > AFAIK, it works, and I haven't > experienced any problems in my testing. Could you please review this when you > get the chance. I don't see the point of this patch by itself, maybe it would be better to work on a branch where you can push commits, so that it's easier to try it out and to see the relationship between commits.
(In reply to comment #6) > (In reply to comment #5) > > Created an attachment (id=229041) [details] [review] [details] [review] > > Make the Pixbuf use HashTable internally > > > > Carlos, I just decided to restart from zero on this. Here is a patch that > > changes the pixbuf_cache to use a hashtable instead of the cache_job_info > > arrays. > > > > The main reason to change to use hashtable is flexibility. For the moment, I > > tried hard of not changing API neither functionality of the pixbuf_cache, but > > in the future, the idea would be to change the keys in the table so we can have > > keys like "page=1,scale=200%,rotation=0,tile=2,1" > > I don't think we want to cache tiles for different scale or orientation. > > > to implement tiling rendering > > in a way that is transparent to the PixbufCache. > > I doesn't need to be transparent for the pixbuf cache, at least not if it means > the code will be more complex. > > > AFAIK, it works, and I haven't > > experienced any problems in my testing. Could you please review this when you > > get the chance. > > I don't see the point of this patch by itself, maybe it would be better to work > on a branch where you can push commits, so that it's easier to try it out and > to see the relationship between commits. Sure, the patch on its own is not valuable, except that to my eyes the code is less complex ;) Anyway, I will try to get more work done and once I have some basic tiling rendering working, I will publish a wip branch, so we can discuss with some complete code under our eyes! Thanks
Ok, so I finally got something working... I pushed my work so far into the github.com/jaliste/evince in the wip/tiling_manager branch. I didn't want to use gnome git server as it is still not very clean (mainly ev-view.c and ev-pixbuf-cache.c have still a lot of commented code), so I expect to rebase it a lot. I would be helpful if you can take a look at it and give general comments on the approach. I finally went to by implementing the hash_table version of the PixbufCache, and then modified the Keys in the hashtable so Pixbufcache would cache tiles instead of pages. The number of tiles is controlled by pixbuf_cache->tile_level, which is the number of divisions (horizontal and vertical) to get tiles out of pages, i.e., each page is divided into tile_level^2 tiles. To compute tile_level, I added a ev_view_set_max_tile_size func, and then the tile_level is computed each time the scaled is changed, the idea is that we only use tiles of a size which is similar to screen size, so you don't get to draw so many tiles (because as we know this will be slow, as poppler reparses data for each tile render) The main benefit of this implementation so far is that now we can activate 1600% zoom without much hassle, I could see the pdf_reference pdf using around 150M at 1600%! Things still to do: 1) dual_mode, 2) selections, 3) refactor pixbuf_cache back to CacheJobInfo arrays. About 3); I am still not sure that changing to hash_table was a good idea, but it certainly made my way out of a tile based pixbuf_cache easier. So, after I have things more or less cleaned up (with a proper/stable API defined) I will look again to see if I can avoid this conversion.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/evince/issues/175.