Using webp as storage format for images

[tclaus]

Supporting heic images to be uploaded.
Posts now support the upload of png, jpg, heic and webp image formats.
Files are converted to the web optimized webp format after upload

Issue: #8355

[SuperTux88]

The code looks fine so far (except a small note), but as discussed in #8355 (comment), at least Ubuntu (18.04 and 20.04) doesn't support HEIC/HEIF yet, and 18.04 also doesn't support WEBP yet, but maybe we can ignore that one (but still has support for almost a year)? On the debian side we should be fine (debian 9/stretch which also doesn't support it yet is EOL in two days), and I didn't check the redhat based distros (I don't know these very well).

But I think Ubuntu 20.04 is the deal-breaker here, because that's still used on many servers and I don't know if we want to say we don't support that anymore. This is also the reason why the tests fail by the way.

So for me there are multiple possibilities now:

• We wait until we feel it's OK to cut off Ubuntu 20.04 (and everything else that might not support it yet)
• We find another solution how we can still support Ubuntu 20.04 (and the current solution of "just compile it yourself" isn't something I want to tell all the podmins still using 20.04).
• We add detection of what is supported and only allow HEIC/HEIF if it's supported and only use WEBP if it's supported (but I'm probably fine to drop 18.04 if this gets only merged to 0.8.x, so we maybe don't need to add detection for WEBP, but we maybe need to check if there are other distros which don't support it yet)

There is also something else to consider, because WEBP also isn't supported in all the browsers yet, because Safari needs a minimal macOS version to support WEBP (macOS 11 Big Sur or later). I'm not in the mac ecosystem enough to know how if most mac users have Big Sur or later already and it's fine, or if there are still many users with older macOS versions. And caniuse.com only shows how many % are using which Safari version, but they don't show percentages for macOS versions. On all my websites where I already use WEBP I still provide jpg/png fallbacks for the poor mac users, but that would require twice as much space on the pod and also would need support by all pods first (to show both versions), so it's not a solution here. So we need somebody familiar with the mac community to know if we can expect most mac users to use the latest macOS version or not.

[tclaus]

Its a pitty with safaris support of webp. I admit I was not aware, that Safari (14) supports webp only since Big Sur.

On the other hand.. Big Sur supports Hardware back since 2013 (see https://support.apple.com/en-us/HT211238 ), so this should not really bother a majority of users.

On the server side, my pod Societas.online runs Ubuntu 20.02 and images just converts file to webp. I do not remember that I ever installed any special supporting libs. It even converts heic to webp, but "convert --version" do not show heic as a supported file format. Maybe the formats listed there are meant for writing?

What do you think about adding a admin configuration setting for this feature? On the other hand, when the next diaspora major is released, it might be OK to cut of old habits?

[SuperTux88]

On the other hand.. Big Sur supports Hardware back since 2013 (see https://support.apple.com/en-us/HT211238 ), so this should not really bother a majority of users.

Yeah, I just want to know if we can expect most users to have already updated to Big Sur or later, or if many users still have an old version "because it still works and why should I update". Are older versions still supported by apple and get security updates, or would they need to update anyway? I just don't want a lot of people complaining that their images are broken (there are probably still some people using old browsers, but I don't care about the last person trying to use internet explorer, but I don't know if I should care about people using old macOS versions or if it's fine to ignore them).

On the server side, my pod Societas.online runs Ubuntu 20.02 and images just converts file to webp.

As said, 20.04 supports WEBP, so that's expected.

It even converts heic to webp, but "convert --version" do not show heic as a supported file format.

That's more interesting, because the version in 20.04 doesn't list HEIC/HEIF as supported format, but the changelog says it's supported. Did you manually install libheif1 (because it isn't automatically installed as dependency on 20.04) and that was enough to make it work, even if it's not listed as supported? If that's the case, maybe installing installing libheif1 is enough to also make it work tests? If that works for the tests, then I'm fine with it, because asking podmins to install an additional dependency is fine, but I don't want to ask them to re-compile a package themselves. Do you want to try that (and maybe also rebase on develop again to reduce the log-spam in tests because of the deprecation warning)?

What do you think about adding a admin configuration setting for this feature? On the other hand, when the next diaspora major is released, it might be OK to cut of old habits?

If 20.04 works with manually installing libheif1 and the only thing we cut of is 18.04, then I think I'm fine with that (since it takes some time until 0.8 gets released anyway). But somebody should probably also check the situation on redhat based distros, if there are also still supported distros/versions that don't support this feature yet. I'm not sure if I would add a setting, because there is no reason to disable it if it's supported, and podmins will forget to enable it once they update (or podmins with old distros forget to disable it), so I would like to automatically enable it if it's supported instead of a manual setting. On the other hand, if on 20.04 it's supported but not listed as supported format, that makes auto-detection a bit more complecated. 🤔

[tclaus]

@SuperTux88
The CI doesn't like to convert heic files.
However - I can remove the heic file support for now and let this PR only convert incoming files to webp. What do you think?

[Flaburgan]

Yes, I think we should only support WebP at the moment, that's a nice way to improve disk usage for podmin. I don't think phones are taking pictures in HEIF, are they? If not and only cameras are, then users need to first go through a computer anyway so they can convert them before uploading.

[tclaus]

Yes, I think we should only support WebP at the moment, that's a nice way to improve disk usage for podmin. I don't think phones are taking pictures in HEIF, are they? If not and only cameras are, then users need to first go through a computer anyway so they can convert them before uploading.

Yes, I think we should only support WebP at the moment, that's a nice way to improve disk usage for podmin. I don't think phones are taking pictures in HEIF, are they? If not and only cameras are, then users need to first go through a computer anyway so they can convert them before uploading.

The iPhone uses heif/heic images internally. If the user sends auch a photo (which's type the user don't know) to a Mac, the image will be send as a heic typed image. Only then the user makes contact tu auch a type.
Direct uploads from a mobile will cause the devise to transparently convert the image on the fly to jpeg format.
So not enabling heic format for uploads is not a big deal.

[SuperTux88]

Thanks for updating the PR, and I'm fine with only merging the webp part of it.

There is one last problem I found: this now breaks the image import. When importing a image archive, it stores the photos again, and before this PR it is used to generate all the thumbnails again (the archive only contains the original size). But with this PR the import would also convert everything to webp, which is a problem, because other pods expect the image to still keep the original filename and not suddenly change to a different extension/format. So all other pods would show broken images because they still link to the png/jpg variant of an image. So for the import to work we need a way to temporarily disable the conversion to webp from different formats for specific images. So png/jpg in the archive stay png/jpg, if the archive contains webp in the future they will be imported as webp (because new uploads will also be exported as webp in the archive).

[tclaus]

@SuperTux88
So, during import- could a flag be set on current_user to mark the user that we are currently in the middle of an import? The photo model then could not convert the image type. After import the flag is then reset to a default value.
is this a way that should work?

[Flaburgan]

@tclaus I think the import could contain both pngs and webp, so we have to be careful about that too.

[SuperTux88]

So, during import- could a flag be set on current_user to mark the user that we are currently in the middle of an import? The photo model then could not convert the image type. After import the flag is then reset to a default value.
is this a way that should work?

The problem with this is, that we don't really know when we are done with importing the photos, since the resizing of the images happens async (also for the import), so the import is finished before the all photos are resized. I was initially thinking of adding a flag to each photo we import, but then I realized we would need to add a column for that to the photos table so when reading the entity from the database again for the resize, it's still set correctly and I don't like to add a column only for the import (adding a flag to the user would have the same problem that it needs an additional column).

My new idea is now to add an optional parameter to the ProcessPhoto worker and setting that when queuing jobs during the import, but not when uploading images. The job can then set the flag on the photo entity dynamically after reading it from the DB but before triggering the resize. So we don't need to persist anything about that to the database and also since it's set per photo we don't need to reset anything after the import and also uploading photos during the import will work just as usual. What do you think about that approach?

I think the import could contain both pngs and webp, so we have to be careful about that too.

As already written in my review, that's not a problem, during the import pngs stay pngs, and webp stay webp ... we only need to disable app/uploaders/unprocessed_image.rb:48 which converts non webp to webp during upload, which it shouldn't do during import. We don't disable webp support during import.

(edit: we also need to disable line 29 ... so basically needs_converting? needs to return false for imported photos)

[tclaus]

Hm - in photos.rb there will be a process started on after_commit.
Without setting a kind of "status" inside this photo or removing this after_commit I have no idea how to distinguish between imported photo and a regular one.
I tend to add another worker, that won't convert a photo, and remove this after_commit. Is this an option that works?

diaspora/app/models/photo.rb

Line 51 in 5764b76

after_commit on: :create do

[SuperTux88]

@tclaus I just noticed that the ProcessPhoto isn't even the issue, the image already gets already converted to webp in UnprocessedImage, while ProcessedImage only creates the different sizes then. So you need to already disabling converting to wepb before setting the unprocessed_image. So remove 5764b76 again, and then this should do the trick:

diff --git a/app/models/photo.rb b/app/models/photo.rb
index 0a77df62a..c776fe184 100644
--- a/app/models/photo.rb
+++ b/app/models/photo.rb
@@ -36,6 +36,8 @@ class Photo < ApplicationRecord
     }, as: :status_message
   end
 
+  attr_accessor :keep_original_format
+
   mount_uploader :processed_image, ProcessedImage
   mount_uploader :unprocessed_image, UnprocessedImage
 
diff --git a/app/services/import_service.rb b/app/services/import_service.rb
index cb6d02e3b..89e107452 100644
--- a/app/services/import_service.rb
+++ b/app/services/import_service.rb
@@ -74,6 +74,7 @@ class ImportService
   def store_and_process_photo(photo, uploaded_file, random_string)
     File.open(uploaded_file) do |file|
       photo.random_string = random_string
+      photo.keep_original_format = true
       photo.unprocessed_image.store! file
       photo.update_remote_path
       photo.save(touch: false)
diff --git a/app/uploaders/unprocessed_image.rb b/app/uploaders/unprocessed_image.rb
index 558202e3e..a653aa34f 100644
--- a/app/uploaders/unprocessed_image.rb
+++ b/app/uploaders/unprocessed_image.rb
@@ -31,7 +31,7 @@ class UnprocessedImage < CarrierWave::Uploader::Base
 
   def needs_converting?
     extname = File.extname(@filename)
-    %w[.webp .gif].exclude?(extname)
+    %w[.webp .gif].exclude?(extname) && !model.keep_original_format
   end
 
   process :basic_process

[tclaus]

Did you notice that the image is not processed directly but in a job queue? So I think by starting the job it should be distinguished between convert format and not. The job just gets the photo ID.

[SuperTux88]

As said, the job only does the resize into different sizes, it doesn't change the format, it keeps the format that's already used in the unprocessed_image. And before the job runs the unprocessed_image already converts the image into webp (if needed), so you need to disable that in the unprocessed_image (which my patch does), not in the processed_image (Which is the one that runs in the job).

[SuperTux88]

this is a leftover from the old commit

[SuperTux88]

this worker isn't needed

[tclaus]

thought this was removed..

[SuperTux88]

Looks good now 🍪