Cleaning, adding documentation, removing warnings and remove bugs#97
Cleaning, adding documentation, removing warnings and remove bugs#97servantftransperfect wants to merge 1 commit intodevelopfrom
Conversation
servantftransperfect
force-pushed
the
dev/cleanup
branch
2 times, most recently
from
abfd44e to
5421d2b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 31 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // commitTextureOperations() runs on the render thread after this function returns | ||
| // and may downscale the image to fit GPU limits, changing the texture size. | ||
| // Post the updated size back to the GUI thread so the signal fires correctly. | ||
| texture->setOnCommit([this](QSize committedSize) { | ||
| QMetaObject::invokeMethod(this, [this, committedSize]() { | ||
| if (_textureSize != committedSize) | ||
| { | ||
| _textureSize = committedSize; | ||
| _geometryChanged = true; | ||
| Q_EMIT textureSizeChanged(); | ||
| } | ||
| }, Qt::QueuedConnection); | ||
| }); |
There was a problem hiding this comment.
The render-thread callback captures a raw this and can outlive the item (e.g., during scene graph teardown), which risks a use-after-free before invokeMethod is even called. Use a QPointer<FloatImageViewer> (or similar guarded handle) inside the callback and return early if it is null, or bind the callback lifetime to a QObject context that is guaranteed to outlive render-thread commits.
| // Work on a local copy so the caller's shared image is never mutated. | ||
| FloatImage scaledImage = *_srcImage; | ||
| while (_maxTextureSize != -1 && (scaledImage.width() > _maxTextureSize || scaledImage.height() > _maxTextureSize)) | ||
| { | ||
| FloatImage tmp; | ||
| aliceVision::image::imageHalfSample(*_srcImage, tmp); | ||
| *_srcImage = std::move(tmp); | ||
| aliceVision::image::imageHalfSample(scaledImage, tmp); | ||
| scaledImage = std::move(tmp); | ||
| } | ||
| _textureSize = {_srcImage->width(), _srcImage->height()}; | ||
| _textureSize = {scaledImage.width(), scaledImage.height()}; | ||
|
|
||
| // Destroy the previous RHI texture before allocating a new one. | ||
| _rhiTexture.reset(); | ||
|
|
||
| const QRhiTexture::Flags texFlags(hasMipmaps() ? QRhiTexture::MipMapped : 0); | ||
| _rhiTexture = rhi->newTexture(texFormat, _textureSize, 1, texFlags); | ||
| _rhiTexture.reset(rhi->newTexture(texFormat, _textureSize, 1, texFlags)); |
There was a problem hiding this comment.
This always deep-copies the full source image and always destroys/recreates the GPU texture on each dirty commit. For large float images or during playback, this can be a significant CPU + GPU allocation cost. Consider (1) only copying/downscaling when the image exceeds _maxTextureSize (otherwise upload from _srcImage directly), and (2) reusing _rhiTexture when the size/format/flags are unchanged (only reallocate on size/flag changes).
| int FloatImageViewerMaterial::compare(const QSGMaterial* other) const | ||
| { | ||
| Q_ASSERT(other && type() == other->type()); | ||
| return other == this ? 0 : (other > this ? 1 : -1); | ||
| } |
There was a problem hiding this comment.
Relational pointer comparison (other > this) on unrelated objects is not guaranteed to be well-defined in standard C++. For a stable strict-weak ordering, use std::less<const QSGMaterial*>{}(this, other) (or equivalent) to avoid undefined/unspecified behavior across platforms/compilers.
servantftransperfect
force-pushed
the
dev/cleanup
branch
from
5421d2b to
3186763
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 31 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| texture->setHorizontalWrapMode(QSGTexture::Repeat); | ||
| texture->setVerticalWrapMode(QSGTexture::Repeat); | ||
| newTextureSize = texture->textureSize(); | ||
| // commitTextureOperations() runs on the render thread after this function returns | ||
| // and may downscale the image to fit GPU limits, changing the texture size. |
There was a problem hiding this comment.
When clearBeforeLoad clears _image before an async request completes, updatePaintNode() can enter the _imageChanged path with _image == nullptr, which installs a FloatTexture with no image and later drives _textureSize to (0,0). Downstream aspect-ratio fitting then risks division-by-zero/NaNs. Consider keeping the existing/placeholder texture while loading (or setting an explicit 1x1 placeholder) and guarding geometry calculations until a valid texture size is available.
| // Work on a local copy so the caller's shared image is never mutated. | ||
| FloatImage scaledImage = *_srcImage; | ||
| while (_maxTextureSize != -1 && (scaledImage.width() > _maxTextureSize || scaledImage.height() > _maxTextureSize)) |
There was a problem hiding this comment.
commitTextureOperations() always copies the full source image into scaledImage even when no downscaling is required. For large float images this can double memory usage and add a full-frame memcpy on every upload. Consider only creating a copy when the image actually exceeds _maxTextureSize (e.g., work with a const FloatImage* initially, and allocate/copy only inside the downscale loop).
| _sequence = paths; | ||
| _currentIndex = 0; | ||
|
|
||
| for (unsigned idx = 0; idx < _sequence.size(); idx++) | ||
| for (std::size_t idx = 0; idx < _sequence.size(); ++idx) | ||
| { | ||
| _pathToSeqId[_sequence[idx]] = idx; | ||
| _pathToSeqId[_sequence[idx]] = static_cast<unsigned>(idx); | ||
| } |
There was a problem hiding this comment.
setSequence() rebuilds _pathToSeqId without clearing it first, so paths from a previous sequence can remain mapped and incorrectly update _currentIndex in getFrame(). Clear _pathToSeqId (and ideally reserve) before repopulating it when a new sequence is set.
|
|
||
| /** | ||
| * @brief Sets the exposure gain factor uploaded to the shader. | ||
| * @param gain Gain multiplier (0.0 = no change in the shader's convention). |
There was a problem hiding this comment.
The setGain() doc says “0.0 = no change”, but the shader multiplies color by gain, so the neutral value is 1.0 (0.0 would turn the image black). Please correct the documentation to avoid misleading callers.
| * @param gain Gain multiplier (0.0 = no change in the shader's convention). | |
| * @param gain Gain multiplier (1.0 = no change). |
servantftransperfect
force-pushed
the
dev/cleanup
branch
from
3186763 to
f21b7f8
Compare
|
This pull request introduces several improvements and refactorings to the
qtAliceVisionplugin, with a focus on code modernization, thread safety, and improved modularity for the float image viewer. The most significant changes include extracting custom material and node classes for the float image viewer into separate files, enhancing thread safety and type correctness inAsyncFetcher, and updating the build system to reflect new source/header files and shader paths.Refactoring and Modularity
FloatImageViewerMaterial,FloatImageViewerMaterialShader, andFloatImageViewerNodeclasses fromFloatImageViewer.cppinto their own dedicated files, cleaning up the main file and improving maintainability. ([[1]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-d79c8b4c6c85b777b154b255c9435ca4877d466c2434dca54d984280e7d839d6L4-L7),[[2]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-d79c8b4c6c85b777b154b255c9435ca4877d466c2434dca54d984280e7d839d6L20-L285),Fe933d4cL3R3,[[3]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e58b57ebfda32ca9d6336be78c2f7e63b6100d4980912b051c5d478ac30bc9deR32-R34))Build System Updates
CMakeLists.txtto add new source and header files for the extracted float image viewer components and to adjust shader file paths, ensuring shaders are referenced from the correct directory and not using a prefix. (Fe933d4cL3R3,[[1]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e58b57ebfda32ca9d6336be78c2f7e63b6100d4980912b051c5d478ac30bc9deR32-R34),[[2]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e58b57ebfda32ca9d6336be78c2f7e63b6100d4980912b051c5d478ac30bc9deL79-R93))Thread Safety and Type Correctness in AsyncFetcher
AsyncFetchermethods thread-safe by marking mutex and methods asmutable/constwhere appropriate, and ensured correct locking when accessing shared resources such as_resizeRatio. ([[1]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-82ef2056ce6c71b07dcd940371c1b579ed183c2a7d8e16d5a50362dbe08edd29L134-R134),[[2]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-82ef2056ce6c71b07dcd940371c1b579ed183c2a7d8e16d5a50362dbe08edd29L55-R55),[[3]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-82ef2056ce6c71b07dcd940371c1b579ed183c2a7d8e16d5a50362dbe08edd29L108-R111),[[4]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e2fcc700adda7349eb7db3477f79101d7c78ecb5151c409799f43d4f72a91dccR183-R195),[[5]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e2fcc700adda7349eb7db3477f79101d7c78ecb5151c409799f43d4f72a91dccL224-R230),[[6]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e2fcc700adda7349eb7db3477f79101d7c78ecb5151c409799f43d4f72a91dccL241-R266))unsigned/int/doubletype usages with more precise and safe types (e.g.,std::size_t,float) and improved type casting for clarity and correctness. ([[1]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-c57d2d3ceda7d72504678aa877062f5c3cead58e2e88886b3aaa142d5494773fL61-R77),[[2]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e2fcc700adda7349eb7db3477f79101d7c78ecb5151c409799f43d4f72a91dccL35-R45),[[3]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e2fcc700adda7349eb7db3477f79101d7c78ecb5151c409799f43d4f72a91dccL112-R115))Code Quality and Minor Improvements
AsyncFetcherby using.empty()instead of.size() == 0, cleaning up unused includes, and updating comments for clarity. ([[1]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e2fcc700adda7349eb7db3477f79101d7c78ecb5151c409799f43d4f72a91dccR6-L10),[[2]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e2fcc700adda7349eb7db3477f79101d7c78ecb5151c409799f43d4f72a91dccL96-R99),[[3]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e2fcc700adda7349eb7db3477f79101d7c78ecb5151c409799f43d4f72a91dccL274-R281),[[4]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e2fcc700adda7349eb7db3477f79101d7c78ecb5151c409799f43d4f72a91dccL283-L284))setResizeRatioif the ratio is non-positive, preventing invalid state. ([src/qtAliceVision/AsyncFetcher.cppL35-R45](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e2fcc700adda7349eb7db3477f79101d7c78ecb5151c409799f43d4f72a91dccL35-R45))API Consistency
getPrefetching()andgetCacheMemory()constfor better API consistency and thread safety. ([[1]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-82ef2056ce6c71b07dcd940371c1b579ed183c2a7d8e16d5a50362dbe08edd29L55-R55),[[2]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-82ef2056ce6c71b07dcd940371c1b579ed183c2a7d8e16d5a50362dbe08edd29L108-R111),[[3]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e2fcc700adda7349eb7db3477f79101d7c78ecb5151c409799f43d4f72a91dccL58-R61),[[4]](https://github.com/alicevision/QtAliceVision/pull/97/files#diff-e2fcc700adda7349eb7db3477f79101d7c78ecb5151c409799f43d4f72a91dccL152-R154))These changes collectively improve the maintainability, safety, and clarity of the codebase, especially around the float image viewer and asynchronous image fetching components.