Skip to content

While processing in CLI, do not crash on large photos #3533

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 5, 2025
Merged

While processing in CLI, do not crash on large photos #3533

merged 6 commits into from
Aug 5, 2025

Conversation

ildyria
Copy link
Member

@ildyria ildyria commented Jul 13, 2025

Avoid the following (which could be resolved by checking if the image size is small enough for Imagick.
See more here:
https://stackoverflow.com/questions/77578848/imagemagick-throws-width-or-height-exceeds-when-converting-svg-to-png-with-ver

Let's just not fail for now. We later can consider a fix or a warning in the settings.

▶ docker exec -it 394cec php artisan lychee:extract_colour_palette       
Extracting Color Palette for IMG_1363-Pano [0VmiYZJs-xGaFypAGiddDcOe].

In ExtractColourPalette.php line 68:
                                                     
  Unknown Lychee exception (this is probably a bug)  
                                                     

In ImageFactoryForColourExtraction.php line 119:
                        
  Failed to load image  
                        

In ImageFactoryForColourExtraction.php line 107:
                                                                                                                    
  width or height exceeds limit `/tmp/magick-xfA9Pq9r7doI2aNcxSwY9sYPDRh85scS' @ error/cache.c/OpenPixelCache/3909

@ildyria ildyria requested a review from a team as a code owner July 13, 2025 14:12
@ildyria ildyria added Review: easy Easy review expected: probably just need a quick to go through. alpha-ready Branch is available in alpha labels Jul 13, 2025
@codecov Codecov
Copy link

codecov bot commented Jul 13, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.74%. Comparing base (93a2101) to head (87f760f).
⚠️ Report is 6 commits behind head on master.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ildyria ildyria force-pushed the cli-color-extraction branch from 6c53d58 to 87f760f Compare July 29, 2025 19:17
JasonMillward
JasonMillward reviewed Jul 31, 2025
View reviewed changes
app/Console/Commands/ImageProcessing/ExtractColourPalette.php
ExtractColoursJob::dispatchSync($photo);
try {
$this->line(sprintf('Extracting Color Palette for %s [%s].', $photo->title, $photo->id));
ExtractColoursJob::dispatchSync($photo);
Copy link
Contributor

@JasonMillward JasonMillward Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to check Configs::getValueAsBool('use_job_queues') here to dispatch jobs to queues?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am considering dropping the use_job_queues config actually. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and have it determined directly by the .env config

@ildyria ildyria merged commit f8838d7 into master Aug 5, 2025
35 checks passed
@ildyria ildyria deleted the cli-color-extraction branch August 5, 2025 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JasonMillward JasonMillward JasonMillward approved these changes

Assignees
No one assigned
Labels
alpha-ready Branch is available in alpha Review: easy Easy review expected: probably just need a quick to go through.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ildyria @JasonMillward