Support HEIC for previews #10526
Support HEIC for previews #10526
Conversation
Signed-off-by: Sebastian Steinmetz <me@sebastiansteinmetz.ch>
Signed-off-by: Sebastian Steinmetz <me@sebastiansteinmetz.ch>
|
Testcases are failing in CI environment, as ImageMagick probably doesn't handle HEIC format. If I run the test in my machine I see all green:
If possible, please update ImageMagick (I'm running 7.0.8-9) and compile it with config option |
|
Let me see what I can do here |
|
Ah so updating the containers with a new imagick is not trivial... but top of that let me make some comments. |
| namespace OC\Preview; | ||
|
|
||
| class HEIC extends Bitmap { | ||
| /** |
There was a problem hiding this comment.
Should also override the isAvailable method. That can then check for support in imagemagick. Else this will do 💥 on system that do now have support for it.
tests/lib/Preview/HEICTest.php
Outdated
| class HEICTest extends Provider { | ||
|
|
||
| public function setUp() { | ||
| parent::setUp(); |
There was a problem hiding this comment.
Since we also run CI on older systems. A check here would also be good. Then we can add imagick support in a newer version. And skip it here if the user running it has an unsupported versions.
|
@steiny2k sorry for having this around so long. Thnx for the PR :) |
- implement isAvailable - run tests only if ImageMagick with HEIC support is available in the environment Signed-off-by: Sebastian Steinmetz <me@sebastiansteinmetz.ch>
|
Hi @rullzer, I think I've addressed your comments with the last input. Would you mind having another look? One thing which came to my mind: would you prefer PNG or JPG previews? I think the nature of the majority of .heic files at the moment are photos rather than illustrations. So JPG is the better preview file format don't you think? If you agree so, I'd submit another commit to make JPG files. |
|
@steiny2k sorry for having this around for a while. yes I think JPG makes more sense indeed. |
|
Hi @rullzer thanks, I've been looking into jpg over png, and would submit it tonight. However it would need to change the inheritance from Bitmap to the more generic Provider class, and copy&pasting some portions of the logic in Bitmap over. Or would you rather change the Bitmap class? It would basically mean to change the private functions Thanks for your input. Also found an issue with the isAvailable check. |
|
Oh - just have seen the update now ... Could you submit this as a separate PR then? :) |
|
Thanks for this nice contribution 👍 |
|
If you are looking for other nice first tasks you can have a look at https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 :) We are happy to help if you have questions. |
Adapt owncloud/core#30093 to NextCloud to tackle #7406
Testcase is developed, please review.