search

CARTAvis / carta-backend

Source code repository for the backend component of CARTA, a new visualization tool designed for the ALMA, the VLA and the SKA pathfinders.
https://cartavis.github.io/
GNU General Public License v3.0
22 stars 11 forks source link

Fix FITS header and data errors #1255

Closed pford closed 1 year ago

pford commented 1 year ago

Description

  1. CompressedFits: fix header parsing error for "COMMENT ================================"
  2. CartaFitsImage: read rescaled 16-bit int data correctly with cfitsio (using equivalent bitpix=float)
  3. FileExtInfoLoader: fix unit string ("degrees") in FITS image header unrecognized by casacore (regression failure)

Checklist

kswang1029 commented 1 year ago

@pford with the same test image from the issue, we can see the file info and file header in the file browser. However, if we load it, the backend will crash with the following carta_backend log and macOS crash log:

[2023-04-28 08:10:35.173] [CARTA] [debug] required mem=411 kB, free mem=8384588 kB, access file in memory=true carta_backend(25275,0x102a6b600) malloc: error for object 0x6000015d9400: pointer being freed was not allocated carta_backend(25275,0x102a6b600) malloc: set a breakpoint in malloc_error_break to debug Abort trap: 6

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 libsystem_kernel.dylib 0x7ff80321200e pthread_kill + 10 1 libsystem_pthread.dylib 0x7ff8032481ff pthread_kill + 263 2 libsystem_c.dylib 0x7ff803193d24 abort + 123 3 libsystem_malloc.dylib 0x7ff803071357 malloc_vreport + 551 4 libsystem_malloc.dylib 0x7ff80307452b malloc_report + 151 5 carta_backend 0x10a22aeb1 void std::1::libcpp_operator_delete<void>(void) + 5 (new:245) [inlined] 6 carta_backend 0x10a22aeb1 void std::1::do_deallocate_handle_size<>(void*, unsigned long) + 5 (new:269) [inlined] 7 carta_backend 0x10a22aeb1 std::1::libcpp_deallocate(void*, unsigned long, unsigned long) + 5 (new:285) [inlined] 8 carta_backend 0x10a22aeb1 std::1::allocator::deallocate(char, unsigned long) + 5 (allocator.h:117) [inlined] 9 carta_backend 0x10a22aeb1 std::__1::allocator_traits<std::1::allocator >::deallocate(std::1::allocator&, char, unsigned long) + 5 (allocator_traits.h:282) [inlined] 10 carta_backend 0x10a22aeb1 std::1::basic_string<char, std::__1::char_traits, std::1::allocator >::~basic_string() + 15 (string:2303) [inlined] 11 carta_backend 0x10a22aeb1 std::1::basic_string<char, std::__1::char_traits, std::1::allocator >::~basic_string() + 15 (string:2298) [inlined] 12 carta_backend 0x10a22aeb1 carta::CartaFitsImage::GetFitsHeaderString(int&, std::1::basic_string<char, std::__1::char_traits, std::1::allocator >&) + 1041 (CartaFitsImage.cc:397) 13 carta_backend 0x10a2240a8 carta::CartaFitsImage::SetUpImage() + 72 (CartaFitsImage.cc:285) 14 carta_backend 0x10a223eba carta::CartaFitsImage::CartaFitsImage(std::1::basic_string<char, std::__1::char_traits, std::1::allocator > const&, unsigned int) + 426 (CartaFitsImage.cc:45) 15 carta_backend 0x10a257add carta::FitsLoader::OpenFile(std::1::basic_string<char, std::__1::char_traits, std::1::allocator > const&) + 525 (FitsLoader.h:97) 16 carta_backend 0x10a1be911 carta::FileExtInfoLoader::FillFileInfoFromImage(CARTA::FileInfoExtended&, std::1::basic_string<char, std::__1::char_traits, std::1::allocator > const&, std::1::basic_string<char, std::__1::char_traits, std::1::allocator >&) + 49 (FileExtInfoLoader.cc:134) 17 carta_backend 0x10a1be3ee carta::FileExtInfoLoader::FillFileExtInfo(CARTA::FileInfoExtended&, std::1::basic_string<char, std::__1::char_traits, std::1::allocator > const&, std::1::basic_string<char, std::__1::char_traits, std::1::allocator > const&, std::1::basic_string<char, std::__1::char_traits, std::1::allocator >&) + 478 (FileExtInfoLoader.cc:104) 18 carta_backend 0x10a3d3020 carta::Session::FillExtendedFileInfo(CARTA::FileInfoExtended&, CARTA::FileInfo&, std::1::basic_string<char, std::__1::char_traits, std::1::allocator > const&, std::1::basic_string<char, std::__1::char_traits, std::1::allocator > const&, std::1::basic_string<char, std::__1::char_traits, std::1::allocator >&, std::1::basic_string<char, std::__1::char_traits, std::1::allocator >&, std::1::basic_string<char, std::__1::char_traits, std::1::allocator >&) + 288 (Session.cc:308) 19 carta_backend 0x10a3d7d54 carta::Session::OnOpenFile(CARTA::OpenFile const&, unsigned int, bool) + 628 (Session.cc:498) 20 carta_backend 0x10a3fc38a carta::SessionManager::OnMessage(uWS::WebSocket<false, true, carta::PerSocketData>, std::1::basic_string_view<char, std::1::char_traits >, uWS::OpCode) + 3866 (SessionManager.cc:263) 21 carta_backend 0x10a404064 ofats::any_detail::any_invocable_impl<void, false, uWS::WebSocket<false, true, carta::PerSocketData>, std::1::basic_string_view<char, std::1::char_traits >, uWS::OpCode>::call(uWS::WebSocket<false, true, carta::PerSocketData>, std::1::basic_string_view<char, std::1::char_traits >, uWS::OpCode) + 17 (MoveOnlyFunction.h:247) [inlined] 22 carta_backend 0x10a404064 ofats::any_invocable<void (uWS::WebSocket<false, true, carta::PerSocketData>, std::1::basic_string_view<char, std::1::char_traits >, uWS::OpCode)>::operator()(uWS::WebSocket<false, true, carta::PerSocketData>, std::1::basic_string_view<char, std::1::char_traits >, uWS::OpCode) + 17 (MoveOnlyFunction.h:354) [inlined] 23 carta_backend 0x10a404064 uWS::WebSocketContext<false, true, carta::PerSocketData>::handleFragment(char, unsigned long, unsigned int, int, bool, uWS::WebSocketState, void) + 1300 (WebSocketContext.h:93) 24 carta_backend 0x10a4032a6 bool uWS::WebSocketProtocol<true, uWS::WebSocketContext<false, true, carta::PerSocketData> >::consumeMessage<6u, unsigned char>(unsigned char, char&, unsigned int&, uWS::WebSocketState, void) + 758 (WebSocketProtocol.h:328) 25 carta_backend 0x10a402f00 uWS::WebSocketProtocol<true, uWS::WebSocketContext<false, true, carta::PerSocketData> >::consume(char, unsigned int, uWS::WebSocketState, void) + 272 (WebSocketProtocol.h:424) 26 carta_backend 0x10a402d9a auto uWS::WebSocketContext<false, true, carta::PerSocketData>::init()::'lambda'(auto, char, int)::operator()(auto, char, int) const + 138 (WebSocketContext.h:286) 27 carta_backend 0x10a4b7065 us_loop_run + 133 (epoll_kqueue.c:147) 28 carta_backend 0x10a3ff8e7 uWS::Loop::run() + 8 (Loop.h:159) [inlined] 29 carta_backend 0x10a3ff8e7 uWS::run() + 15 (Loop.h:176) [inlined] 30 carta_backend 0x10a3ff8e7 uWS::TemplatedApp::run() + 15 (App.h:393) [inlined] 31 carta_backend 0x10a3ff8e7 carta::SessionManager::RunApp() + 343 (SessionManager.cc:574) 32 carta_backend 0x10a33b4e5 main + 3877 (Main.cc:189) 33 dyld 0x10c84852e start + 462

confluence commented 1 year ago

I can confirm that there's still an issue on Ubuntu 20.04 -- I get a segfault when opening the gzipped file.

confluence commented 1 year ago

The backend does not segfault when I use valgrind, but I do get this debugging output, which seems to match what @kswang1029 found:

==706114== Memcheck, a memory error detector
==706114== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==706114== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==706114== Command: ./carta_backend --frontend_folder ../../carta-frontend/build
==706114== 
[2023-04-28 10:04:25.919] [CARTA] [info] Writing to the log file: /home/adrianna/.carta/log/carta.log
[2023-04-28 10:04:25.984] [CARTA] [info] /home/adrianna/repos/carta-backend/build/carta_backend: Version 3.0.0
[2023-04-28 10:04:26.112] [CARTA] [info] Serving CARTA frontend from /home/adrianna/repos/carta-frontend/build
[2023-04-28 10:04:26.142] [CARTA] [info] Listening on port 3002 with top level folder /, starting folder /home/adrianna/repos/carta-backend/build. The number of OpenMP worker threads will be handled automatically.
[2023-04-28 10:04:26.201] [CARTA] [info] CARTA is accessible at http://localhost:3002/?token=7d241321-c6a3-4929-8b0b-2987fa0a7614
[2023-04-28 10:04:29.432] [CARTA] [info] 0xcaf2800 ::Session (2572075271:1)
[2023-04-28 10:04:29.437] [CARTA] [info] Session 2572075271 [127.0.0.1] Connected. Num sessions: 1
==706114== Invalid free() / delete / delete[] / realloc()
==706114==    at 0x483CFBF: operator delete(void*) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==706114==    by 0x34BB65: deallocate (new_allocator.h:128)
==706114==    by 0x34BB65: deallocate (alloc_traits.h:469)
==706114==    by 0x34BB65: _M_destroy (basic_string.h:241)
==706114==    by 0x34BB65: _M_dispose (basic_string.h:236)
==706114==    by 0x34BB65: ~basic_string (basic_string.h:662)
==706114==    by 0x34BB65: carta::CartaFitsImage::GetFitsHeaderString(int&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (CartaFitsImage.cc:391)
==706114==    by 0x3523C3: carta::CartaFitsImage::SetUpImage() (CartaFitsImage.cc:285)
==706114==    by 0x3529A7: carta::CartaFitsImage::CartaFitsImage(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int) (CartaFitsImage.cc:45)
==706114==    by 0x37C8B1: carta::FitsLoader::OpenFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (FitsLoader.h:97)
==706114==    by 0x28515B: carta::FileExtInfoLoader::FillFileInfoFromImage(CARTA::FileInfoExtended&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (FileExtInfoLoader.cc:134)
==706114==    by 0x286729: carta::FileExtInfoLoader::FillFileExtInfo(CARTA::FileInfoExtended&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (FileExtInfoLoader.cc:104)
==706114==    by 0x4C2D92: carta::Session::FillExtendedFileInfo(CARTA::FileInfoExtended&, CARTA::FileInfo&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (Session.cc:308)
==706114==    by 0x4D5444: carta::Session::OnOpenFile(CARTA::OpenFile const&, unsigned int, bool) (Session.cc:498)
==706114==    by 0x4EAED5: carta::SessionManager::OnMessage(uWS::WebSocket<false, true, carta::PerSocketData>*, std::basic_string_view<char, std::char_traits<char> >, uWS::OpCode) (SessionManager.cc:263)
==706114==    by 0x4F7473: call (MoveOnlyFunction.h:247)
==706114==    by 0x4F7473: operator() (MoveOnlyFunction.h:354)
==706114==    by 0x4F7473: uWS::WebSocketContext<false, true, carta::PerSocketData>::handleFragment(char*, unsigned long, unsigned int, int, bool, uWS::WebSocketState<true>*, void*) (WebSocketContext.h:93)
==706114==    by 0x4F7EDD: consumeMessage<6, unsigned char> (WebSocketProtocol.h:328)
==706114==    by 0x4F7EDD: uWS::WebSocketProtocol<true, uWS::WebSocketContext<false, true, carta::PerSocketData> >::consume(char*, unsigned int, uWS::WebSocketState<true>*, void*) (WebSocketProtocol.h:424)
==706114==  Address 0x1fffffffff is not stack'd, malloc'd or (recently) free'd
==706114== 
[2023-04-28 10:04:57.786] [CARTA] [warning] Session 2572075271: Image has no beam information.
confluence commented 1 year ago

OK, I'm pretty sure I know what's causing the segfault. It's happening in the bitpix > 0 branch in CartaFitsImage::GetFitsHeaderString (but isn't always triggered). ~fits_read_key dereferences the char* with the key name~, and this is subsequently causing a segfault when the std::string destructor tries to clean up key.

Removing the std::string key("BLANK") and passing a literal "BLANK" to fits_read_key eliminates the segfault, but there's still something else wrong: instead of the un-gzipped image I get a completely blank, black image. This is the same thing that previously happened when I tried to load it when running valgrind or gdb or compiling the backend in Debug mode rather than RelWithDebInfo.

_Actually, I misread the cfitsio code -- it's not directly dereferencing that pointer. I'm not sure what the issue is -- it's const, so should be safe to use .c_str() here. It should not be modified. So maybe my "fix" is coincidental and this is masking undefined behaviour somewhere else._

pford commented 1 year ago

I think I have fixed the seg fault, changed the type of the BLANK value from TLONG to TINT (variable to hold the value is int). Not sure why it was long.

I will need more time to debug the image data issue, so will postpone to the bug fix phase of v4. Should I move this to a draft PR?

pford commented 1 year ago

While testing code changes with all of my test FITS files, I also fixed the regression issue (header unit string) and fixed a CartaFitsImage spectral coordinate issue using the wcsprm struct with ctype FREQ if ImageFITSConverter::getCoordinateSystem fails.

pford commented 1 year ago

Getting an error opening the fits.gz image on macOS, looking into it.

pford commented 1 year ago

Spectral and stokes were not being set correctly in CompressedFits, so got a Stokes error from casacore. Now these axes are initialized to -1 and set from the CTYPE headers. Should be ready for test/review now.

pford commented 1 year ago

Good catch, @kswang1029 ! Fixed complex image type.

kswang1029 commented 1 year ago

Just realized I missed a header check. Will re-approve once I have done the header check.

confluence commented 1 year ago

I have opened the compressed and uncompressed images from the issue side by side, and while I no longer see the weird value distortion in the compressed image that was happening before when the crash was suppressed, I still can't spatially or spectrally match the compressed image to the uncompressed image or to itself (but I can match the uncompressed image to itself). This suggests that there's still some kind of bug in the parsing of the compressed image's metadata from the header.

kswang1029 commented 1 year ago

I have opened the compressed and uncompressed images from the issue side by side, and while I no longer see the weird value distortion in the compressed image that was happening before when the crash was suppressed, I still can't spatially or spectrally match the compressed image to the uncompressed image or to itself (but I can match the uncompressed image to itself). This suggests that there's still some kind of bug in the parsing of the compressed image's metadata from the header.

We might need to reopen https://github.com/CARTAvis/carta-frontend/issues/2087 @crocka

confluence commented 1 year ago

Do we have an e2e test case which checks that a compressed and uncompressed FITS file result in identical file info (or at least compares some basic properties)? E.g. I can see by using snippets in the GUI that the compressed file here has a lot of null and undefined values where the uncompressed file has valid values.

kswang1029 commented 1 year ago

Do we have an e2e test case which checks that a compressed and uncompressed FITS file result in identical file info (or at least compares some basic properties)? E.g. I can see by using snippets in the GUI that the compressed file here has a lot of null and undefined values where the uncompressed file has valid values.

🤔will look into this

kswang1029 commented 1 year ago

Do we have an e2e test case which checks that a compressed and uncompressed FITS file result in identical file info (or at least compares some basic properties)? E.g. I can see by using snippets in the GUI that the compressed file here has a lot of null and undefined values where the uncompressed file has valid values.

we have two, one for gz and one for fz but those are for the derived file info part, not the full header part.

kswang1029 commented 1 year ago

@pford Using the GASS test image attached to the original issue, I am comparing FITS headers generated with ds9 and CARTA. For the unzipped version, the header is different between ds9 and CARTA. For example, CRVAL3=-4.947000122070E+05 in ds9 and CRVAL3=-4.955176865649E+05 in CARTA (BITPIX is also changed, due to rescaling?). Is this due to the fact that we are seeing casacore processed header, not the "raw" header? If we compare the gz header from CARTA and unzipped header from ds9, we see basically identical result.

pford commented 1 year ago

@kswang1029 I have done additional work on the headers for uncompressed and compressed fits with velocity/wavelength headers. As you suspected, casacore derives its headers from the produced image (FITSImage is always float and spectral axis is always converted to frequency then back to velocity, so you will see a difference in the velocity range). Please compare again.

pford commented 1 year ago

Do we have an e2e test case which checks that a compressed and uncompressed FITS file result in identical file info (or at least compares some basic properties)? E.g. I can see by using snippets in the GUI that the compressed file here has a lot of null and undefined values where the uncompressed file has valid values.

@confluence could you give more details on this issue? Do you mean file info header or data values? I am not seeing any invalid data values for the fits.gz image before compression, and the nan encodings for compressed data also indicate no nan values.

confluence commented 1 year ago

@confluence could you give more details on this issue? Do you mean file info header or data values? I am not seeing any invalid data values for the fits.gz image before compression, and the nan encodings for compressed data also indicate no nan values.

I mean metadata values. I'll check if it's still happening in the updated code, and provide a snippet / script which illustrates the issue.

confluence commented 1 year ago

After the latest changes, the metadata of both images appears to match (but I haven't done an extensive recursive diff); however, now the values I noticed before are missing / invalid in both images, and now the uncompressed image can't be spatially or spectrally matched with itself either.

Steps to reproduce: 1) open the uncompressed image; 2) append the uncompressed image; 3) attempt spatial or spectral matching.

If I roll back to revision 8b0005983cc46ff508a6677cc5ff6bffeb2c6263, the uncompressed image definitely can be matched with itself, but the compressed image can't be matched with itself or with the uncompressed image. Inspecting the frame objects in the console shows that many properties related to the WCS metadata differ between the images and appear to be invalid or incomplete in the compressed image.

Steps to reproduce:

  1. open the uncompressed image
  2. append the compressed image
  3. open the browser console
  4. execute this snippet: 
    console.log(app.frames[0]);
console.log(app.frames[1]);

If you expand both objects and scroll down, you can see that e.g. validWcs is true in the uncompressed image and false in the compressed image; spectralAxis has a different type and is invalid in the compressed image, spectralType and spectralSystem are null, etc..

If you repeat these steps using the latest revision, both images have the null / invalid values.

Is this a bug, or is the metadata in this file actually invalid and now being correctly parsed as such in both cases?

pford commented 1 year ago

@confluence thank you for the explanation. Apparently the AST library used by the frontend does not support the CTYPE "VELO-LSRK" found in the headers of both images. I shortened this value to "VELO" and put whatever is after '-' in a SPECSYS header with a comment that it came from the VELO CTYPE. The fits and fits.gz images may now be matched.

github-actions[bot] commented 1 year ago

Code Coverage

Package Line Rate Health
src.Cache 66% ➖
src.DataStream 52% ➖
src.FileList 67% ➖
src.Frame 50% ➖
src.HttpServer 43% ➖
src.ImageData 28% ❌
src.ImageFitter 89% ✔
src.ImageGenerators 44% ➖
src.ImageStats 76% ✔
src.Logger 44% ➖
src.Main 54% ➖
src.Region 18% ❌
src.Session 29% ❌
src.Table 52% ➖
src.ThreadingManager 87% ✔
src.Timer 85% ✔
src.Util 49% ➖
Summary 38% (6932 / 18298) ❌