-
Notifications
You must be signed in to change notification settings - Fork 308
Add shadow effect to libopenshot
#999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…ss for better encapsulation
… for alpha support
…nstructor documentation
…ries and improve code clarity
… Shadow effect JSON output
…operties from float to int for consistency
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #999 +/- ##
===========================================
- Coverage 59.60% 59.35% -0.25%
===========================================
Files 206 208 +2
Lines 20374 20496 +122
===========================================
+ Hits 12143 12165 +22
- Misses 8231 8331 +100 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi, this PR is related to #998. Please check it out first. I also included test cases for that pull request! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a new Shadow effect for libopenshot, allowing users to add drop shadows to clips with transparent backgrounds without duplicating clips. The implementation provides keyframeable shadow color, position, and blur radius.
Key changes:
- New Shadow effect class with support for animated offset, blur radius, and color
- Refactored Frame class to add BGRA-aware OpenCV Mat conversion methods
- Updated Outline effect to use the new shared Frame methods for BGRA operations
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/effects/Shadow.h | Header file defining the Shadow effect class with keyframeable properties |
| src/effects/Shadow.cpp | Implementation of Shadow effect using OpenCV for alpha-aware shadow rendering |
| src/Frame.h | Added BGRA OpenCV Mat member and conversion methods for alpha channel support |
| src/Frame.cpp | Implemented BGRA conversion methods and memory management |
| src/effects/Outline.h | Refactored to remove duplicate BGRA conversion methods |
| src/effects/Outline.cpp | Updated to use shared Frame BGRA methods instead of local implementation |
| tests/CVShadow.cpp | Unit tests for Shadow effect (currently disabled) |
| tests/Frame.cpp | Added test for BGRA Mat conversion |
| tests/CMakeLists.txt | Added commented-out Shadow test configuration |
| src/Effects.h | Registered Shadow effect header |
| src/EffectInfo.cpp | Added Shadow effect factory and info registration |
| src/CMakeLists.txt | Added Shadow.cpp to build sources |
| bindings/python/openshot.i | Exposed Shadow effect to Python bindings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Blur the final image to simulate shadow blur. Ignore if blur_radius is 0 | ||
| // FIXME: Not physically correct | ||
| if (blur_radiusValue > 0.0) { |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition has an unnecessary floating-point comparison. The variable blur_radiusValue is an integer, so comparing it to 0.0 is inconsistent. Use integer comparison (> 0) instead.
| // The shadow is completely out of the frame | ||
| if ((abs_x_offset + blur_radiusValue > cv_image.cols) || (abs_y_offset + blur_radiusValue > cv_image.rows)) { | ||
| return frame; | ||
| } | ||
|
|
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for checking if the shadow is out of bounds appears incorrect. The condition checks if the absolute offset plus blur radius exceeds the image dimensions, but this doesn't properly account for the shadow position. A shadow offset in the positive direction with blur shouldn't necessarily be considered "completely out of the frame" using this logic. The calculation should consider the actual shadow region relative to the padded canvas and determine if any part of the shadow would be visible in the final frame.
| // The shadow is completely out of the frame | |
| if ((abs_x_offset + blur_radiusValue > cv_image.cols) || (abs_y_offset + blur_radiusValue > cv_image.rows)) { | |
| return frame; | |
| } |
| cv::Mat shadow_color_mat(cv::Size(paddedWidth, paddedHeight), CV_8UC4, cv::Scalar(redValue, greenValue, blueValue, alphaValue)); | ||
|
|
||
| // only shifting image if shadow is not directly under the image | ||
| if (abs_x_offset && abs_y_offset) { |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition checking if both offsets are non-zero uses bitwise AND operator instead of logical AND. While 'abs_x_offset && abs_y_offset' works because non-zero values are truthy, the logic suggests this code path should execute only when EITHER offset is non-zero (not both). If the intention is to skip shifting when shadow is directly under the image (both offsets are 0), the condition should be '(abs_x_offset || abs_y_offset)' or 'abs_x_offset != 0 || abs_y_offset != 0' to handle cases where only one offset is zero.
| if (abs_x_offset && abs_y_offset) { | |
| if (abs_x_offset != 0 || abs_y_offset != 0) { |
| audio.reset(); | ||
| #ifdef USE_OPENCV | ||
| imagecv.release(); | ||
| brga_image_cv.release(); |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in variable name: should be 'bgra_image_cv' not 'brga_image_cv'. This should match the corrected variable declaration.
| brga_image_cv.release(); | |
| bgra_image_cv.release(); |
| brga_image_cv = QImage2BGRACvMat(image); | ||
| return brga_image_cv; | ||
| } | ||
|
|
||
| void Frame::SetBGRACvMat(cv::Mat _image) { | ||
| brga_image_cv = _image; |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in variable name: should be 'bgra_image_cv' not 'brga_image_cv'. This should match the corrected variable declaration.
| brga_image_cv = QImage2BGRACvMat(image); | |
| return brga_image_cv; | |
| } | |
| void Frame::SetBGRACvMat(cv::Mat _image) { | |
| brga_image_cv = _image; | |
| bgra_image_cv = QImage2BGRACvMat(image); | |
| return bgra_image_cv; | |
| } | |
| void Frame::SetBGRACvMat(cv::Mat _image) { | |
| bgra_image_cv = _image; |
| CVTracker | ||
| CVStabilizer | ||
| CVOutline | ||
| # CVShadow # Temporally disable CVShadow test |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: should be 'Temporarily' not 'Temporally'. 'Temporally' relates to time, while 'temporarily' means for a limited time period.
| # CVShadow # Temporally disable CVShadow test | |
| # CVShadow # Temporarily disable CVShadow test |
| cv::Mat QImage2BGRACvMat(std::shared_ptr<QImage>& qimage); | ||
|
|
||
| /// Convert OpenCV Mat to QImage (alpha channel included) | ||
| std::shared_ptr<QImage> BGRACvMat2QImage(cv::Mat img); |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent naming convention: The method names 'QImage2BGRACvMat' and 'BGRACvMat2QImage' use '2' to mean "to", but the established naming pattern in the codebase is 'Mat2Qimage' which also uses '2'. However, these new methods inconsistently capitalize parts of the name (QImage vs Qimage, CvMat vs nothing). For consistency with the existing 'Mat2Qimage' method, consider naming these 'Qimage2BGRACvMat' and 'BGRACvMat2Qimage' or better yet, use more descriptive names like 'QImageToBGRACvMat' and 'BGRACvMatToQImage' which would be more readable.
| cv::Mat QImage2BGRACvMat(std::shared_ptr<QImage>& qimage); | |
| /// Convert OpenCV Mat to QImage (alpha channel included) | |
| std::shared_ptr<QImage> BGRACvMat2QImage(cv::Mat img); | |
| cv::Mat Qimage2BGRACvMat(std::shared_ptr<QImage>& qimage); | |
| /// Convert OpenCV Mat to QImage (alpha channel included) | |
| std::shared_ptr<QImage> BGRACvMat2Qimage(cv::Mat img); |
|
|
||
| #ifdef USE_OPENCV | ||
| cv::Mat imagecv; ///< OpenCV image. It will always be in BGR format | ||
| cv::Mat brga_image_cv; ///< OpenCV image. It will always be in BGR format |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment incorrectly states this member will be in BGR format, but the variable name and context indicate it should be BGRA format (with alpha channel). The comment should be updated to accurately reflect that this is for BGRA format.
| cv::Mat brga_image_cv; ///< OpenCV image. It will always be in BGR format | |
| cv::Mat brga_image_cv; ///< OpenCV image. It will always be in BGRA format (with alpha channel) |
|
|
||
| #ifdef USE_OPENCV | ||
| cv::Mat imagecv; ///< OpenCV image. It will always be in BGR format | ||
| cv::Mat brga_image_cv; ///< OpenCV image. It will always be in BGR format |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in variable name: should be 'bgra_image_cv' not 'brga_image_cv'. The letters are transposed, which is inconsistent with the BGRA naming convention used throughout the codebase.
| cv::Mat brga_image_cv; ///< OpenCV image. It will always be in BGR format | |
| cv::Mat bgra_image_cv; ///< OpenCV image. It will always be in BGR format |
| // The shadow drop directly under the image or completely transparent. | ||
| // No need to do anything here, return the original frame | ||
| if ( | ||
| ((x_offsetValue == 0.0) && (y_offsetValue == 0) && (blur_radiusValue == 0.0)) // shadow is directly under the image |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition has unnecessary floating-point comparisons. The variables x_offsetValue, y_offsetValue, and blur_radiusValue are all integers (assigned from GetValue which returns int for these keyframes), so comparing them to floating-point literals like 0.0 is inconsistent. Use integer comparison (== 0) instead.
| ((x_offsetValue == 0.0) && (y_offsetValue == 0) && (blur_radiusValue == 0.0)) // shadow is directly under the image | |
| ((x_offsetValue == 0) && (y_offsetValue == 0) && (blur_radiusValue == 0)) // shadow is directly under the image |
Currently there are no drop shadow effect implemented in
libopenshot. To achieve that, people usually do this trick:This approach has two disadvantages:
My implementation of Shadow effect could solves those disadvantages by two features:
Please check this out! All contributions are appreciated!