search

bailuk / AAT

Another Activity Tracker for Android
https://bailu.ch/aat
GNU General Public License v3.0
150 stars 41 forks source link

Fix issues with previews in track list #136

Closed rparkins999 closed 1 year ago

rparkins999 commented 1 year ago

This patch fixes some problems with the previews in the track list, where a preview could fail to be created at all, or show only the track with no map behind it.

This is a fairly big change as there were two areas with bugs:-

I don't know what CacheOnlySource is supposed to do, but what it did do was to prevent bitmaps from being being loaded into a MapsForgePreview if they were not already in memory. This can happen if the track was recorded without displaying a map: I prefer to run AAT with my phone in my pocket and the screen off. I have replaced occurrences of CacheOnlySource in MapsForgePreview with the content. As far as I can see, this doesn't affect anything else.

DirectorySynchronizer wasn't thread-safe. Starting it more than once could cause the database to be updated showing a preview to exist without actually creating it. This happens on the first request for a track list: inside AbsGpxListActivity.onResumeWithService() iteratorSimple calls rescan() when the DirectorySynchronizer is created and it is called again later on, presumably because it will not have been called if the DirectorySynchronizer already existed. Also the test for whether all tiles had their bitmaps loaded was incorrect, so that a preview could be created with some or all of its map data missing.

There seem to be some issues with spacing in the layout of the track list. I have not (yet) attempted to correct those.

bailuk commented 1 year ago

This merge request introduces a lot of issues without fixing any real ones.

Directory synchronizer is very old an tested code. Its far from perfect but it works and is fast. If there is a real issue with it it must be addressed precisely. Refactoring the hole code can be done, but it must be really a visible improvement over the current one. And of course it must be tested thoroughly.

rparkins999 commented 1 year ago

The issues fixed are certainly real. I was consistently seeing previews with no map behind the track or no map and no track, both with the emulator and on my phone. The changes fix these problems. I've tested with both the emulator and my phone, each with both downloaded OpenAndroMaps and loaded-on-demand MAPNIK..

The no map and no track was caused by multiple calls to rescan() when the DirectoryServer has to be created. The first call updates the database to show that the preview has been created and then that instance of DirectorySynchronizer gets stopped before its background process has had time to run to completion. The next instance sees that the database says that the preview has been created and doesn't try to create it, so it never gets created. Arguably you're doing things in the wrong order here: you shouldn't update the database to show that the preview has been created until after it has actually been created. I didn't change that as it would have required an even bigger redesign of DirectorySynchronizer.

My changes force a DirectorySynchronizer once started to be allowed to run to completion before another rescan() creates a new one, which avoids this problem.

The track without a map behind it was caused by the DirectorySynchronizer not waiting for the map to be rendered into memory (which can be done by a background process) before attempting to write the preview file. There was a test, but it was incorrect. If the logic that writes the preview file sees a null bitmap, it writes a blank map behind the track. Once the preview file has been created, you don't write it again, so the blank map stays there forever.

My changes force a DirectorySynchronizer to wait until all bitmaps required for the preview have been rendered into memory.

I think that there was a third place where DirectorySynchronizer wasn't waiting for a background action to be completed. I didn't see a visible problem caused by that, but it was wrong so I fixed it.

You may have spotted that I changed the message broadcast by the background process which reads the gpx file. This was a maintainability issue. The message originally used was also used elsewhere, so another thread could broadcast that message and move the DirectorySynchronizer on to the next state before the background process has completed. Even if that can't happen at present, a future change in an entirely unrelated part of the code could cause it to happen and break DirectorySynchronizer. Since this message is an essentially private communication to the DirectorySynchronizer from its background process, it should not be used anywhere else.

I don't know why you haven't seen these problems, but they are of course thread synchronisation errors, so It may depend on the order in which threads get run. This can differ with different versions of Android and with different devices. Both the emulator and my phone (Samsung Galaxy S21 Ultra 5G) are running Android 12. If you haven't tested your version with Android 12 yet, I would suggest that you try it.

I could have split the changes into separate pull requests, but I had to make all of them to make it work, and they all make changes to DirectorySynchronizer to fix thread synchronization problems, so I issued a pull request with all of my changes.

bailuk commented 1 year ago

Preview images are either generated or not. Black tiles do not result from "not generated" preview images but from missing tiles. Tiles are missing because they are not downloaded. The Preview generator is designed with intent to not download tiles.

Now preview images without a track that would be a bug. But that must be further tracked down. How often does this happen? Was a preview image generated at all? Is it reproducible. What happens if you select "reload preview" from context menu?

I think that there was a third place where DirectorySynchronizer wasn't waiting for a background action to be completed. I didn't see a visible problem caused by that, but it was wrong so I fixed it.

AppBroadcaster.FILE_CHANGED_INCACHE is a general broadcasting message. The pattern here is that the receiver must check if he is in charge for this file. This happens in the ping() method of a state. Less broadcast message types makes the code more maintable IMHO. It's a bit like with globals.

rparkins999 commented 1 year ago

Now preview images without a track that would be a bug. But that must be further tracked down. How often does this happen? Was a preview image generated at all? Is it reproducible.

It is certainly reproducible, but I will have to reinstall your release build into my phone and go for a long walk in order to verify the exact circumstances required to reproduce it. I'm quite busy with other things at the moment.

I think that it happens when you install AAT into a phone which didn't have it, start it up for the first time (so no database, no files, and no cached map tiles), go for a long walk while tracking without displaying any maps (so no tiles get loaded), stop tracking, and ask for a track list. The first call to rescan() creates a new DirectorySynchronizer which runs a BackgroundTask: this updates the database to show that there is a preview file and then sends a FILE_CHANGED_INCACHE message to its DirectorySynchronizer. However by the time the original DirectorySynchronizer gets to run, rescan() has been called again, and the original DirectorySynchronizer has been told to terminate and doesn't actually write a preview file. The second call to rescan() creates a new DirectorySynchronizer which sees from the database that the preview file already exists and doesn't attempt to write one, so the track list entry appears with no preview. If I recall correctly it displays three bars instead of a preview.

"Reload preview" doesn't help. It doesn't try to create a new preview as you seem to think, but just tries (and fails) to load the preview that it thinks already exists.

I think that I was also able to reproduce this problem with the emulator by uninstalling AAT if it was previously installed, installing it, playing a stored gpx file into the emulator while tracking (again without displaying at any maps), stopping tracking, and asking for a track list.

Since the problem is caused by a synchronisation failure between threads, reproducibility may depend on how many processor cores your device has, how fast they are, which version of Android you are running, and whether you are running a release build or a debug build (which potentially affects timing).

rparkins999 commented 1 year ago

The pattern here is that the receiver must check if he is in charge for this file. This happens in the ping() method of a state.

Well, it doesn't quite do that. The background tasks sets backgroundTask to null before broadcasting its message, and ping() checks backgroundTask and does nothing if it isn't null. I agree that has the same effect.

rparkins999 commented 1 year ago

The Preview generator is designed with intent to not download tiles

The problem with this is that once you have generated a preview image with missing tiles, you are stuck with it. There is no way to regenerate it. "Reload preview" doesn't help as I said in my previous comment. You can of course delete the entry, but I think that throws away the gpx file as well.

If you are concerned about waiting to load tiles before displaying the track list, there is a standard Android model for dealing with this. You display the track list without waiting for previews to be created, and fire off a series of background processes to load and render any needed tiles and update the display. Of course you need to keep track of which tiles have been loaded so that if later on you ask for the track list again you will try again to load any missing tiles. This covers the situation where the on-line map was temporarily inaccessible. The Gallery app on most Android phones works by firing off processes like this.

The difficulty here is that you need to decide when to write the preview file. Waiting until all tiles have been loaded and rendered doesn't work because some tiles may not exist in the map data or an on-line map may currently not be accessible at all. Rewriting the preview file whenever you update the displayed image does work, but is a lot of wasted effort. However it does mean that you can tell which tiles still need to be rendered of you put a tile list into the preview file. Perhaps a compromise would be to throttle the frequency at which the preview file gets written so that if several tiles arrive in quick succession you only rewrite the file once.

My solution of waiting until all tiles are rendered works for me because I use off-line mapping and I don't walk in the middle of the ocean or in a desert where there is no data in my offline map. My machine is fast so the wait while the tiles are rendered doesn't bother me much.

I concede that it doesn't work correctly if some tiles are unavailable either because the source map doesn't have data there or because on-line mapping is currently inaccessible. In that case it will wait indefinitely and the track list won't get displayed at all. However I still think that the existing code needs improving so that you don't get stuck with a preview without any mapping when the required tiles weren't in the cache when you asked for a track list.

bailuk commented 1 year ago

Sorry for the long delay. Unfortunately I'm not always able to spend as much time on my GitHub projects as I would like to.

Just some thoughts about Improving Tile Preview:

The problem with this is that once you have generated a preview image with missing tiles, you are stuck with it. There is no way to regenerate it. "Reload preview" doesn't help as I said in my previous comment. You can of course delete the entry, but I think that throws away the gpx file as well.

"Reload preview" Removes the preview image, loads the track and regenerates with the available (cached) tiles. This has nothing to do with the record in the summary database.

If you are concerned about waiting to load tiles before displaying the track list, there is a standard Android model for dealing with this.

My suggestion (in the sense that I could accept it) would be to implement a new preview generator with the following requirements:

rparkins999 commented 1 year ago

Well, my use case is different. I don't show maps while tracking, and I normally don't look at the map of the current location either, so there are never any cached tiles, even though I use off-line mapping. Because of the issue with the Directory synchronizer getting restarted before it has had time to do anything apart from updating the database to show that the preview image has been generated, the preview image almost never does get generated, and no tiles ever get cached. It does read the gpx file, but that gets read into a buffer which is private to the particular instance of the Directory synchronizer, so if that instance gets stopped and thrown away before it updates the screen, you get no preview image at all. If the instance gets stopped a bit later, you get a preview image with a track but no map behind it.

I think the issue with the Directory synchronizer getting restarted too soon is a real bug in the existing Directory synchronizer and needs to be fixed. Making an alternative Directory synchronizer won't fix it. I may be able to extract a fix from my code without the other changes, but I too am busy with other things at the moment.

On Friday, 21 October 2022 at 06:59:57 BST, bailuk ***@***.***> wrote:

Sorry for the long delay. Unfortunately I'm not always able to spend as much time on my GitHub projects as I would like to.

Just some thoughts about Improving Tile Preview:

The problem with this is that once you have generated a preview image with missing tiles, you are stuck with it. There is no way to regenerate it. "Reload preview" doesn't help as I said in my previous comment. You can of course delete the entry, but I think that throws away the gpx file as well.

"Reload preview" Removes the preview image, loads the track and regenerates with the available (cached) tiles. This has nothing to do with the record in the summary database.

If you are concerned about waiting to load tiles before displaying the track list, there is a standard Android model for dealing with this.

My suggestion (in the sense that I could accept it) would be to implement a new preview generator with the following requirements:

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>