Modify

Opened 6 years ago

Last modified 6 weeks ago

#16472 assigned enhancement

Add support to 360 / spherical image

Reported by: StephaneP Owned by: Don-vip
Priority: major Milestone:
Component: Core image mapping Version:
Keywords: 360, panorama, spherical sotmfr2019 Cc: taylor.smock

Description (last modified by Klumbumbus)

Attachments (13)

spherical image inside josm.JPG (96.8 KB ) - added by StephaneP 6 years ago.
16472.patch (116.4 KB ) - added by taylor.smock 3 years ago.
Initial, mostly working, patch
16472.2.patch (11.2 KB ) - added by taylor.smock 3 years ago.
WIP: DO NOT APPLY -- this code applies to attachment:16472.patch, and is an attempt to figure out where azimuth/polar angles are being flipped
16472.3.patch (122.0 KB ) - added by taylor.smock 3 years ago.
16472.4.patch (123.8 KB ) - added by taylor.smock 3 years ago.
Working 360 viewer, minimal tests, many tests disabled due to failures, but is usable and can be merged
16472.5.patch (28.0 KB ) - added by taylor.smock 3 years ago.
Test work for attachment:16472.4.patch . THIS BREAKS IMAGE PANNING! DO NOT APPLY!
16472.6.patch (123.9 KB ) - added by taylor.smock 3 years ago.
Update attachment:16472.4.patch
16472.fix_shortcuts.patch (792 bytes ) - added by taylor.smock 3 years ago.
Fix shortcuts (add a listener for enable state change on button creation)
pano_Nanterre_universités_V6MPack_with_distortion.jpg (17.0 MB ) - added by StephaneP 3 years ago.
picture not detected as a 360 image
16472.read_image_type_prior_to_short_circuit.patch (2.8 KB ) - added by taylor.smock 3 years ago.
Move xmp and iptc parsing to before gps short circuit
16472.coverity_fixes.patch (1.5 KB ) - added by taylor.smock 3 years ago.
Fix coverity FB.FE_FLOATING_POINT_EQUALITY and FB.UG_SYNC_SET_UNSYNC_GET
16472.AIOOBE.patch (993 bytes ) - added by taylor.smock 3 years ago.
16472.load_all.patch (4.0 KB ) - added by taylor.smock 3 years ago.
DO NOT APPLY -- load the entire image into memory, with attendent calculations (this is very expensive). There is also a UI bug with this patch.

Change History (91)

by StephaneP, 6 years ago

comment:1 by Klumbumbus, 6 years ago

Component: CoreCore image mapping

comment:2 by Klumbumbus, 6 years ago

Type: defectenhancement

comment:3 by Don-vip, 6 years ago

Description: modified (diff)

comment:4 by Don-vip, 6 years ago

Priority: normalmajor

comment:6 by floscher, 6 years ago

The Mapillary plugin now displays 360° images: https://github.com/JOSM/Mapillary/pull/79

comment:7 by Klumbumbus, 6 years ago

Description: modified (diff)

comment:8 by Klumbumbus, 6 years ago

Description: modified (diff)

comment:9 by Don-vip, 5 years ago

Keywords: sotmfr2019 added

comment:10 by StephaneP, 5 years ago

New ressources: pv - a free open source multi platform image and panorama viewer (C++)
https://bitbucket.org/kfj/pv/src/master/

comment:11 by jBeata, 4 years ago

Any update regarding this feature? It would help to have a common library or functionality in JOSM that we could use for visualizing 360 images.

comment:12 by mdk, 4 years ago

There are at least two plugins dealing with 360° images: Mapillary and Streetside, Do we really need an other implementation? Wouldn't it make more sense to add missing functionality in one of them?

Last edited 3 years ago by Don-vip (previous) (diff)

comment:13 by jBeata, 4 years ago

I was thinking more about, having only 1 common implementation in JOSM core and everyone else just using it. Instead of adding the same functionality to a third plugin.

comment:14 by Don-vip, 4 years ago

It depends on the technical dependencies (libraries). If based on JavaFX, it can't be yet included in JOSM core and must be included in a plugin (could be the JavaFX plugin). Then all plugins can depend on it. Otherwise, If Swing-based, it's ok for JOSM core.

Last edited 3 years ago by Don-vip (previous) (diff)

comment:15 by Don-vip, 3 years ago

Owner: changed from team to Don-vip
Status: newassigned

comment:16 by Don-vip, 3 years ago

See #17858 for JavaFX progress. I can finally start to look at this.

comment:17 by Don-vip, 3 years ago

Cc: taylor.smock added

@taylor I've never looked at 360 before. Can you please confirm that:

  • Mapillary 360 viewer code is in org.openstreetmap.josm.plugins.mapillary.gui.panorama.*
  • StreetSide 360 viewer code is in org.openstreetmap.josm.plugins.streetside.gui.imageinfo.ThreeSixtyDegreeViewerPanel
  • they have nothing in common
  • Mapillary code is more advanced / robust

in reply to:  17 comment:18 by taylor.smock, 3 years ago

Replying to Don-vip:

@taylor I've never looked at 360 before. Can you please confirm that:

  • Mapillary 360 viewer code is in org.openstreetmap.josm.plugins.mapillary.gui.panorama.*

Partially. Some of it is also in the image viewer dialog. (org.openstreetmap.josm.plugins.mapillary.gui.MapillaryImageDisplay -- see paintPano and paintNonPano).

  • StreetSide 360 viewer code is in org.openstreetmap.josm.plugins.streetside.gui.imageinfo.ThreeSixtyDegreeViewerPanel

That is what it looks like -- I was just looking at it today, since I was intending to try to move the code for one of them into JOSM core.

  • they have nothing in common

Pretty much. Mapillary just has one image, Streetside has six. And Mapillary's image is restricted to 2048px in one dimension. Streetside uses JavaFX. Mapillary does not.

  • Mapillary code is more advanced / robust

The Mapillary plugin implementation is probably better if you have one image and/or don't want to depend on JavaFX. If you have multiple/higher resolution images (i.e., front/top/left/right/back/down), I think Streetside's implementation is better. But I haven't tested both on Java 11+. The Mapillary implementation is a lot more performant on Java 16 than I remember it being on Java 8.

Right now, I believe the Mapillary plugin calculates where each pixel of the image is, instead of what is in the view. It also looks like the Mapillary website has some kind of tiling, so I was starting to write an image tiler so that (a) we don't need the full image in memory and (b) we can just send the tiles of interest to be painted, and only calculate the 3d location of the pixels in the tile (this would make a large difference).

Issues (IIRC) with:

  • Streetside
    • Super-fast panning (Mapillary had the same issue, once I made it dynamic -- probably an easy fix)
    • Camera can become upside down (Mapillary had the same issue, I think I clamped max y)
  • Mapillary
    • Performance? (may be fixed by tiling and not running calculations for all pixels)

comment:19 by Don-vip, 3 years ago

Ah I thought both required JavaFX. It just depends on org.apache.commons.math3 then? How much work it would be to drop this dependency and recode the required math stuff in org.openstreetmap.josm.tools?

Right now I'd like to focus on a simple 360 image viewer (single image) with no dependency, so we could include it in core.

We'll look at StreetSide later, I didn't know it was handling 6 images at once.

comment:20 by Don-vip, 3 years ago

Milestone: 21.08

in reply to:  19 comment:21 by taylor.smock, 3 years ago

Replying to Don-vip:

Ah I thought both required JavaFX. It just depends on org.apache.commons.math3 then? How much work it would be to drop this dependency and recode the required math stuff in org.openstreetmap.josm.tools?

Not difficult at all. IIRC, I'm pretty much just using it for fastmath. I was intending to port everything over to use the vector math code in apache commons math, but I never got around to it. Much like I've never gotten around to merging the image viewing code into JOSM core, although it has long been on my wishlist...
Mapillary used to depend on JavaFX, strictly for a user-friendly date picker UI. I switched to a swing-based library to fix a recurring, hard to debug, bug report.

Right now I'd like to focus on a simple 360 image viewer (single image) with no dependency, so we could include it in core.

We'll look at StreetSide later, I didn't know it was handling 6 images at once.

Yeah. StreetSide is pretty much combining each side of a cube into a 360 image. Probably not useful at all for JOSM core (I don't think most contributors have multiple synchronized cameras), unless we generate the cuboid ourselves from a single image.

in reply to:  19 comment:22 by StephaneP, 3 years ago

Replying to Don-vip:

We'll look at StreetSide later, I didn't know it was handling 6 images at once.

"Cube map" is one of the standard 360 image projection, but equirectangular is more common for local files.

in reply to:  19 ; comment:23 by taylor.smock, 3 years ago

Replying to Don-vip:

Ah I thought both required JavaFX. It just depends on org.apache.commons.math3 then? How much work it would be to drop this dependency and recode the required math stuff in org.openstreetmap.josm.tools?

Right now I'd like to focus on a simple 360 image viewer (single image) with no dependency, so we could include it in core.

We'll look at StreetSide later, I didn't know it was handling 6 images at once.

OK. It looks like I'm using the following imports:

  • org.apache.commons.math3.geometry.euclidean.threed.Rotation: Used to help show user which direction they are pointed in. Not necessary for the initial implementation, but it is a nice-to-have.
  • org.apache.commons.math3.geometry.euclidean.threed.RotationConvention: See above
  • org.apache.commons.math3.geometry.euclidean.threed.RotationOrder: See above
  • org.apache.commons.math3.geometry.euclidean.threed.Vector3D: There was a custom implementation I binned to use the code in apache commons math. I'm in the process of reverting that change.
  • org.apache.commons.math3.util.FastMath: %s/FastMath/Math/g (AKA super easy)

TBH, the main reason why I binned the custom Vector3D is that I was trying to use FastMath to get more performant code. I believe newer Java versions have more performant code.

in reply to:  23 comment:24 by Don-vip, 3 years ago

Replying to taylor.smock:

  • org.apache.commons.math3.geometry.euclidean.threed.Rotation: Used to help show user which direction they are pointed in. Not necessary for the initial implementation, but it is a nice-to-have.
  • org.apache.commons.math3.geometry.euclidean.threed.RotationConvention: See above
  • org.apache.commons.math3.geometry.euclidean.threed.RotationOrder: See above
  • org.apache.commons.math3.geometry.euclidean.threed.Vector3D: There was a custom implementation I binned to use the code in apache commons math. I'm in the process of reverting that change.
  • org.apache.commons.math3.util.FastMath: %s/FastMath/Math/g (AKA super easy)

Let's be iterative. It's OK for the first step to not have the direction arrow, we can add it in a second time.

comment:25 by taylor.smock, 3 years ago

Yep. Iterative is good.

The code in the Mapillary plugin is poorly documented, and the original code predates my involvement. I've been making changes, but I think some of the classes change interpretations -- I'm having to figure out which convention the original programmer used (physics? math?), since that makes a difference for how the symbol (phi, theta) is used. I've been renaming the variables so that they do not depend upon the convention used (physics phi and math theta -> azimuthalAngle, physics theta and math phi -> polarAngle (inclination)), and then documenting "This is the physics phi/theta and the math theta/phi.

And then there is the question of "how were the coordinate axis defined?" I think they used x and y in the image plane.

Anyway, I'll try to start moving it over into the JOSM codebase today. I just don't want it to be poorly documented magic. :)

comment:26 by taylor.smock, 3 years ago

Well, I think I'm going to have to implement some kind of tiling -- Mapillary has only returned 2048 × 1024 images (2,097,152 pixels).

My 360 camera gave me an image with 6080 x 3040 pixels (18,483,200 pixels). That is ~9x more, and it does not work well.

In any case, current WIP is here: https://gitlab.com/smocktaylor/josm/-/merge_requests/10.patch

Current things that need to be done:

  • Modify IImaveViewer interface to get movement information (this is needed to update rotation information).
  • Optimizations
  • ???

in reply to:  26 comment:27 by Don-vip, 3 years ago

Replying to taylor.smock:

My 360 camera gave me an image with 6080 x 3040 pixels (18,483,200 pixels). That is ~9x more, and it does not work well.

Stéphane gave me an HR sample for testing and it's even bigger: 17020 x 8465 pixels (144,074,300 pixels, 41 Mb). You can find it here: download/tmp/Pano_Vieillevigne_1.jpg

in reply to:  26 comment:28 by Don-vip, 3 years ago

Replying to taylor.smock:

In any case, current WIP is here: https://gitlab.com/smocktaylor/josm/-/merge_requests/10.patch

I have some trouble to apply it.

comment:29 by taylor.smock, 3 years ago

I haven't rebased all day. That might be it.

Or it might be how git did the diff (it munges the patches together, and I don't think it simplifies it for patch).

by taylor.smock, 3 years ago

Attachment: 16472.patch added

Initial, mostly working, patch

comment:30 by taylor.smock, 3 years ago

Notes on attachment:16472.patch

Known issues:

  • Panning while zoomed in is kind of jerky (as in, viewport jumps around while dragging (<-, ->, <-, ->)
  • Panning from right to left (<-) creates an empty bar on the right side Fixed. I added a method to ImageDisplay that can be called by IImageViewer implementations (it updates the visible rectangle).
  • When the image display is not in the sidebar (docked), zooming really messes things up. Making the image display docked and zooming once again fixes the issue. Fixed. I added a method to ImageDisplay that can be called by IImageViewer implementations (it updates the visible rectangle).
  • Full resolution is not displayed (pro: Pano_Viellevigne_1.jpg just works, con: cannot zoom in to full resolution)
    • Not related to current patch -- see #20813 for the subsampling change
  • Performance at small viewports is great, really starts slowing down at larger viewports. Still usable, just jerky. Java 16 is better than Java 8.
Last edited 3 years ago by taylor.smock (previous) (diff)

by taylor.smock, 3 years ago

Attachment: 16472.2.patch added

WIP: DO NOT APPLY -- this code applies to attachment:16472.patch, and is an attempt to figure out where azimuth/polar angles are being flipped

comment:31 by Don-vip, 3 years ago

Milestone: 21.0821.09

by taylor.smock, 3 years ago

Attachment: 16472.3.patch added

comment:32 by taylor.smock, 3 years ago

Notes on attachment:16472.3.patch

  • When resizing window, the orientation of the viewpoint changes -- this is due to something, somewhere, flipping angles. I wouldn't call this a show-stopper (the Mapillary viewport just resets to the the vector set in initialization). This can be removed if necessary (just drop Equirectangular.java#L82 (temporaryCameraPlane.setRotation(currentRotation);)).

EDIT:
There should be no plugin incompatibilities -- see https://gitlab.com/smocktaylor/josm/-/pipelines/363568437 (all possible incompatibilities are from the HistoryComboBox changes).

I still need to check and make certain that the tests pass.

EDIT 2: Tests aren't passing -- this is due to a change to make things "work", but which completely ruins the expected values. (this is related to the comment for attachment:16472.2.patch)

Last edited 3 years ago by taylor.smock (previous) (diff)

by taylor.smock, 3 years ago

Attachment: 16472.4.patch added

Working 360 viewer, minimal tests, many tests disabled due to failures, but is usable and can be merged

by taylor.smock, 3 years ago

Attachment: 16472.5.patch added

Test work for attachment:16472.4.patch . THIS BREAKS IMAGE PANNING! DO NOT APPLY!

comment:33 by taylor.smock, 3 years ago

I'm going to take a break on this for awhile, and come back with fresh eyes. Most of my time spent this week has been trying to figure out how to fix an issue in CameraPlane due to the tests and expected tests I used for the Vector3D class.

Anyway, if a bus runs over me:

  • attachment:16472.4.patch can be applied.
  • attachment:16472.5.patch should not be applied. It is largely tests for 16472.4.patch, with fixes for expected test behavior. IMO, the equations are more consistent with what should be reality, but I've got to be missing something. Hopefully it jumps out to me in a week or two. (I've got to fix the camera roll).

comment:34 by Don-vip, 3 years ago

Sorry I didn't have enough time in the past 4 weeks to take a look :(
Should I apply attachment:16472.4.patch​, or do you have since a better version?

in reply to:  34 comment:35 by taylor.smock, 3 years ago

Replying to Don-vip:

Sorry I didn't have enough time in the past 4 weeks to take a look :(
Should I apply attachment:16472.4.patch​, or do you have since a better version?

Go ahead and apply attachment:16472.4.patch . I don't currently have a better version. It works, but there is a subtle bug that has been present in the Mapillary code for years. It does not affect JOSM, as it has to do with drawing additional items in the image view (in Mapillary's case, detections).

I still want to fix that bug (it is easy to find, but has been a pain to debug).

comment:36 by Don-vip, 3 years ago

OK. The patch needs an update though, I have some trouble to apply the changes in ImageDisplay.java (no problem for other files).

by taylor.smock, 3 years ago

Attachment: 16472.6.patch added

in reply to:  36 comment:37 by taylor.smock, 3 years ago

Replying to Don-vip:

OK. The patch needs an update though, I have some trouble to apply the changes in ImageDisplay.java (no problem for other files).

Fixed. :)

comment:38 by Don-vip, 3 years ago

In 18246/josm:

see #16472 - add initial working 360 viewer, minimal tests, many tests disabled due to failures, but is usable (patch by taylor.smock)

comment:39 by StephaneP, 3 years ago

Thanks taylor.smock and Don-vip! That's a good start!

@taylor.smock Which image's metadata is checked to detect if the image is a spherical image?

I found one small regression:
If you use keyboard shortcuts to navigate thru the images and reach the end of the sequence, the viewer displays "no image" and you can't go back to the previous image, you have to click on a picture on the geoimage layer.

Last edited 3 years ago by StephaneP (previous) (diff)

in reply to:  39 ; comment:40 by taylor.smock, 3 years ago

Replying to StephaneP:

Thanks taylor.smock and Don-vip! That's a good start!

@taylor.smock Which image metadata is checked to detect if the image is a spherical image?

I found one small regression:
If you use keyboard shortcuts to navigate thru the images and reach the end of the sequence, the viewer displays "no image" and you can't go back to the previous image, you have to click on a picture on the geoimage layer.

I'm using the XMP GPano:ProjectionType. See source:trunk/src/org/openstreetmap/josm/data/gpx/GpxImageEntry.java@18246:767-776#L767 .

I didn't see anything in EXIF for panoramic images, and all the test images (not downloaded from Mapillary) that I had used that particular key.

Thanks for the bug report. I thought I checked that, but I'll double-check. While I'm double checking, if you are running a local build, can you check if it exists after running ant clean dist? Sometimes some bad behavior can creep in when not doing a clean build.

EDIT: You made a change Which image -> Which image's. The detection is done on a per-image basis.

Last edited 3 years ago by taylor.smock (previous) (diff)

in reply to:  39 comment:41 by taylor.smock, 3 years ago

Replying to StephaneP:

I found one small regression:
If you use keyboard shortcuts to navigate thru the images and reach the end of the sequence, the viewer displays "no image" and you can't go back to the previous image, you have to click on a picture on the geoimage layer.

Reproduced. It looks like the shortcut isn't disabled with the button. I'll look into that today.

in reply to:  40 ; comment:42 by StephaneP, 3 years ago

Replying to taylor.smock:

I'm using the XMP GPano:ProjectionType. See source:trunk/src/org/openstreetmap/josm/data/gpx/GpxImageEntry.java@18246:767-776#L767 .

I didn't see anything in EXIF for panoramic images, and all the test images (not downloaded from Mapillary) that I had used that particular key.

Perhaps you can OR on this XMP exiftag : ProjectionType=equirectangular That's what tools like Hugin adds on pano.

Reproduced. It looks like the shortcut isn't disabled with the button. I'll look into that today.

Thanks!

by taylor.smock, 3 years ago

Attachment: 16472.fix_shortcuts.patch added

Fix shortcuts (add a listener for enable state change on button creation)

comment:43 by taylor.smock, 3 years ago

In attachment:16472.fix_shortcuts.patch, I add a property change listener to created buttons for the enabled property. I chose to go the listener route instead of adding btn.getAction().setEnabled everywhere, since I am much more likely to miss one if I go that route. It should also be more robust in the future so that we don't have to remember to update the enabled state of the action.

It used to not matter since ImageData didn't allow next/previous/first/last to deselect data, but with the change, we were effectively deselecting data at the start/end of the data layer.

in reply to:  42 ; comment:44 by taylor.smock, 3 years ago

Replying to StephaneP:

Perhaps you can OR on this XMP exiftag : ProjectionType=equirectangular That's what tools like Hugin adds on pano.

Do you have a sample picture (i.e., one that doesn't work with current code)? I tried making a pano with Hugin on Mac and it worked just fine.

Last edited 3 years ago by taylor.smock (previous) (diff)

in reply to:  44 ; comment:45 by StephaneP, 3 years ago

Replying to taylor.smock:

Do you have a sample picture (i.e., one that doesn't work with current code)? I tried making a pano with Hugin on Mac and it worked just fine.

I'm sorry, you're right, it seems the blurring tools I've used on these pictures deleted some metadata. I'll try to investigate this morning.

comment:46 by Don-vip, 3 years ago

In 18247/josm:

see #16472 - Fix shortcuts (add a listener for enable state change on button creation, patch by taylor.smock)

by StephaneP, 3 years ago

picture not detected as a 360 image

in reply to:  45 comment:47 by StephaneP, 3 years ago

Replying to StephaneP:

Replying to taylor.smock:

Do you have a sample picture (i.e., one that doesn't work with current code)? I tried making a pano with Hugin on Mac and it worked just fine.

I'm sorry, you're right, it seems the blurring tools I've used on these pictures deleted some metadata. I'll try to investigate this morning.

I found some pictures which seem embed the right metadata, but is not detected as a 360 image:

picture not detected as a 360 image

comment:48 by taylor.smock, 3 years ago

It is working for me. Are you running a local build? If so, please run svn up; ant clean dist, and try again. If not, please give me the build information and/or update to today's JOSM latest.

EDIT: And by working for me, I mean with today's josm-latest.jar (curl -LO https://josm.openstreetmap.de/josm-latest.jar ; java -jar josm-latest.jar).

Last edited 3 years ago by taylor.smock (previous) (diff)

comment:49 by StephaneP, 3 years ago

I was using r18248
And now....it works. I don't understand.

Ok....got it!
I've added some gps coordinates before uploading the picture here. If I remove these metadata with exiftool -gps:all= your_filename then it's not detected as a pano anymore.

in reply to:  49 comment:50 by taylor.smock, 3 years ago

Replying to StephaneP:

I was using r18248
And now....it works. I don't understand.

Ok....got it!
I've added some gps coordinates before uploading the picture here. If I remove these metadata with exiftool -gps:all= your_filename then it's not detected as a pano anymore.

Well, now I know what the problem is.

JOSM short-circuits reading image metadata when there is no gps tags.

See source:trunk/src/org/openstreetmap/josm/data/gpx/GpxImageEntry.java@18247:733-737#L733 , which comes prior to source:trunk/src/org/openstreetmap/josm/data/gpx/GpxImageEntry.java@18247:767-775#L767 .

It might be worth moving IptcDirectory and XmpDirectory items prior to the dirGps check.

by taylor.smock, 3 years ago

Move xmp and iptc parsing to before gps short circuit

comment:51 by Don-vip, 3 years ago

In 18252/josm:

see #16472 - Move xmp and iptc parsing to before gps short circuit (patch by taylor.smock)

comment:52 by Don-vip, 3 years ago

Taylor, coverity detected three new defects, can you please check?

New defect(s) Reported-by: Coverity Scan
Showing 3 of 3 defect(s)


** CID 1464108:  API usage errors  (SWAPPED_ARGUMENTS)


________________________________________________________________________________________________________
*** CID 1464108:  API usage errors  (SWAPPED_ARGUMENTS)
/src/org/openstreetmap/josm/gui/util/imagery/CameraPlane.java: 182 in org.openstreetmap.josm.gui.util.imagery.CameraPlane.setRotation(org.openstreetmap.josm.gui.util.imagery.Vector3D)()
176         /**
177          * Set camera plane rotation by spherical vector.
178          *
179          * @param vec vector pointing new view position.
180          */
181         public void setRotation(Vector3D vec) {
>>>     CID 1464108:  API usage errors  (SWAPPED_ARGUMENTS)
>>>     The positions of arguments in the call to "setRotation" do not match the ordering of the parameters:
* "vec.getPolarAngle()" is passed to "azimuthalAngle"
* "vec.getAzimuthalAngle()" is passed to "polarAngle"
182             setRotation(vec.getPolarAngle(), vec.getAzimuthalAngle());
183         }
184     
185         public Vector3D getRotation() {
186             return this.rotation;
187         }

** CID 1464107:  SpotBugs: Multithreaded correctness  (FB.UG_SYNC_SET_UNSYNC_GET)
/src/org/openstreetmap/josm/gui/util/imagery/CameraPlane.java: 185 in org.openstreetmap.josm.gui.util.imagery.CameraPlane.getRotation()()


________________________________________________________________________________________________________
*** CID 1464107:  SpotBugs: Multithreaded correctness  (FB.UG_SYNC_SET_UNSYNC_GET)
/src/org/openstreetmap/josm/gui/util/imagery/CameraPlane.java: 185 in org.openstreetmap.josm.gui.util.imagery.CameraPlane.getRotation()()
179          * @param vec vector pointing new view position.
180          */
181         public void setRotation(Vector3D vec) {
182             setRotation(vec.getPolarAngle(), vec.getAzimuthalAngle());
183         }
184     
>>>     CID 1464107:  SpotBugs: Multithreaded correctness  (FB.UG_SYNC_SET_UNSYNC_GET)
>>>     org.openstreetmap.josm.gui.util.imagery.CameraPlane.getRotation() is unsynchronized, org.openstreetmap.josm.gui.util.imagery.CameraPlane.setRotation(double, double) is synchronized.
185         public Vector3D getRotation() {
186             return this.rotation;
187         }
188     
189         synchronized void setRotation(double azimuthalAngle, double polarAngle) {
190             // Note: Something, somewhere, is switching the two.

** CID 1464106:  SpotBugs: Dodgy code  (FB.FE_FLOATING_POINT_EQUALITY)
/src/org/openstreetmap/josm/gui/util/imagery/Vector3D.java: 239 in org.openstreetmap.josm.gui.util.imagery.Vector3D.equals(java.lang.Object)()


________________________________________________________________________________________________________
*** CID 1464106:  SpotBugs: Dodgy code  (FB.FE_FLOATING_POINT_EQUALITY)
/src/org/openstreetmap/josm/gui/util/imagery/Vector3D.java: 239 in org.openstreetmap.josm.gui.util.imagery.Vector3D.equals(java.lang.Object)()
233         }
234     
235         @Override
236         public boolean equals(Object o) {
237             if (o instanceof Vector3D) {
238                 Vector3D other = (Vector3D) o;
>>>     CID 1464106:  SpotBugs: Dodgy code  (FB.FE_FLOATING_POINT_EQUALITY)
>>>     Test for floating point equality.
239                 return this.x == other.x && this.y == other.y && this.z == other.z;
240             }
241             return false;
242         }
243     
244         @Override

in reply to:  52 comment:53 by taylor.smock, 3 years ago

Replying to Don-vip:

Taylor, coverity detected three new defects, can you please check?

Will do. I know that at least one of those is "this doesn't make sense, but changing it borks rendering and/or panning".

Specifically, "SWAPPED_ARGUMENTS" in "source:trunk/src/org/openstreetmap/josm/gui/util/imagery/CameraPlane.java#L182 in org.openstreetmap.josm.gui.util.imagery.CameraPlane.setRotation(org.openstreetmap.josm.gui.util.imagery.Vector3D)()".

IIRC, that is one of the things I fixed for tests, but since it ruins rendering, cannot be merged in. I did actually comment in the setRotation(double, double) method that something, somewhere was switching them, and in the tests patch, I think I do have the arguments reversed.

Which, as I mentioned, ruins rendering, and I haven't figured out how to fix that and make the code make sense (case in point, SWAPPED_ARGUMENTS).

For FB.UG_SYNC_SET_UNSYNC_GET, I don't know why setRotation is synchronized. It was introduced in Mapillary e96a558add three years ago by floscher. I don't know about floscher, but I don't think I would remember why I did something like that three months ago, much less three years ago.

We can probably synchronize getRotation and see what happens. It shouldn't harm performance too much (only call for that method is in Equirectangular, and is only called by getRotation() and componentResized(ComponentEvent)). The getRotation() method call in Equirectangular isn't used yet, but probably won't be hammered.

Finally, for FB.FE_FLOATING_POINT_EQUALITY, it is in the equals method. I'd have to look into what makes sense for those. I'll probably start with 1e-8 for the nearness test. Maybe more, maybe less. But most of the vector values are going to be integers (Vector3D is largely called using image coordinates as inputs). Or I'll just use Utils.equalsEpsilon.

by taylor.smock, 3 years ago

Attachment: 16472.coverity_fixes.patch added

Fix coverity FB.FE_FLOATING_POINT_EQUALITY and FB.UG_SYNC_SET_UNSYNC_GET

comment:54 by Don-vip, 3 years ago

In 18255/josm:

see #16472 - Fix coverity FB.FE_FLOATING_POINT_EQUALITY and FB.UG_SYNC_SET_UNSYNC_GET (patch by taylor.smock)

comment:55 by Don-vip, 3 years ago

Mmm I just got a crash while panning Pano_Viellevigne_1.jpg:

Revision:18253
Is-Local-Build:true
Build-Date:2021-10-06 13:19:51

Identification: JOSM/1.5 (18253 SVN fr) Windows 10 64-Bit
OS Build number: Windows 10 Pro 2009 (19042)
Memory Usage: 778 MB / 4088 MB (414 MB allocated, but free)
Java version: 16.0.1+9, AdoptOpenJDK, OpenJDK 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 2560×1440 (scaling 1.00×1.00) \Display1 1920×1080 (scaling 1.00×1.00)
Maximum Screen Size: 2560×1440
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: UTF-8
System property sun.jnu.encoding: Cp1252
Locale info: fr_FR
Numbers with default locale: 1234567890 -> 1234567890
VM arguments: [-ea, --add-opens=java.base/java.io=ALL-UNNAMED, --add-opens=java.base/java.lang=ALL-UNNAMED, --add-opens=java.base/java.nio=ALL-UNNAMED, --add-opens=java.base/java.text=ALL-UNNAMED, --add-opens=java.base/java.util=ALL-UNNAMED, --add-opens=java.base/jdk.internal.loader=ALL-UNNAMED, --add-opens=java.desktop/java.awt=ALL-UNNAMED, --add-opens=java.desktop/javax.swing.text.html=ALL-UNNAMED, --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED, --module-path=C:\Java\javafx-sdk-16\lib, --add-modules=java.scripting,java.sql,javafx.controls,javafx.media,javafx.swing,javafx.web, -Dfile.encoding=UTF-8]
Program arguments: [--debug]

Last errors/warnings:
- 00000.910 W: extended font config - overriding 'filename.Myanmar_Text=mmrtext.ttf' with 'MMRTEXT.TTF'
- 00000.912 W: extended font config - overriding 'filename.Mongolian_Baiti=monbaiti.ttf' with 'MONBAITI.TTF'
- 00081.646 E: Handled by bug report queue: java.lang.ArrayIndexOutOfBoundsException: Index 285 out of bounds for length 285

=== REPORTED CRASH DATA ===
BugReportExceptionHandler#handleException:
No data collected.

Warning issued by: BugReportExceptionHandler#handleException

=== STACK TRACE ===
Thread: AWT-EventQueue-0 (22) of main
java.lang.ArrayIndexOutOfBoundsException: Index 285 out of bounds for length 285
	at org.openstreetmap.josm.gui.util.imagery.CameraPlane.setRotationFromDelta(CameraPlane.java:168)
	at org.openstreetmap.josm.gui.layer.geoimage.viewers.projections.Equirectangular.mouseDragged(Equirectangular.java:98)
	at org.openstreetmap.josm.gui.layer.geoimage.ImageDisplay$ImgDisplayMouseListener.mouseDragged(ImageDisplay.java:505)
	at java.desktop/java.awt.Component.processMouseMotionEvent(Component.java:6665)
	at java.desktop/javax.swing.JComponent.processMouseMotionEvent(JComponent.java:3360)
	at java.desktop/java.awt.Component.processEvent(Component.java:6386)
	at java.desktop/java.awt.Container.processEvent(Container.java:2264)
	at java.desktop/java.awt.Component.dispatchEventImpl(Component.java:4993)
	at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2322)
	at java.desktop/java.awt.Component.dispatchEvent(Component.java:4825)
	at java.desktop/java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4934)
	at java.desktop/java.awt.LightweightDispatcher.processMouseEvent(Container.java:4580)
	at java.desktop/java.awt.LightweightDispatcher.dispatchEvent(Container.java:4504)
	at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2308)
	at java.desktop/java.awt.Window.dispatchEventImpl(Window.java:2773)
	at java.desktop/java.awt.Component.dispatchEvent(Component.java:4825)
	at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:772)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:721)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:715)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:391)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:95)
	at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:745)
	at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:743)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:391)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85)
	at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:742)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)

reproduced in debug:

java.lang.ArrayIndexOutOfBoundsException: Index 247 out of bounds for length 247

- from = java.awt.Point[x=275,y=237]
- to = java.awt.Point[x=277,y=247]
- f1 = [x=-0.12866953370492973, y=0.46361879604792133, z=0.8766423233264704, r=1.0, inclination=-0.1457348292682141, azimuthal=1.088721212255313]
- this.vectors[to.x].length = 247
Last edited 3 years ago by Don-vip (previous) (diff)

in reply to:  55 comment:56 by taylor.smock, 3 years ago

Replying to Don-vip:

Mmm I just got a crash while panning Pano_Viellevigne_1.jpg:

And I was unable to reproduce (up/down extremes, full 360). But it should be fairly easy to fix. And there is a check for that sort of thing right before the crash location.

EDIT: About the only way to get that is to have some kind of non-square matrix, which shouldn't happen. But obviously did.

Last edited 3 years ago by taylor.smock (previous) (diff)

by taylor.smock, 3 years ago

Attachment: 16472.AIOOBE.patch added

comment:57 by Don-vip, 3 years ago

it takes some time to reproduce, I scroll / zoom in all directions without problem for about 1 minute before triggering the bug, it's strange.

in reply to:  57 comment:58 by taylor.smock, 3 years ago

Replying to Don-vip:

it takes some time to reproduce, I scroll / zoom in all directions without problem for about 1 minute before triggering the bug, it's strange.

OK. I'll see if I can repro without the patch and with the patch. I wonder if you have to do the drag at exactly the border (max x/y).

comment:59 by Don-vip, 3 years ago

In 18256/josm:

see #16472 - AIOOBE (patch by taylor.smock)

comment:60 by Don-vip, 3 years ago

In 18263/josm:

see #16472 - draw direction arrow of 360 images

comment:61 by Don-vip, 3 years ago

Milestone: 21.0921.10

Milestone renamed

in reply to:  60 ; comment:62 by taylor.smock, 3 years ago

Replying to Don-vip:

In 18263/josm:

see #16472 - draw direction arrow of 360 images

I was going to use compass direction when I got around to implementing that, but I like your method better. :)

in reply to:  62 ; comment:63 by Don-vip, 3 years ago

Replying to taylor.smock:

I was going to use compass direction when I got around to implementing that, but I like your method better. :)

Thanks! I started to look at the scaling / performance issue but that's harder. I disabled the subsampling code from #20813 and still couldn't display the image in full resolution, I don't understand yet why.

in reply to:  63 comment:64 by taylor.smock, 3 years ago

Replying to Don-vip:

Replying to taylor.smock:

I was going to use compass direction when I got around to implementing that, but I like your method better. :)

Thanks! I started to look at the scaling / performance issue but that's harder. I disabled the subsampling code from #20813 and still couldn't display the image in full resolution, I don't understand yet why.

This is due to how zooming works in the image viewer at this time.

What happens right now is we only have one image buffer. The size of the image buffer is determined at time of image load based off of the size of the component. This means that we only have that number of pixels available in that buffered image, so zooming in will only really help when something is not easily visible. This does help work around performance issues by allowing users to make the window smaller for better performance.

This has been on my list of things to fix (I was going to go for a zoom/tiling approach, since AFAIK most street level imagery providers use some kind of tiling for images), but it hasn't been super critical yet.

EDIT: Image loading is really expensive in terms of memory used and time, so we have a single buffered image that we do not modify. At least, this is how it works on the 360 side. I copied/pasted the code for standard images, but I think it works the same way.

Last edited 3 years ago by taylor.smock (previous) (diff)

by taylor.smock, 3 years ago

Attachment: 16472.load_all.patch added

DO NOT APPLY -- load the entire image into memory, with attendent calculations (this is very expensive). There is also a UI bug with this patch.

in reply to:  63 comment:65 by taylor.smock, 3 years ago

Replying to Don-vip:

Thanks! I started to look at the scaling / performance issue but that's harder. I disabled the subsampling code from #20813 and still couldn't display the image in full resolution, I don't understand yet why.

I've attached a patch that should allow you to load an image at full resolution. If you are using download/tmp/Pano_Vieillevigne_1.jpg, you should (a) be prepared to wait awhile (something like 144,074,300 vector calculations, which is not cheap) and (b) have allocated sufficient memory (around 580 MB).

As noted in the patch, don't apply it. Not only is it not going to work well with large images (due to many calculations), it does mess up zoom.

comment:66 by Don-vip, 3 years ago

In 18300/josm:

see #16472 - allow to open geoimages into external viewer, useful for 360 images until we can display them in full resolution

comment:67 by Don-vip, 3 years ago

Milestone: 21.1021.11

in reply to:  66 ; comment:68 by skyper, 3 years ago

Replying to Don-vip:

In 18300/josm:

see #16472 - allow to open geoimages into external viewer, useful for 360 images until we can display them in full resolution

Is this only a temporary solution? If not, it would be nice to specify the external program similar to the web browser.

in reply to:  68 comment:69 by skyper, 3 years ago

Replying to skyper:

Replying to Don-vip:

In 18300/josm:

see #16472 - allow to open geoimages into external viewer, useful for 360 images until we can display them in full resolution

Is this only a temporary solution? If not, it would be nice to specify the external program similar to the web browser.

Actually, I am not sure if this new button needs to be documented.

Last edited 3 years ago by skyper (previous) (diff)

comment:70 by Don-vip, 3 years ago

Milestone: 21.1121.12

Milestone renamed

comment:71 by Don-vip, 3 years ago

Milestone: 21.1222.01

comment:72 by stoecker, 3 years ago

Milestone: 22.0122.02

Milestone renamed

comment:73 by Don-vip, 3 years ago

Milestone: 22.0222.03

comment:74 by Corentin LEMAITRE, 3 years ago

I have used the 360 picture viewer embeded in JOSM, it works very well, so first thank you for this first ! :+1:

I have only a little bug to report when I save a JOSM session. When I open pictures in a new JOSM session they are detected as 360 images with a pano view. If I save my session in a JOZ file and open it with JOSM then the picture are not opened as 360 pictures by the viewer but as flat picture.

It would be really useful because extracting geolocation of a big number of picture could take 10 minutes or more.

in reply to:  74 comment:75 by corentinlemaitre, 3 years ago

It has been reported in ticket #21884 so i will post my message in the correct ticket.

Replying to Corentin LEMAITRE:

I have used the 360 picture viewer embeded in JOSM, it works very well, so first thank you for this first ! :+1:

I have only a little bug to report when I save a JOSM session. When I open pictures in a new JOSM session they are detected as 360 images with a pano view. If I save my session in a JOZ file and open it with JOSM then the picture are not opened as 360 pictures by the viewer but as flat picture.

It would be really useful because extracting geolocation of a big number of picture could take 10 minutes or more.

comment:76 by stoecker, 3 years ago

Milestone: 22.0322.04

comment:77 by stoecker, 3 years ago

Milestone: 22.04

comment:78 by richlv, 6 weeks ago

Sorry if I'm misunderstanding something - is this ticket still open to deal with the open items, mentioned in https://josm.openstreetmap.de/ticket/16472#comment:30 (full zoom, performance)?

I assume full image would be loaded only upon zooming in, otherwise resource usage would explode - is that correct?

Modify Ticket

Change Properties
Set your email in Preferences
Action
as assigned The owner will remain Don-vip.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from Don-vip to the specified user. Next status will be 'new'.
Next status will be 'needinfo'. The owner will be changed from Don-vip to StephaneP.
as duplicate The resolution will be set to duplicate. Next status will be 'closed'. The specified ticket will be cross-referenced with this ticket.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.