Modify

#20813 closed enhancement (fixed)

Modernize org.openstreetmap.josm.gui.layer.geoimage.ImageDisplay

Reported by: simon04 Owned by: simon04
Priority: normal Milestone: 21.05
Component: Core image mapping Version:
Keywords: heap profiling intellij yourkit Cc: Don-vip, taylor.smock, Bjoeni, GerdP, StephaneP

Description (last modified by simon04)

  1. ImageDisplay is using java.awt.MediaTracker.

According to https://stackoverflow.com/a/7369580/205629 (written in 2010):

MediaTracker was useful in 1995. Back then the primary GUI use of java was Applets, and Applets would usually load images slowly over the network.
...
These days you can typically load images synchronously, and it is best to use ImageIO.

  1. ImageDisplay creates a new thread for each image to be loaded. We can use MainApplication.worker

attachment:20813-alpha.patch​ contains 1. + 2.

  1. Omit rescaling via ImageProvider.toBufferedImage and ImageProvider.createScaledImage? When zapping through images, approx. 50% of heap allocations are due to ImageIO.read() and approx. 50% are due to ImageProvider.toBufferedImage/ImageProvider.createScaledImage.

Attachments (2)

20813-alpha.patch (8.6 KB) - added by simon04 13 months ago.
20813-beta.patch (16.9 KB) - added by simon04 13 months ago.

Download all attachments as: .zip

Change History (24)

Changed 13 months ago by simon04

Attachment: 20813-alpha.patch added

comment:1 Changed 13 months ago by simon04

Description: modified (diff)

comment:2 Changed 13 months ago by simon04

Cc: Don-vip taylor.smock Bjoeni added

comment:3 Changed 13 months ago by simon04

Description: modified (diff)

comment:4 Changed 13 months ago by taylor.smock

My major concern is that this doesn't seem to handle large image files (e.g. 56 MP) or low memory conditions gracefully.

I'd have to double check with a high-res image and a low max memory, but I suspect it will throw an OOM.

mayFitMemory(((long) width)*height*4*2)

If I didn't screw up the math:
6 MP -> 48 MB
12 MP -> 96 MB
56 MP -> 448 MB (I think I've seen a 56 MP camera on a phone somewhere).

comment:5 Changed 13 months ago by Don-vip

Milestone: 21.0421.05

Ticket retargeted after milestone closed

comment:6 in reply to:  5 ; Changed 13 months ago by simon04

Replying to taylor.smock:

My major concern is that this doesn't seem to handle large image files (e.g. 56 MP) or low memory conditions gracefully.

True. We could make use of IIOParam.setSourceSubsampling. Example code: https://stackoverflow.com/a/3296139/205629

Replying to Don-vip:

Ticket retargeted after milestone closed

Ups, thanks. I've meant to look into this in milestone:21.05.

comment:7 in reply to:  6 ; Changed 13 months ago by taylor.smock

Replying to simon04:

[...]
True. We could make use of IIOParam.setSourceSubsampling. Example code: https://stackoverflow.com/a/3296139/205629

I think this would be significantly better than our current potential OOM solution (not showing the image).

From the linked discussion:

For extra optimization, I render the buffered image in tiles also...

I'll have to look into this -- it will probably make 360 imagery a bit more performant (I've been intending to copy/move the Mapillary Image viewer code into JOSM, but priorities...)

Changed 13 months ago by simon04

Attachment: 20813-beta.patch added

comment:8 Changed 13 months ago by simon04

In 17834/josm:

see #20813 - Add ImageDisplayTest.testLoadImageRunnablePerformance

comment:9 Changed 13 months ago by simon04

Cc: GerdP StephaneP added
Keywords: heap profiling intellij yourkit added

attachment:20813-beta.patch​​ contains improvements 1. + 2. + 3. TODO: load higher resolution or correct source region when zooming in.

The patch reduces heap allocations for reading and rendering 85 images with size 6000x4000 from 2 GiB to 1 MiB.

 Method                                           Objects          Size (Allocations)
-ImageDisplayTest.testLoadImageRunnablePerformance() 2374 2_103_274_448
+ImageDisplayTest.testLoadImageRunnablePerformance()  569       983_304
Last edited 13 months ago by simon04 (previous) (diff)

comment:10 Changed 13 months ago by GerdP

Sounds great!
Sorry for my poor understanding: In what situation do I want to load images into JOSM instead of using a dedicated program like IrfanView? I've tried that once but found that most of my images "were not found to be GPS tagged". This maybe caused by a bad configuration of my rather old smartfone (Sony Xperia Z3) or quite normal. I've not invested much time in this so far.
What can I do with my images in JOSM that I can't do when I open them in IrfanView? Is it about mapping or about modifying the EXIF data in the imgage?

comment:11 in reply to:  10 Changed 13 months ago by StephaneP

Replying to GerdP:

Sounds great!
Sorry for my poor understanding: In what situation do I want to load images into JOSM instead of using a dedicated program like IrfanView?

Yes, Josm is very useful to:

  • Geolocalize images with a gnss trace and an aerial imagery
  • Edit images locations

And mappping with geolocalized images is very very useful. Here is an example:
https://pbs.twimg.com/media/EhpPyrfWsAAlMy0.jpg

comment:12 Changed 13 months ago by GerdP

My current workflow with mapping is this:
I cycle somewhere and stop from time to time to take some fotos of things that I want to map later PLUS one foto that shows my Garmin Oregon navi so that I know where I took this pictures. At home I

  • load images with IrfanView
  • load recorded gpx track into JOSM
  • look at first image and find out where I took it
  • load data around that place and use the images to improve the OSM data

If I got that right I can do this:

  • load gpx track into JOSM
  • load all images into JOSM
  • let JOSM find out where the images where taken using the timestamps
  • load data around the located places and use the images to improve the OSM data

I tried this but the results of the automatic correlation was very poor, so I gave up. Maybe I too stupid, maybe there is an error in JOSM or in my images. If this is supposed to work easily should I open a new ticket and upload my data?

comment:13 Changed 13 months ago by simon04

Please don't discuss the relevance of ImageDisplay here, but focus on its performance/improvements. Thanks!

comment:14 in reply to:  9 Changed 13 months ago by Bjoeni

Replying to GerdP:

I tried this but the results of the automatic correlation was very poor, so I gave up. Maybe I too stupid, maybe there is an error in JOSM or in my images. If this is supposed to work easily should I open a new ticket and upload my data?

Actually the autoguess function never worked for me either. That only works when the first picture is taken a the same time the track recording starts, so I guess it's only useful for very few cases. Usually you just have to set timezone and offset manually. Maybe the "Auto-Guess" function should be renamed or behave differently, it confused me as well. New ticket I guess?


Replying to simon04:

attachment:20813-beta.patch​​ contains improvements 1. + 2. + 3. TODO: load higher resolution or correct source region when zooming in.

The patch reduces heap allocations for reading and rendering 85 images with size 6000x4000 from 2 GiB to 1 MiB.

 Method                                           Objects          Size (Allocations)
-ImageDisplayTest.testLoadImageRunnablePerformance() 2374 2_103_274_448
+ImageDisplayTest.testLoadImageRunnablePerformance()  569       983_304

Looks good! I agree that it's long overdue. I thought about doing it in #20341, but didn't have the time.

Some remarks about the getSubsampling() method:

  • Sometimes the subsampling is a bit too agressive. It looks much better when using Math.floor() instead of Math.round(), since only pixels that are definitely not going to be shown should be discarded.
  • I think the method doesn't work the way you intended it to: When both dimensions of the source image d1 are larger than those of the viewer d2 (which is the case most of the time), the subsampling is always based on the width, even though the height might be the limiting factor.

I think it should look something like this:

        private int getSubsampling(Dimension d1, Dimension d2) {
            if (d1.getWidth() > d2.getWidth() || d1.getHeight() > d2.getHeight()) {
                return (int) Math.floor(Math.max(
                        d1.getWidth() / d2.getWidth(),
                        d1.getHeight() / d2.getHeight()));
            }
            
            return 1;
        }
Last edited 13 months ago by Bjoeni (previous) (diff)

comment:15 Changed 13 months ago by simon04

In 17856/josm:

see #20813 - Add ThumbsLoaderTest.testPerformance

comment:16 in reply to:  7 Changed 13 months ago by ar2988-os@…

Replying to taylor.smock:

I'll have to look into this -- it will probably make 360 imagery a bit more performant (I've been intending to copy/move the Mapillary Image viewer code into JOSM, but priorities...)

(OT) +1 for making the image viewer handle 360° panoramas. As a workaround, I coded a patch #13815 to open an image in an external viewer, but the code was not sufficiently polished to be incorporated into JOSM. I am still using the patch, updated for changes in JOSM.

comment:17 Changed 13 months ago by simon04

In 17870/josm:

see #20813 - ThumbsLoaderTest: add assertions

comment:18 Changed 13 months ago by simon04

In 17871/josm:

see #20813 - Modernize ImageDisplay using ImageIO and subsampling

comment:19 Changed 13 months ago by simon04

In 17872/josm:

see #20813 - Apply EXIF rotation in ImageEntry.load

comment:20 Changed 13 months ago by simon04

In 17873/josm:

see #20813 - Modernize ThumbsLoader using ImageIO and subsampling

comment:21 in reply to:  9 Changed 13 months ago by simon04

Replying to simon04:

attachment:20813-beta.patch​​ contains improvements 1. + 2. + 3. TODO: load higher resolution or correct source region when zooming in.

The patch reduces heap allocations for reading and rendering 85 images with size 6000x4000 from 2 GiB to 1 MiB.

I haven't implemented this aggressive subsampling to keep the high resolution for zooming in. If anybody is interested to implement reloading in a higher resolution only when necessary, please go ahead. I will not be working on that in the forseable future.

The maximum width/height (given in pixels) to load can be changed using the advanced preference geoimage.maximum-width (defaulting to 6000px).

comment:22 Changed 12 months ago by simon04

Resolution: fixed
Status: assignedclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain simon04.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.