Conversation
|
Heya, thanks a lot for the PR. Currently don't have all that much bandwidth, so might be a little bit until I get to review & merge. |
|
No worries at all -- no rush, and thanks! I'm just happy to be able to examine astrophoto images in my favorite viewer, rather than having to use one of the terrible alternatives. ;) To be a more full-featured replacement for said alternatives, tev would also need a astrophoto-centric tone mapping operator and associated controls. However, such a specific/extreme tone mapper probably won't be of interest to most tev users, and thus would be a waste of UI space to render it all of the time. Once I have something working, I'm likely to have questions about how to integrate it such that it doesn't clutter the UI when not needed (e.g., a visibility toggle, defaulting to rendering it if a FITS image is loaded, for example...?) Or it may prove to be too specialized to add to tev proper. In any case, more to come. |
|
I was planning to add soft clipping (log-space sigmoid with configurable min/max brightness) to the HDR popup menu. Probably not the kind of tone mapping you are looking for 😅, but mentioning it just in case. I think other specialized tone maps would also fit into the HDR popup for now… to be consolidated in an actual tone map selector when/if tev gets support for a bunch of other standard graphics ones (that’d share aforementioned min/max brightness controls and also consume image metadata like max FALL etc.) |
| namespace tev { | ||
|
|
||
| Task<vector<ImageData>> FitsImageLoader::load(istream& iStream, const fs::path&, string_view, const ImageLoaderSettings&, int priority) const { | ||
| char magic[9]; |
There was a problem hiding this comment.
Should get zero-initialized via = {}; to avoid UB.
Following two lines should use sizeof(magic) instead of repeating the hardcoded size.
| void* pixels = nullptr; | ||
| int err = tinyfits_load_from_memory(&fits, data.data(), (size_t)dataSize, &pixels); | ||
| if (err != TINYFITS_OK) { | ||
| tinyfits_free(&fits); |
There was a problem hiding this comment.
All the explicit tinyfits_free and tinyfits_free_buffer calls should be replaced with a ScopeGuard. (See e.g. STBI image loader for a example usage.)
| // Convert native pixel data to normalized float32 for display. | ||
| // tinyfits_to_float normalizes unsigned integer types to [0,1]; floats pass through as-is. | ||
| const auto totalSamples = numPixels * numChannels; | ||
| vector<float> floatPixels(totalSamples); |
There was a problem hiding this comment.
Better to create the channels upfront and then populate them directly via a switch over channel format w/
co_await toFloat32((pixel_t*)pixels + numPixels * c, 1, channels[c].view(), false, priority);Avoids the memcpy, extra allocation, and uses tev's thread pool to parallelize.
| memcpy(view.data(), floatPixels.data() + c * numPixels, numPixels * sizeof(float)); | ||
| } | ||
|
|
||
| resultData.hasPremultipliedAlpha = false; |
There was a problem hiding this comment.
From the channel naming scheme above, it seems to me like FITS images can't contain an alpha channel at all? In that case, it'd be better to specify resultData.hasPremultipliedAlpha = true to avoid some tev-internal conversion checks.
| AttributeNode node; | ||
| node.name = fits.keywords[i].key; | ||
| node.value = fits.keywords[i].value; | ||
| section.children.push_back(std::move(node)); |
There was a problem hiding this comment.
Rather than std::move, prefer direct emplace AttributeNode& node = section.children.emplace_back();
Similarly for the root node.
2 participants
No description provided.