[zathura] If the page is not cached, then this implies that it doesn't have a surface

[Marwan Tanager]

Hi, Sebastian.

In commit b5237680f28d585fecf1c6d23bc7ec044e623ae1, you provide the following
check:

"if the page is not visible and not cached, but still has a surface, we need to
get rid of the surface"

Wouldn't that be redundant? I mean that if the page is not visible and not
cached, then we have two possibilities:

1. It was evicted earlier from the cache, which implies that it doesn't
have a surface now, since the cache code already takes care of
destroying it's surface once it chooses to evict it.

2. It hasn't been rendered yet.

So, in both cases, the page doesn't have a surface.

Frankly, I would prefer zathura_page_cache_is_cached to be static, and only
expose zathura_page_cache_add, because I think we don't need to concern
external code with such details (just add pages to the page cache, and it will
then take care of the rest; that's how I thought of the code when implementing
it).


--
Marwan

[Sebastian Ramacher]

Hi

Post by Marwan Tanager
In commit b5237680f28d585fecf1c6d23bc7ec044e623ae1, you provide the following
"if the page is not visible and not cached, but still has a surface, we need to
get rid of the surface"
Wouldn't that be redundant? I mean that if the page is not visible and not
1. It was evicted earlier from the cache, which implies that it doesn't
have a surface now, since the cache code already takes care of
destroying it's surface once it chooses to evict it.
2. It hasn't been rendered yet.
So, in both cases, the page doesn't have a surface.
Frankly, I would prefer zathura_page_cache_is_cached to be static, and only
expose zathura_page_cache_add, because I think we don't need to concern
external code with such details (just add pages to the page cache, and it will
then take care of the rest; that's how I thought of the code when implementing
it).

The thing is that we need to know if a page is cached or not. Consider
the following scenario: the user scrolls through the document at a very
fast pace and the render thread doesn't keep up with the requests. This
can happen very easily in documents with large images, for example. Now
the following things can happen:

1. the render thread renders the page and the page is still visible.
2. the render thread renders the page and the page is not visible
anymore but still cached.
3. the render thread renders the page and the page is not visible
anymore but also not cached.

Everything works as expected in the cases 1. and 2., but in case 3. we
kepp the page in memory altough it shouldn't. The problem is that at the
time the cache tries to clear the surface, the widget doesn't have a
surface so there is nothing to destroy.

The check has been added in two places. It's probably unnecessary in the
scroll callback, but I haven't tested that.

There are of course other ways to fix this. One of them would be to
abort the rendering of the page if it becomes invisible and leaves the
cache. However, implementing the necessary message passing for this is
less trivial than just exposing this one function to check if a page is
cached or not.

Regards

[Marwan Tanager]

Post by Sebastian Ramacher
The thing is that we need to know if a page is cached or not. Consider
the following scenario: the user scrolls through the document at a very
fast pace and the render thread doesn't keep up with the requests. This
can happen very easily in documents with large images, for example. Now
1. the render thread renders the page and the page is still visible.
2. the render thread renders the page and the page is not visible
anymore but still cached.
3. the render thread renders the page and the page is not visible
anymore but also not cached.
Everything works as expected in the cases 1. and 2., but in case 3. we
kepp the page in memory altough it shouldn't. The problem is that at the
time the cache tries to clear the surface, the widget doesn't have a
surface so there is nothing to destroy.

Hmmm. you're absolutely right about that.

Post by Sebastian Ramacher
The check has been added in two places. It's probably unnecessary in the
scroll callback, but I haven't tested that.
There are of course other ways to fix this. One of them would be to
abort the rendering of the page if it becomes invisible and leaves the
cache. However, implementing the necessary message passing for this is
less trivial than just exposing this one function to check if a page is
cached or not.

Yes, based on the above reasoning, using zathura_page_cache_is_cached is indeed
a lot easier than trying to synchronize the two threads.

In this case, I have to admit that I haven't seen the forest for the trees :)

I think one of the biggest issues about Zathrua is the sometimes out-of-sync
things like this, that happen as a result of the rendering thread not keeping
up with rendering pages that are heavy on graphics, but as you said, fixing
this requires a great deal of work. (hopefully, one of us would find the time
someday to sit and fix things up).


--
Marwan

[Sebastian Ramacher]

Post by Sebastian Ramacher
There are of course other ways to fix this. One of them would be to
abort the rendering of the page if it becomes invisible and leaves the
cache. However, implementing the necessary message passing for this is
less trivial than just exposing this one function to check if a page is
cached or not.

I'm currently working on a different render infrastructure in the
features/renderer-take-2 branch. The setup is currently the following:

- There is a ZathuraRenderer object that renders a page in a separate
thread.
- Every page widget holds a ZathuraRenderRequest object to manage
render requests. Widgets use this object to request a page to be
renderer and can also abort an request.
- ZathuraRenderRequest emits a signal as soon as the page is ready.

There are somethings that are not done yet. The sorting of the render
requests is currently broken.

In the end I hope it's enough to cancel pending requests in the caching
code and remove the workaround again.

Regards

[Marwan Tanager]

Post by Sebastian Ramacher

Post by Sebastian Ramacher
There are of course other ways to fix this. One of them would be to
abort the rendering of the page if it becomes invisible and leaves the
cache. However, implementing the necessary message passing for this is
less trivial than just exposing this one function to check if a page is
cached or not.

I'm currently working on a different render infrastructure in the
- There is a ZathuraRenderer object that renders a page in a separate
thread.
- Every page widget holds a ZathuraRenderRequest object to manage
render requests. Widgets use this object to request a page to be
renderer and can also abort an request.
- ZathuraRenderRequest emits a signal as soon as the page is ready.

This sounds very cool! Nice work, Sebastian.

Post by Sebastian Ramacher
There are somethings that are not done yet. The sorting of the render
requests is currently broken.

I'm going to have a look at those rough edges at the nearest opportunity
(unless, of course, you reached there first!).

Post by Sebastian Ramacher
In the end I hope it's enough to cancel pending requests in the caching
code and remove the workaround again.

Yes, once the infrastructure is in place, this is going to be a matter of a
trivial check. Great work, man.


--
Marwan

[Sebastian Ramacher]

Post by Marwan Tanager

Post by Sebastian Ramacher

Post by Sebastian Ramacher
There are of course other ways to fix this. One of them would be to
abort the rendering of the page if it becomes invisible and leaves the
cache. However, implementing the necessary message passing for this is
less trivial than just exposing this one function to check if a page is
cached or not.

I'm currently working on a different render infrastructure in the
- There is a ZathuraRenderer object that renders a page in a separate
thread.
- Every page widget holds a ZathuraRenderRequest object to manage
render requests. Widgets use this object to request a page to be
renderer and can also abort an request.
- ZathuraRenderRequest emits a signal as soon as the page is ready.

This sounds very cool! Nice work, Sebastian.

Post by Sebastian Ramacher
There are somethings that are not done yet. The sorting of the render
requests is currently broken.

I'm going to have a look at those rough edges at the nearest opportunity
(unless, of course, you reached there first!).

The sorting should work again.

Post by Marwan Tanager

Post by Sebastian Ramacher
In the end I hope it's enough to cancel pending requests in the caching
code and remove the workaround again.

Yes, once the infrastructure is in place, this is going to be a matter of a
trivial check. Great work, man.
--
Marwan
_______________________________________________
zathura mailing list
zathura at lists.pwmt.org
http://lists.pwmt.org/mailman/listinfo/zathura

[Marwan Tanager]

Post by Sebastian Ramacher

Post by Marwan Tanager

Post by Sebastian Ramacher
There are somethings that are not done yet. The sorting of the render
requests is currently broken.

I'm going to have a look at those rough edges at the nearest opportunity
(unless, of course, you reached there first!).

The sorting should work again.

Good to know.


--
Marwan

[Sebastian Ramacher]

Post by Sebastian Ramacher

Post by Sebastian Ramacher
There are of course other ways to fix this. One of them would be to
abort the rendering of the page if it becomes invisible and leaves the
cache. However, implementing the necessary message passing for this is
less trivial than just exposing this one function to check if a page is
cached or not.

I'm currently working on a different render infrastructure in the
- There is a ZathuraRenderer object that renders a page in a separate
thread.
- Every page widget holds a ZathuraRenderRequest object to manage
render requests. Widgets use this object to request a page to be
renderer and can also abort an request.
- ZathuraRenderRequest emits a signal as soon as the page is ready.
There are somethings that are not done yet. The sorting of the render
requests is currently broken.
In the end I hope it's enough to cancel pending requests in the caching
code and remove the workaround again.

I've worked on this a bit the last few days and finally merged into develop.
The caching logic is now in ZathuraRenderer and pages are notified via their
associated ZathuraRenderRequest if they are cached or got removed from the
cache. Pages will now release the surface themselves once they are not cached
and not visible.

There are still some workarounds that I'm not fully happy with and I'll probably
end up implementing some signals for the visibility stuff to make things easier.

Feedback is very welcome.

Regards