Skip to content
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

[status-mobile-16457] Fix messages containing Giphy URLs not being sent #3821

Closed
wants to merge 40 commits into from

Conversation

ibrkhalil
Copy link
Contributor

When trying to calculate an image for a preview using incorrect field names, It resulted in the message not to be sent

Closes #16457 on status-mobile

@ibrkhalil ibrkhalil self-assigned this Jul 29, 2023
@status-im-auto
Copy link
Member

status-im-auto commented Jul 29, 2023

Jenkins Builds

Click to see older builds (91)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 9ae179f #1 2023-07-29 17:12:46 ~3 min linux 📦zip
✔️ 9ae179f #1 2023-07-29 17:14:16 ~4 min ios 📦zip
✔️ 9ae179f #1 2023-07-29 17:14:20 ~4 min android 📦aar
✖️ 9ae179f #1 2023-07-29 17:15:03 ~5 min tests 📄log
✖️ 9ae179f #2 2023-07-31 10:53:14 ~5 min tests 📄log
✖️ 9ae179f #3 2023-07-31 11:09:55 ~2 min tests 📄log
✔️ 9ae179f #4 2023-07-31 11:40:14 ~23 min tests 📄log
✔️ c679e90 #2 2023-07-31 14:11:01 ~2 min linux 📦zip
✔️ c679e90 #2 2023-07-31 14:13:13 ~4 min android 📦aar
✖️ c679e90 #5 2023-07-31 14:13:47 ~5 min tests 📄log
✔️ c679e90 #2 2023-07-31 14:28:26 ~20 min ios 📦zip
✖️ f04c472 #6 2023-07-31 16:26:08 ~1 min tests 📄log
✔️ f04c472 #3 2023-07-31 16:27:09 ~2 min android 📦aar
✔️ f04c472 #3 2023-07-31 16:27:54 ~2 min linux 📦zip
✔️ f04c472 #3 2023-07-31 16:28:12 ~3 min ios 📦zip
✖️ 70273a7 #7 2023-08-01 06:00:02 ~1 min tests 📄log
✔️ 70273a7 #4 2023-08-01 06:00:17 ~1 min linux 📦zip
✔️ 70273a7 #4 2023-08-01 06:00:49 ~2 min android 📦aar
✔️ 70273a7 #4 2023-08-01 06:02:28 ~3 min ios 📦zip
✖️ 35aa9ca #8 2023-08-01 06:01:52 ~1 min tests 📄log
✔️ 35aa9ca #5 2023-08-01 06:02:13 ~1 min linux 📦zip
✔️ 35aa9ca #5 2023-08-01 06:02:44 ~1 min android 📦aar
✔️ 35aa9ca #5 2023-08-01 06:05:25 ~2 min ios 📦zip
✖️ daccdd5 #9 2023-08-01 06:13:41 ~1 min tests 📄log
✔️ daccdd5 #6 2023-08-01 06:14:20 ~1 min linux 📦zip
✔️ daccdd5 #6 2023-08-01 06:14:31 ~1 min android 📦aar
✔️ daccdd5 #6 2023-08-01 06:15:41 ~3 min ios 📦zip
✖️ 6778e48 #10 2023-08-01 06:17:08 ~49 sec tests 📄log
✔️ 6778e48 #7 2023-08-01 06:17:29 ~1 min linux 📦zip
✔️ 6778e48 #7 2023-08-01 06:17:51 ~1 min android 📦aar
✔️ 6778e48 #7 2023-08-01 06:19:18 ~3 min ios 📦zip
✖️ a5a1345 #11 2023-08-01 06:56:20 ~1 min tests 📄log
✔️ a5a1345 #8 2023-08-01 06:56:46 ~1 min linux 📦zip
✔️ a5a1345 #8 2023-08-01 06:57:12 ~1 min android 📦aar
✔️ a5a1345 #8 2023-08-01 06:58:37 ~3 min ios 📦zip
1d04388 #9 2023-08-01 07:41:24 ~13 sec linux 📄log
1d04388 #9 2023-08-01 07:41:25 ~19 sec android 📄log
1d04388 #9 2023-08-01 07:41:30 ~19 sec ios 📄log
✖️ 1d04388 #12 2023-08-01 07:42:02 ~51 sec tests 📄log
✔️ 3fe1802 #10 2023-08-01 07:51:20 ~1 min linux 📦zip
✔️ 3fe1802 #10 2023-08-01 07:51:37 ~1 min android 📦aar
✔️ 3fe1802 #10 2023-08-01 07:52:45 ~3 min ios 📦zip
✖️ 3fe1802 #13 2023-08-01 07:53:04 ~3 min tests 📄log
✔️ ebe9ee4 #11 2023-08-01 08:31:15 ~3 min ios 📦zip
✔️ ebe9ee4 #11 2023-08-01 08:32:21 ~4 min linux 📦zip
✖️ ebe9ee4 #14 2023-08-01 08:32:41 ~4 min tests 📄log
✔️ ebe9ee4 #11 2023-08-01 08:34:12 ~6 min android 📦aar
✖️ 3501bba #15 2023-08-01 09:20:00 ~1 min tests 📄log
✔️ 3501bba #12 2023-08-01 09:20:23 ~1 min linux 📦zip
✔️ 3501bba #12 2023-08-01 09:20:53 ~2 min android 📦aar
✔️ 3501bba #12 2023-08-01 09:22:03 ~3 min ios 📦zip
✔️ 7e19552 #13 2023-08-01 09:57:16 ~1 min linux 📦zip
✔️ 7e19552 #13 2023-08-01 09:57:55 ~2 min android 📦aar
✖️ 7e19552 #16 2023-08-01 09:58:58 ~3 min tests 📄log
✔️ 7e19552 #13 2023-08-01 09:59:03 ~3 min ios 📦zip
✔️ fab8dea #14 2023-08-01 10:47:22 ~2 min linux 📦zip
✔️ fab8dea #14 2023-08-01 10:48:11 ~3 min ios 📦zip
✔️ fab8dea #14 2023-08-01 10:49:53 ~5 min android 📦aar
✖️ fab8dea #17 2023-08-01 11:07:13 ~22 min tests 📄log
✖️ dd91560 #18 2023-08-02 05:29:33 ~1 min tests 📄log
✔️ dd91560 #15 2023-08-02 05:29:50 ~1 min linux 📦zip
✔️ dd91560 #15 2023-08-02 05:32:00 ~3 min ios 📦zip
✔️ dd91560 #15 2023-08-02 05:33:01 ~4 min android 📦aar
✖️ a84b3fa #19 2023-08-02 05:33:01 ~59 sec tests 📄log
✔️ a84b3fa #16 2023-08-02 05:33:28 ~1 min linux 📦zip
✔️ a84b3fa #16 2023-08-02 05:35:10 ~2 min android 📦aar
✔️ a84b3fa #16 2023-08-02 05:35:16 ~3 min ios 📦zip
✔️ 7e2defa #17 2023-08-02 11:57:17 ~3 min linux 📦zip
✔️ 7e2defa #17 2023-08-02 11:58:55 ~4 min ios 📦zip
✔️ 7e2defa #17 2023-08-02 11:59:09 ~5 min android 📦aar
✖️ 7e2defa #20 2023-08-02 12:16:42 ~22 min tests 📄log
✔️ bda8580 #18 2023-08-03 09:22:27 ~2 min android 📦aar
✔️ bda8580 #18 2023-08-03 09:23:20 ~3 min linux 📦zip
✔️ bda8580 #18 2023-08-03 09:24:55 ~4 min ios 📦zip
✔️ bda8580 #21 2023-08-03 09:45:12 ~24 min tests 📄log
✔️ 9896d31 #19 2023-08-05 18:30:41 ~1 min linux 📦zip
✖️ 9896d31 #22 2023-08-05 18:30:41 ~1 min tests 📄log
✔️ 9896d31 #19 2023-08-05 18:31:06 ~1 min android 📦aar
✔️ 9896d31 #19 2023-08-05 18:32:33 ~3 min ios 📦zip
✔️ 9085414 #20 2023-08-05 18:40:14 ~1 min linux 📦zip
✔️ 9085414 #20 2023-08-05 18:40:20 ~1 min android 📦aar
✔️ 9085414 #20 2023-08-05 18:41:27 ~2 min ios 📦zip
✖️ 9085414 #23 2023-08-05 18:48:24 ~9 min tests 📄log
✔️ 244742e #21 2023-08-05 18:41:32 ~1 min linux 📦zip
✔️ 244742e #21 2023-08-05 18:42:05 ~1 min android 📦aar
✔️ 244742e #21 2023-08-05 18:44:01 ~2 min ios 📦zip
✖️ 244742e #24 2023-08-05 19:07:07 ~18 min tests 📄log
✖️ 56ba2f6 #25 2023-08-07 11:56:52 ~1 min tests 📄log
✔️ 56ba2f6 #22 2023-08-07 11:57:00 ~1 min linux 📦zip
✔️ 56ba2f6 #22 2023-08-07 11:57:20 ~1 min android 📦aar
✔️ 56ba2f6 #22 2023-08-07 11:58:16 ~2 min ios 📦zip
Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ cd03e90 #26 2023-08-07 11:58:17 ~1 min tests 📄log
✔️ cd03e90 #23 2023-08-07 11:58:31 ~1 min linux 📦zip
✔️ cd03e90 #23 2023-08-07 11:58:55 ~1 min android 📦aar
✔️ cd03e90 #23 2023-08-07 12:00:53 ~2 min ios 📦zip
✔️ b8317c6 #24 2023-08-07 12:14:50 ~1 min android 📦aar
✔️ b8317c6 #24 2023-08-07 12:14:54 ~1 min linux 📦zip
✔️ b8317c6 #24 2023-08-07 12:15:46 ~2 min ios 📦zip
✖️ b8317c6 #27 2023-08-07 12:15:48 ~2 min tests 📄log

protocol/linkpreview/linkpreview.go Show resolved Hide resolved
protocol/linkpreview/linkpreview.go Outdated Show resolved Hide resolved
protocol/linkpreview/linkpreview_test.go Outdated Show resolved Hide resolved
@ibrkhalil ibrkhalil force-pushed the status-mobile-16457 branch from 7e19552 to fab8dea Compare August 1, 2023 10:44
protocol/linkpreview/linkpreview.go Show resolved Hide resolved
protocol/linkpreview/linkpreview.go Outdated Show resolved Hide resolved
@ibrkhalil ibrkhalil force-pushed the status-mobile-16457 branch from a84b3fa to b4790ec Compare August 2, 2023 11:51
@ibrkhalil ibrkhalil force-pushed the status-mobile-16457 branch from 9896d31 to 9085414 Compare August 5, 2023 18:38
@ilmotta
Copy link
Contributor

ilmotta commented Aug 14, 2023

Hey @ibrkhalil, is there anything holding this PR from advancing? I'm already working on changes that will touch the same files, so we'll face conflicts for sure.

@ibrkhalil
Copy link
Contributor Author

Hey @ibrkhalil, is there anything holding this PR from advancing? I'm already working on changes that will touch the same files, so we'll face conflicts for sure.

Hey Icaro, I am awaiting a reply from design team on this comment

@ibrkhalil
Copy link
Contributor Author

Hey @ibrkhalil, is there anything holding this PR from advancing? I'm already working on changes that will touch the same files, so we'll face conflicts for sure.

Hey Icaro, I am awaiting a reply from design team on this comment

I posted it on their Discord server

@ilmotta
Copy link
Contributor

ilmotta commented Aug 15, 2023

@ibrkhalil, interesting proposal status-im/status-mobile#16814 (comment) for processing gifs.

The feature I'm working on is to add support for static image URLs, which, if we go the route you and @cammellos suggested of taking a snapshot of a single gif frame, means image URLs and gif URLs will share a lot in common code-wise and behavior-wise.

The agreed flow with image URLs is that the image will be unfurled once on the sender side in a lower resolution and compressed image, and then on the client side, the user will need to explicitly press on the preview thumbnail to see the full image, a la Discord.

Just one question @cammellos @ibrkhalil, why can't we unfurl and persist the full gif (heavily preprocessed of course) and then serve it to clients? Are there technical limitations? Or is it just for simplicity at this stage of the product?

The ideal behavior in my head would be:

  1. Preprocess gifs: lower their frame rate & resolution, and compress them.
  2. Persist on waku the preprocessed gifs and the URL to the original resource (in case the user wants to see the gif in its full glory).
  3. On clients, by default, show the preprocessed gifs (not just a static frame), but allow users to see the original resource if they so want. This step is essentially the same we'll go for static images.

The benefit, I believe, is that the majority of users will be happy to see only the preprocessed (also interactive) gifs, and they will never pay the privacy cost because they won't bother seeing the full gif in the original website. If we decide to show only a static frame of the gif on clients, it kind of diminishes the value of the gif feature, because I believe a lot of users will still press on the image to the see the interactive gif, but this will cost them on privacy. You see, the static gif snapshot sort of invites users to interact with centralized servers because humans are curious and they want to see more frames 🤷🏼

@cammellos
Copy link
Contributor

@ilmotta If we can keep gifs size lower than 300/400KB I think sending through waku is fine and preferable, @Samyoul has worked on the compression, not sure it supports gifs.
If the size is higher though, we might want to do as discussed (click to view gif).
A play button though is also useful for performance, as a side note, so that they don't consume cpu cycles by sending multiple gifs that are all animating, but that's a different issue.

@ilmotta
Copy link
Contributor

ilmotta commented Aug 15, 2023

@ilmotta If we can keep gifs size lower than 300/400KB I think sending through waku is fine and preferable, @Samyoul has worked on the compression, not sure it supports gifs. If the size is higher though, we might want to do as discussed (click to view gif).

I sincerely have no idea if average smartphones can quickly process gifs to make them fit this sizing constraint. Some gifs are almost like full short videos, so preprocessing them could take a considerable amount of time on mobile devices. That's why I asked if there's any technical limitation, but I guess we would need to quickly try on status-go and see? Unless status-go isn't equipped now to do this sort of processing on gifs, then it could be too much effort for the MVP (?).

A play button though is also useful for performance, as a side note, so that they don't consume cpu cycles by sending multiple gifs that are all animating, but that's a different issue.

100%. Addtionally, so many users hate autoplay gifs.

@Samyoul
Copy link
Member

Samyoul commented Aug 15, 2023

Hey @ilmotta / @cammellos sorry for the late reply.

Gifs in status-go.

Gifs manipulation is not supported currently in status-go, but it can be. Gifs manipulation in Go is slightly different to jpg or other raster image format types. A gif (as you may expect) is an array of frames, and each frame needs to be processed individually and then the gif wrapper data needs to be constituted (which can be fiddly, or at least isn't straight forward).

Back in the day I did do some basic work on gif support in status-go, but the biggest issue is that the native mobile image picker would "typecast" the image as a JPEG and send only the first the frame of the gif, with all the gif metadata stripped out. I didn't want to spend time working more on this so I parked the feature.

If we are pulling the gif from a URL the image being mangled by the image selector isn't a problem. But it would be really nice to also fix that issue as many phones have gif selection options built into the keyboard and inject the image data as if it was selected from the phone's storage.

Issues about computational power, that is a tricky one.

For processing the image data, in general most image manipulation is handled easily by even low powered phones, we really don't do anything complex. Cropping is very fast (basically trimming arrays), resize is also quick (2D array nearest neighbour(s) value averaging). These are the 2 main processes we use. In the jpg specific context we also run many multiple quality level renders to discover the optimal jpg quality level to ideal file size, this is also fast even for low spec devices.

For running the gifs that is a different scenario, it really depends on the gif and the device. So maybe auto-stop playing once a gif is out of frame, and default no-auto play (Although I like seeing all the gifs play like a nausea inducing circus of chaos.).

@ilmotta
Copy link
Contributor

ilmotta commented Aug 15, 2023

Thank you for the detailed explanation @Samyoul.

Back in the day I did do some basic work on gif support in status-go, but the biggest issue is that the native mobile image picker would "typecast" the image as a JPEG and send only the first the frame of the gif, with all the gif metadata stripped out. I didn't want to spend time working more on this so I parked the feature.

If we are pulling the gif from a URL the image being mangled by the image selector isn't a problem. But it would be really nice to also fix that issue as many phones have gif selection options built into the keyboard and inject the image data as if it was selected from the phone's storage.

Agreed 👍🏼

From what I understood, it seems we have some unknowns that may turn out to be blockers. In this case, the static image approach to gifs is starting to grow on me :)

Gifs manipulation is not supported currently in status-go, but it can be. Gifs manipulation in Go is slightly different to jpg or other raster image format types. A gif (as you may expect) is an array of frames, and each frame needs to be processed individually and then the gif wrapper data needs to be constituted (which can be fiddly, or at least isn't straight forward).

Since animated wepb can deliver similar quality for lower sizes compared to gif, I wonder if mobile supports animated webp, and if that could be of any help. This link is quite useful https://developers.google.com/speed/webp/faq#why_should_i_use_animated_webp This could be interesting anyway for the desktop app.

For processing the image data, in general most image manipulation is handled easily by even low powered phones

I'm impressed, cool!


@cammellos, that was all! I guess we can evolve gif previews in later iterations.

@ilmotta ilmotta mentioned this pull request Aug 16, 2023
@ibrkhalil ibrkhalil marked this pull request as draft August 28, 2023 12:06
@igor-sirotin
Copy link
Collaborator

@ilmotta do you know if this PR is still needed?
status-im/status-mobile#16457 seems to be resolved, but there's some long discussion here 🤔

@ilmotta
Copy link
Contributor

ilmotta commented Sep 3, 2024

@ilmotta do you know if this PR is still needed? status-im/status-mobile#16457 seems to be resolved, but there's some long discussion here 🤔

Good finding @igor-sirotin. We can close the issue because there are too many open points and, at least on Mobile, it's not in our priorities and probably won't be for a long time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Messages containing Giphy/Tenor links are not delivered to receiver
6 participants