Re: New tile unref logic and various leak fixes

[Jiří Techet <techet gmail com>]

On Tue, Mar 23, 2010 at 15:40, Pierre-Luc Beaudoin
<pierre-luc pierlux com> wrote:
>> Concerning ClutterGroup inheritance I agree in principle - when we
>> inherit from it, we inherit also all the container methods, which,
>> however, shouldn't be used by the user. On the other hand, it's quite
>> a lot of work to do and I'd postpone the correct implementation for
>> 0.8. And in fact, I haven't introduced this to libchamplain - it was
>> already there - the following classes were derived from ChamplainGroup
>> before my changes:
>>
>> ChamplainBaseMarker
>> ChamplainView
>> ChamplainLayer
>>
>> So I'll change my classes after you fix your classes ;-)
>
> I never made the changes because there were IMHO no clear documentation
> on how do do it at the time.  The "Implementing a new actor" section of
> the Clutter Reference Documentation seems to have all it takes.

I haven't seen any documentation myself either. But we can just have a
look how ClutterGroup is implemented. Also something similar is done
in TidyScrollView - see tidy_scroll_view_add_actor(). Or is there any
easier way how to implement it? In fact, we don't need to implement
the whole ClutterContainer interface - for instance for tiles we don't
want that the user inserts new actors to it from the outside - we just
need to do this from the inside of the class.

>>
>> Not really, I have tried it. the reason is that the speed of go_to()
>> depends on the distance where you go - and with gps, the distance on
>> the map is usually small, so the speed is low as well. This means that
>> the point starts moving from the center until it gets far enough to
>> reach equilibrium so the speed equals the speed of the point - that's
>> not very nice though. We'd need something like go_to_with_speed() to
>> make it look good. For GPS navigation I also wanted to avoid problems
>> when you don't have signal for a while so the point gets stuck on one
>> place and then, when you get the signal again 100km far from the place
>> where you lost the signal with go_to _all_ the tiles in between would
>> have to be filled - a bit problematic.
>
> go_to's math should be revisited.  The intention was that if the
> distance is far, it doesn't take an hour to get there.  Somehow the time
> should be passed to the function, leaving that for the caller to decide.
>

I take it back, I misunderstood the comment in the function description:

 * Move from the current position to these coordinates. All tiles in the
 * intermediate view WILL be loaded!

I thought that all the tiles between the two points will be
downloaded, but it's clearly not the case after looking at the code.
This will happen only for the tiles that are located within the
current frame position.

I haven't tested it, but I think it won't work well for longer
distances - if the distance is long enough, for every frame you'll
have to download all the tiles, but before this happens, new frame
will come so you won't see anything. Anyway, I think this function is
meant for smaller distances only, where it seems to do its job.

As I said, I think that there should be also some go_to_with_speed()
function and the user code can then decide which of them to use
(center_on() when the distance is big to avoid tile download between
the points, go_to() otherwise).

>> By the way, do you think it would be possible to put some traces into
>> ChamplainView? What I did was that I put printf("%s", __FUNCTION__);
>> at the beginning of every function there - it's very helpful for
>> debugging - especially with signals and problems like the complicated
>> one above. I did this about five times now - inserting this trace for
>> every function and then removing for the final commit - a little
>> boring. Having some macro permanently present in every function would
>> be nice.
>
> I wouldn't mind it if it's only built in DEBUG :)

Sure. I think it should even be disabled by default when you compile
in debug mode - there are hundreds of functions called and you don't
want to see them normally when developing - but these traces are very
useful under some specific occasions. So what I'd like to do is
something like this:

//#define TRACE_VIEW
#ifdef TRACE_VIEW
#define TRACE_MSG() printf ("%s", __FUNCTION__);
#else
#define TRACE_MSG() ;
#endif

and put TRACE_MSG() at the beginning of every function. If someone
needs the traces, all he has to do is to uncomment the first line,
otherwise the macro is empty.

>
>> I still wonder about the memory problem. One thing is clear - it's not
>> because the tile isn't deallocated. I've added one more optimization
>> (based on oprofile output), where view_update_state() consumed too
>> much time because it had to go through all the tiles every time tile
>> changed its state. I'm now just keeping a counter of tiles that are
>> being loaded and when their number drops to 0, the state changes to
>> DONE. I do this by connecting to the tile's destroy signal so as long
>> as the state indication works, we can be sure that all tiles are
>> deallocated - quite a good leak test as well :-).
>>
>> If we are not leaking tiles, we might still be leaking something else.
>> I'll make some simple test program that checks whether these leaks
>> appear only when using clutter textures. Personally I suspect that
>> this:
>>
>> https://bugs.freedesktop.org/show_bug.cgi?id=23530
>>
>> didn't get to Karmic, which I use. Will try after Jaunty is out. What
>> graphics card and linux distribution do you use?
>
> I have Karmic right now.
>

Good news here. I tested test-textures from "interactive" clutter
tests and it suffers the same problem under Karmic. Then I booted
Lucid beta from CD, installed clutter prerequisites, compiled clutter
and ran the same demo - no leaks there. The future looks bright...

>>
>> This worked when filling tiles was synchronous - after calling
>> view_load_visible_tiles() you could be sure that all the tiles are
>> loaded and swapping the old screen with the new screen did what you
>> expected. With asynchronous tile loading it's different. When
>> view_load_visible_tiles() finishes, _no_ tiles have been loaded yet so
>> new_screen is empty when you swap the screens and you get the "black
>> out". I don't see any simple solution how it fix it with asynchronous
>> tile loading (which speeds up things a lot, so I wouldn't sacrifice it
>> in order to get the original behaviour). Well, actually I do. Use
>> white (or other light) background instead of black background like
>> launcher demo does. Instead of "black out" you get "white out", which
>> is much less disturbing because the maps are light too. Is it possible
>> to change the background colour in Champlain-gtk? Would be nice to
>> have.
>
> But we could still delay the removal of already loaded tiles until the
> new ones have been displayed. Sure there would be an initial black out
> on start up, but as you said we could use another colour such as the Gtk
> + background colour for widgets.
>

I made a simple demonstration how this could work here:

http://gitorious.org/~techy/libchamplain/flicker-free-zoom-clone

The implementation is not correct - there are many cases where this
causes problems so don't apply it (I don't move the shadow layer to
correct position, it is drawn over the markers layer and there are
other problems). It works reasonably with the launcher demo with the
default view.

The main problem I see with this kind of implementation is that when
loading of some tile gets stuck (e.g. from network tile source), the
tile from the previous zoom level remains in the background, which
looks very ugly. One solution might be that tiles will have a special
signal associated to them that is signalled when a "slow map source"
(such as network map source) loads the tile. Then the view could
register for this signal and remove the shadow layer completely so
once something starts downloading from the network, the background
becomes black. This kind of simulates the previous behaviour of
libchamplain.

The second main problem I see is that I'm not motivated enough to
implement this - I'm fine with how it works now - so it will be up to
you if you want this feature ;-).

> I reviewed all of your changes and I only have one comment about the new
> fade in code: it should be possible to disable it for Maemo devices.
> Fade-in is really CPU intensive on the N900 and I disable it in the
> builds.  I'll make that change official using the MAEMO defines.

OK.

>
> Thanks for your changes.  Make sure to continue to work from the latest
> version (with your changes!).  If you do not oppose, I'd make a 0.5.2
> release with your changes this week.

Great!

I have one comment regarding MemphisTileSource - I think that
libchamplain shouldn't require libmemphis to be present  in order to
compile. It's not a hard dependency - libchamplain will work even
without local rendering. I think that libmemphis should be
autodetected by the configure script (plus manually overridden by
--enable-memphis --disable-memphis) and then compiled with or without
memphis support. We could actually do the same for caches (sqlite
dependency) and network tile sources (libsoup) - these are however
much more "core" features so I don't see it that important, but as
memphis doesn't seem mature enough yet, I think that not many people
will use it they might not wish to deploy one more library with their
software.

Jiri

>
> Pierre-Luc
>