Download Manager #40

ChocolateFrogsNuts · 2018-07-09T01:31:33Z

Some bug fixes and a Download Manager to allow parallel background downloading of OSM tiles.
Really speeds up map display on high-latency connections (eg satellite internet)

Some minor bugfixes to prevent potential buffer overruns. Use TransformImageColorspace on non-RGB OSM Tiles.

Changes for MagickLibVersion > 0x669 GetImagePixels() -> GetAuthenticPixels() GetIndexes() -> GetAuthenticIndexQueue() SetImage() -> SetImageBackgroundColor(), SetImageOpacity() DescribeImage() -> IdentifyImage() Also newer versions of IM use double rather than int for certain colour values. This required replacing <<8 with *256 in a number of places. Also a cast to (int) in a few places.

The DownLoad Manager (DLM) will transfer up to 8 tiles at a time. It can also be used as a generic file downloader. OSM tiles are now downloaded to the cache in the background with up to 8 tiles being transferred at a time. The most recently requested tiles are downloaded first so that the area in view is updated in a timely manner. We then go back to catch up on any remaining tiles. As tiles become available, the map is updated on screen. Tiles are cached for up to 7 days. If an older tile is in the cache, a download is queued, and the old tile is used until the updated version is available.

tvrusso · 2018-07-09T02:04:19Z

This looks like an interesting and exciting PR, and I will have to test it out a bit. You might also advertise it on the Xastir mailing list and let some other folks test it out while it's pending. I may not be able to get a look at it for a few weeks, and Curt is pretty busy, too.

It appears you may also have partially addressed issue report #23. Have you tested this set of code changes against GraphicsMagick, in which GetIndexes is also deprecated?

ChocolateFrogsNuts · 2018-07-10T08:07:23Z

I don't use GraphicsMagick, so it will need testing against that, but I only made the minimal changes to get a compile. The changes may well work with both.
Not much traffic on the mailing list recently but I'll announce it on there shortly anyway so we get some testers on board.

we7u · 2018-07-10T17:58:28Z

Merged into my own local git repo, on SUSE Leap 42.3 with all updates and GraphicsMagick installed:

map_OSM.o: In functiondraw_OSM_image':
/home/archer/xastir/src/map_OSM.c:726: undefined reference to GetAuthenticPixels' /home/archer/xastir/src/map_OSM.c:736: undefined reference to GetAuthenticIndexQueue'
map_OSM.o: In function draw_image': /home/archer/xastir/src/map_OSM.c:525: undefined reference to GetAuthenticPixels'
/home/archer/xastir/src/map_OSM.c:535: undefined reference to GetAuthenticIndexQueue' map_OSM.o: In function draw_OSM_tiles':
/home/archer/xastir/src/map_OSM.c:1212: undefined reference to SetImageBackgroundColor' /home/archer/xastir/src/map_OSM.c:1242: undefined reference to ClearMagickException'
/home/archer/xastir/src/map_OSM.c:1279: undefined reference to IdentifyImage' collect2: error: ld returned 1 exit status

tvrusso · 2018-07-10T18:27:37Z

It appears that GraphicsMagick uses "MagickLibVersion" exactly as named in ImageMagick, but uses a very different numbering scheme, so a simple comparison against a value from ImageMagick is no good. The code in question is testing against the value 0x669 but the recent GraphicsMagick I have has the value 0x211800.

There will have to be a test against HAVE_GRAPHICS_MAGICK in there, too.

We have tons of old ifdefs in there from ancient versions of Magick (for which there's a separate issue #25 calling for their removal). They are probably all broken as well, but don't matter because nobody has used those ancient versions for years.

This also means that the fixes here do not in fact address #23 either, since GraphicsMagick has a different function that is intended as the replacement for GetIndexes. When I opened #23, I checked and it seemed that ImageMagick had the same one as well, but a different, ImageMagick-specific one is being used here.

we7u · 2018-07-10T19:26:06Z

FYI: We have historically supported both GM and IM. GM has been more stable, originally a fork of IM. Some people only have IM available, some GM, so supporting both has worked well for us minus the struggles IM sometimes has. I run only GM, unless on a system that doesn't have it in its package manager.

we7u

Needs fixes to handle the GraphicsMagick library as well. Some of our users use GraphicsMagick, some use ImageMagick. Sometimes a bug in one forces the use of the other on any given Linux distribution.

we7u · 2018-07-10T21:07:59Z

A few more things (all minor stuff):

Changes to interface.c have nothing to do with the main reason for the PR. Would be better to do a separate PR for it so it's not hidden in the big patch.
Changing from "<< 8" to "* 256"" is slower. I suggest keeping the "<< 8" code in the map*.c files.
The headers on the two new files appear to have been copied from an existing file. They need Copyright dates updated at least. Should be only "Copyright (C) 2018 The Xastir Group" since they are new files. "dlm.h" says: "contributed by Jerry Dunmire, KA6HLD." which needs to be corrected as well. "dlm.c" has an $Id: message which is wrong, plus also has "This file derived from" message at the bottom of the header. Correct? Should be deleted?

I'm looking forward to trying this out when the GM stuff gets fixed!

Modify the changes for newer ImageMagick versions to accommodate GraphicsMagick.

ChocolateFrogsNuts · 2018-07-14T18:42:40Z

Hi all, Found the messages disappearing into my spam folder... and fixed that :) *interface.c:* Yes that should probably have been a separate PR - I'm relatively new to git so I'll see if I can manage to change that. *<<8 vs *256:* For int types the compiler will (at least should) optimize *256 into <<8 making it no slower. The reason for the change is that the type of the variable changes to double, so <<8 is no longer possible. I may have to have another go at 3 printf calls that I changed (%i -> %1.2f). It's probably easiest to change them back to %i and cast the args to match. *Copyright:* Yes, they were copied. Suggestions adopted. *IM vs GM* I'm no expert on IM or GM but I found deprecation info for IM 6.7 that supports the function substitutions I made (compiling with 6.9 here). http://patrickdieudonne.com/ImageMagick-6.7.0-2/www/api/deprecate.html GM has only deprecated GetPixels() and GetIndexes() from version 1.3.28 (Jan 2018), so that's not urgent. I didn't find any info on what GM is replacing it with, but IM7 replaces it yet again :( For now I have modified the #if directives to only apply the changes where GraphicsMagick is not in use. That should get it to compile, and once we have a plan for how to handle this I'll hunt down the remaining GetIndexes() calls that haven't been replaced yet. Mike Curt Mills, WE7U wrote on 11/07/18 05:08:

…

A few more things (all minor stuff): * Changes to interface.c have nothing to do with the main reason for the PR. Would be better to do a separate PR for it so it's not hidden in the big patch. * Changing from "<< 8" to "* 256"" is slower. I suggest keeping the "<< 8" code in the map*.c files. * The headers on the two new files appear to have been copied from an existing file. They need Copyright dates updated at least. Should be only "Copyright (C) 2018 The Xastir Group" since they are new files. "dlm.h" says: "contributed by Jerry Dunmire, KA6HLD." which needs to be corrected as well. "dlm.c" has an $Id: message which is wrong, plus also has "This file derived from" message at the bottom of the header. Correct? Should be deleted? I'm looking forward to trying this out when the GM stuff gets fixed! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#40 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AaQZuqr5l-9jJplRf2XuQAifbIPa73Mwks5uFRexgaJpZM4VG9hD>.

Created a new branch bugfix1 so this can be pulled separately.

This call is sort of redundant as the next call is to GetExceptionInfo which clears it anyway. GraphicsMagick doesn't have it, but ImageMagick does, although it requires newer versions.

Now compatible with GraphicsMagick 1.3.30 and ImageMagick 6.9.10. No more IM/GM related deprecation warnings (with the usual #if ugliness) Fixed several mis-spellings of HAVE_GRAPHICSMAGICK (an extra underscore) that I had introduced.

Change the MagickLibVersion checks.

These had been updated for newer ImageMagick, but produce warnings on GraphicsMagick. Fixed by casting the args to int instead of updating the format string to %1.2f.

ChocolateFrogsNuts · 2018-07-20T00:28:59Z

Ok, I think I've taken care of everything requested, and I now have it compiling on both ImageMagick 6.9.10 and GraphicsMagick 1.3.30 without any related deprecation warnings. I left the *256 in because it's backward compatible, the compiler will optimise it to <<8 for older IM/GM and IM7 will require the change (6.9.10 already does and it's in the documentation for porting to 7). Give it a whirl Mike Curt Mills, WE7U wrote on 11/07/18 05:08:

…

A few more things (all minor stuff): * Changes to interface.c have nothing to do with the main reason for the PR. Would be better to do a separate PR for it so it's not hidden in the big patch. * Changing from "<< 8" to "* 256"" is slower. I suggest keeping the "<< 8" code in the map*.c files. * The headers on the two new files appear to have been copied from an existing file. They need Copyright dates updated at least. Should be only "Copyright (C) 2018 The Xastir Group" since they are new files. "dlm.h" says: "contributed by Jerry Dunmire, KA6HLD." which needs to be corrected as well. "dlm.c" has an $Id: message which is wrong, plus also has "This file derived from" message at the bottom of the header. Correct? Should be deleted? I'm looking forward to trying this out when the GM stuff gets fixed! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#40 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AaQZuqr5l-9jJplRf2XuQAifbIPa73Mwks5uFRexgaJpZM4VG9hD>.

tvrusso · 2019-03-16T17:45:43Z

It would probably be good to get this resolved soon. Curt: have your requested changes been addressed to your satisfaction? It's been over a year since we released Xastir 2.1.0, and maybe we should start thinking about releasing 2.1.2. We should not do so with all of those ImageMagick and GraphicsMagick deprecated function calls, lest we find Xastir unbuildable on some distro in short order.

we7u · 2019-04-04T17:02:20Z

Letting you know I'm looking at this again and have part of it checked. I'm really liking the code so far. I hope to have the check completed by this weekend.

tvrusso · 2019-04-04T17:13:50Z

For what it's worth, while I'm not reviewing this code, I have just downloaded the branch and am at least compile and run testing it on my system. The graphicsmagick deprecation warnings are gone, and the code is running. I am delighted with the new speed of the updates as I pan the map with an online map source, which used to be quite slow.

we7u · 2019-04-04T17:18:30Z

Yep! You should probably read the comments at the top of dlm.c to see how he's doing it. Pretty impressive so far.

godfreja · 2019-04-04T17:57:29Z

src/dlm.c

+pthread_t DLM_queue_thread;
+
+volatile int DLM_queue_progress_flag=0;
+volatile int DLM_queue_state=DLM_Q_STOP;


Should DLM_queue_state be protected by a mutex to ensure correct memory visibility? I don't believe volatile is enough.

Are you ok if I merge this while we wait on the answer? I'm good with the rest of the code and think this would be a step forward. If the variable needs protection one of us can add that.

I am OK with that, although it might be good to hold on a new release until we figure it out.

I'm confused as to how DLM_queue_state should be working. I could see it protected by a mutex and being an INT (for perhaps a count of download threads running), but I don't see how having multiple threads able to write specific values into it will work, mutex or not. It's a single variable but multiple threads can write STOP/RUN/QUIT/IDLE/STARTING values into it at any time. Perhaps make it a count variable with defined ALL-STOP value (-1) or something? It's also easily possible that I misunderstand what its purpose is.

Just to clarify what my concern is: I've been bitten in the past where only one thread is modifying a variable and the reading thread does not see the update. The variable was marked volatile. A mutex is needed to ensure changes are visible to other threads. Having the mutex inserts a memory barrier.

See https://en.wikipedia.org/wiki/Memory_barrier#Example for a bit of an explanation.

I can add in the mutexes if desired.

Feel free. Usual fork / modify(on a branch) / pull-request. I'd have to look deeper at the code to decide for sure that it's needed, but if one thread is writing and at least one other is reading - we need it. Thanks!

ChocolateFrogsNuts · 2019-04-05T13:14:51Z

One thread manages all the downloads, no matter how many downloads are in parallel and that thread is a state machine which operates according to DLM_queue_state. The only time DLM_queue_state is changed outside of that thread is by DLM_queue_start_if_needed(), and then ONLY when the downloader thread is not running, thus no lock is required. There may be some confusion coming from my re-use of DLM_Q_* for the state of individual tiles as well as the state of the entire queue, but that is fairly easy to spot as it always involves tile->state rather than DLM_queue_state. Good to see it getting some attention again :) Mike VK6MN Curt Mills, WE7U wrote on 05/04/19 05:36:

…

***@***.**** commented on this pull request. ------------------------------------------------------------------------ In src/dlm.c <#40 (comment)>: > + char *curlErrBuf; + CURL *curlSession; +#endif +#endif +}; + + +/* DLM_queue_lock is held whenever reading or changing DLM_queue_entry or + * the linkages of the items in the queue. Each item also has a lock + * for changing data in that item. + */ +xastir_mutex DLM_queue_lock = { .threadID=0, .lock=PTHREAD_MUTEX_INITIALIZER }; +pthread_t DLM_queue_thread; + +volatile int DLM_queue_progress_flag=0; +volatile int DLM_queue_state=DLM_Q_STOP; I'm confused as to how DLM_queue_state should be working. I could see it protected by a mutex and being an INT (for perhaps a count of download threads running), but I don't see how having multiple threads able to write specific values into it will work, mutex or not. It's a single variable but multiple threads can write STOP/RUN/QUIT/IDLE/STARTING values into it at any time. Perhaps make it a count variable with defined ALL-STOP value (-1) or something? It's also easily possible that I misunderstand what its purpose is. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#40 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AaQZugT2VTP5L1xUoFY1sAM3vGXMyBPtks5vdnBggaJpZM4VG9hD>.

we7u · 2019-04-05T14:24:26Z

Excellent! That means we shouldn't have to do anything to the code. It's been merged with master. Thanks for your contribution!

In commits b071954 and 7815dd9 (and possibly others associated with PR Xastir#40), the GetIndexes function (present in both ImageMagick and GraphicsMagick, and deprecated in both) was coded around so that if we've got newer versions of the library, we use the new API, and if we have an older version, we use the older API. But Xastir#40 only fixed these deprecated uses in a few files, and mised the one usage in map_geo.c This commit copies over the fix verbatim from map_OSM.c, and shuts up the deprecation warning. Fixes Xastir#81.

ChocolateFrogsNuts added 3 commits July 2, 2018 11:58

Bugfixes and handle non-RGB OSM tiles

67b084d

Some minor bugfixes to prevent potential buffer overruns. Use TransformImageColorspace on non-RGB OSM Tiles.

tvrusso requested a review from we7u July 9, 2018 02:04

we7u requested changes Jul 10, 2018

View reviewed changes

ChocolateFrogsNuts added 2 commits July 14, 2018 23:15

Update copyright info in dlm.c, dlm.h

e388d93

GraphicsMagick vs ImageMagick

e9535ee

Modify the changes for newer ImageMagick versions to accommodate GraphicsMagick.

ChocolateFrogsNuts added 5 commits July 15, 2018 19:51

Roll back the bugfix part of commit 0cea7102

6e40b31

Created a new branch bugfix1 so this can be pulled separately.

ClearMagickException not available in GraphicsMagick

1832107

This call is sort of redundant as the next call is to GetExceptionInfo which clears it anyway. GraphicsMagick doesn't have it, but ImageMagick does, although it requires newer versions.

GetIndexes is deprecated from GraphicsMagick 1.3.28

b071954

Change the MagickLibVersion checks.

Fix two debugging printf formats

986038a

These had been updated for newer ImageMagick, but produce warnings on GraphicsMagick. Fixed by casting the args to int instead of updating the format string to %1.2f.

tvrusso requested a review from we7u March 16, 2019 17:45

godfreja reviewed Apr 4, 2019

View reviewed changes

we7u merged commit 0cb9a07 into Xastir:master Apr 4, 2019

tvrusso mentioned this pull request Apr 5, 2019

All map processing in Xastir that uses graphicsmagick uses deprecated "GetIndexes" function. #23

Closed

This was referenced Apr 21, 2019

Xastir crashes on map manipulation, leaves stray processes #66

Closed

Warning fixes 2 #70

Merged

map_geo.c still uses one deprecated GraphicsMagick call #81

Closed

tvrusso mentioned this pull request Apr 23, 2019

Fix deprecated GetIndexes usage #82

Merged

tvrusso added this to Done in Release 2.1.2 May 1, 2019

tvrusso mentioned this pull request Jul 17, 2019

Remove ImageMagick version 6 deprecation warnings #151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download Manager #40

Download Manager #40

ChocolateFrogsNuts commented Jul 9, 2018

tvrusso commented Jul 9, 2018

ChocolateFrogsNuts commented Jul 10, 2018

we7u commented Jul 10, 2018 •

edited

tvrusso commented Jul 10, 2018

we7u commented Jul 10, 2018 •

edited

we7u left a comment

we7u commented Jul 10, 2018

ChocolateFrogsNuts commented Jul 14, 2018 via email

ChocolateFrogsNuts commented Jul 20, 2018 via email •

edited

tvrusso commented Mar 16, 2019 •

edited

we7u commented Apr 4, 2019

tvrusso commented Apr 4, 2019

we7u commented Apr 4, 2019

godfreja Apr 4, 2019

we7u Apr 4, 2019

godfreja Apr 4, 2019

we7u Apr 4, 2019

godfreja Apr 5, 2019

godfreja Apr 5, 2019

we7u Apr 5, 2019

ChocolateFrogsNuts commented Apr 5, 2019 via email

we7u commented Apr 5, 2019

Download Manager #40

Download Manager #40

Conversation

ChocolateFrogsNuts commented Jul 9, 2018

tvrusso commented Jul 9, 2018

ChocolateFrogsNuts commented Jul 10, 2018

we7u commented Jul 10, 2018 • edited

tvrusso commented Jul 10, 2018

we7u commented Jul 10, 2018 • edited

we7u left a comment

Choose a reason for hiding this comment

we7u commented Jul 10, 2018

ChocolateFrogsNuts commented Jul 14, 2018 via email

ChocolateFrogsNuts commented Jul 20, 2018 via email • edited

tvrusso commented Mar 16, 2019 • edited

we7u commented Apr 4, 2019

tvrusso commented Apr 4, 2019

we7u commented Apr 4, 2019

godfreja Apr 4, 2019

Choose a reason for hiding this comment

we7u Apr 4, 2019

Choose a reason for hiding this comment

godfreja Apr 4, 2019

Choose a reason for hiding this comment

we7u Apr 4, 2019

Choose a reason for hiding this comment

godfreja Apr 5, 2019

Choose a reason for hiding this comment

godfreja Apr 5, 2019

Choose a reason for hiding this comment

we7u Apr 5, 2019

Choose a reason for hiding this comment

ChocolateFrogsNuts commented Apr 5, 2019 via email

we7u commented Apr 5, 2019

we7u commented Jul 10, 2018 •

edited

we7u commented Jul 10, 2018 •

edited

ChocolateFrogsNuts commented Jul 20, 2018 via email •

edited

tvrusso commented Mar 16, 2019 •

edited