Skip to content

Conversation

@szaimen
Copy link
Contributor

@szaimen szaimen commented Dec 21, 2022

Close #31284

@szaimen szaimen added this to the Nextcloud 26 milestone Dec 21, 2022
@szaimen szaimen force-pushed the enh/31284/preview-max-memory branch 3 times, most recently from b2ac011 to 3ca9409 Compare December 21, 2022 16:35
@szaimen szaimen force-pushed the enh/31284/preview-max-memory branch from 3ca9409 to ed83721 Compare December 21, 2022 16:39
@szaimen szaimen force-pushed the enh/31284/preview-max-memory branch from ed83721 to c5212ed Compare December 21, 2022 17:13
->method('getSystemValueInt')
->with('preview_max_memory', 128)
->willReturn(128);
->with('preview_max_memory', 384)
Copy link
Contributor

@kesselb kesselb Dec 21, 2022

Choose a reason for hiding this comment

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

The value depends on your machine. I fear we are going to introduce the next "wonky" test ;)
Could you try to set the memory limit for this test to a fixed value?

ini_set('memory_limit', '256M');

Best would be to mock the MemoryInfo class but that's a bit tricky here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you try to set the memory limit for this test to a fixed value?

ini_set('memory_limit', '256M');

Good idea, thanks! :)

@kesselb
Copy link
Contributor

kesselb commented Dec 21, 2022

$this->logger->debug('Image size of ' . $width . 'x' . $height . ' would exceed allowed memory limit of ' . $memory_limit);

A higher log level could also help.

Auto-calculating preview_max_memory is a bit too much magic for me.
I prefer to improve our documentation instead of yet another runtime check for a static value.

@szaimen
Copy link
Contributor Author

szaimen commented Dec 21, 2022

Auto-calculating preview_max_memory is a bit too much magic for me.

All right. Then lets increase the default to 256 and also increase the log entry to info which should fix the issue as well imho.

@kesselb
Copy link
Contributor

kesselb commented Dec 21, 2022

All right. Then lets increase the default to 256 and also increase the log entry to info which should fix the issue as well

Your choice ;) I just shared my concerns.

The issue with info is that most people use warning as default log level.

@szaimen
Copy link
Contributor Author

szaimen commented Dec 21, 2022

Superseded by #35856

@szaimen szaimen closed this Dec 21, 2022
@szaimen szaimen deleted the enh/31284/preview-max-memory branch December 21, 2022 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Nextcloud cannot create previews for jpg images 34 MP or larger

3 participants